feat: add ace protocols #785
Conversation
Will-Smith11
changed the title
Will-Smith11
changed the title
| /// Handle ACE unlocking interaction after successful simulation. | ||
| /// Returns optional cancellation OrderId if a mempool unlock cancels an optional ACE tx. | ||
| /// Note: NonUnlocking ACE interactions are handled at the OrderSimResult level. | ||
| pub fn handle_ace_unlock( |
There was a problem hiding this comment.
Should take as args simulated_order,previous_orders since it's always called in match SimulatedResult::Success no need to recheck that.
| pub fn handle_ace_unlock( | ||
| &mut self, | ||
| result: &SimulatedResult, | ||
| ) -> Result<Vec<OrderId>, ProviderError> { |
There was a problem hiding this comment.
Never returns error!!! Remove Result
| "Cancelling optional ACE unlock for {:?} - user unlock exists", | ||
| contract_address | ||
| ); | ||
| cancellations.push(simulated_order.order.id()); |
There was a problem hiding this comment.
It's cancelling it's own order?
It should only cancell the order returned by mark_mempool_unlock.
If it wants to express that the order analized should not be inserted it should return something like a bool saying it so we don’t send and order and it’s cancellation.
| }; | ||
|
|
||
| // If this order already has parents, it was re-simulated - just pass through | ||
| if !previous_orders.is_empty() { |
There was a problem hiding this comment.
Will return for a user tx having a previous nonce dep, are we ok with this?
| contract_address | ||
| ); | ||
| } | ||
| AceUnlockSource::User => { |
There was a problem hiding this comment.
There is NO guaranty that a AceUnlockSource::User will be executed before orders depending o it.
A depending order could make a lot of money so it could be placed before the AceUnlockSource::User.
There was a problem hiding this comment.
This should be fine. otherwise we are making a lot more downstream changes
| &mut self, | ||
| results: Vec<SimulatedResult>, | ||
| ) -> Result<(), ProviderError> { | ||
| ) -> Result<(Vec<SimulatedResult>, Vec<OrderId>), ProviderError> { |
There was a problem hiding this comment.
nobody uses the Vec in the result. Remove it.
crates/rbuilder/src/building/sim.rs
Outdated
| if let Some(contract_address) = failure.ace_dependency { | ||
| // Order failed but needs ACE unlock - queue for re-simulation | ||
| // Pass existing ace_unlock_contracts for progressive multi-ACE discovery | ||
| sim_tree.add_ace_dependency_for_order( |
There was a problem hiding this comment.
Why simulate_all_orders_with_sim_tree is calling "sim_tree.add_ace_dependency_for_order" manually? Isn't submit_simulation_tasks_results doing that already?
| let pending_state = self.get_order_dependency_state(&ready_pending_order.order)?; | ||
| match pending_state { | ||
| OrderNonceState::Ready(parents) => { | ||
| OrderDependencyState::Ready(mut parents) => { |
There was a problem hiding this comment.
Here parents contains only Nonce dependencies and not any other prev ACE dependency.
If the order was depending in another ACE dependency that will not be included.
If looks like you should add all deps in PendingOrder::ace_unlock_contracts not only this one last unlocked.
crates/rbuilder/src/building/sim.rs
Outdated
| order: Order, | ||
| unsatisfied_nonces: usize, | ||
| unsatisfied_dependencies: usize, | ||
| /// ACE contracts already provided as unlock parents (for progressive multi-ACE discovery) |
There was a problem hiding this comment.
Shouldn't this commet just be "ACE contract needed to be unlocked"?
You can have this pending without alread having an unlock parent.....
crates/rbuilder/src/building/sim.rs
Outdated
| // Check if we already have an unlock provider for the new contract | ||
| if self.dependency_providers.contains_key(&dep_key) { | ||
| // Build parents from ALL ACE unlock contracts we need | ||
| let mut parents = Vec::new(); |
There was a problem hiding this comment.
You are not adding Nonce deps here.
|
From the comment I made in add_ace_dependency_for_order and process_simulation_task_result it feels that you need a common func that computes the parent. |
| // Use HashMap to track best interaction per contract | ||
| let mut per_contract: HashMap<Address, AceInteraction> = HashMap::default(); | ||
|
|
||
| for (tx, _) in order.list_txs() { |
There was a problem hiding this comment.
All txts are being iterated, even if they where droped
| // Check this transaction against all ACE configs | ||
| for (_, config) in ace_configs.iter() { | ||
| if let Some(interaction) = | ||
| classify_ace_interaction(trace, sim_success, config, selector, tx_to, tx_from) |
There was a problem hiding this comment.
sim_success is for the whole Order, it's not related to the particular execution of this Tx.
This might be a problem since we don't have at the moment trace state per child tx.
Eg: For a Bundle we could have a Dropeable/Revertable Tx accessing the pool and failing but the bundle succeding so we would have classify_ace_interaction answering Unlocking.
To detect Force/Optional unlocking orders we could asume that Order must be a single Tx.
2 participants
📝 Summary
Adjusted the ingestion pipeline to allow for specific interactions with known ace protocols to modify the simulation process to allow orders, in which we know are valid with a defined ace protocol through. Also some minor changes to the block building process putting protocol ace tx's that don't interact with state at the top
💡 Motivation and Context
Currently Other builders have started to include ace protocols and we want to also have buildernet cover this. Ace protocols have specific sequencing rules enforced on-chain that allow them to dictate execution order. By allowing for ace protocols to integrate into builders, It unlocks more transactions to execute in the same block that wouldn't be able to otherwise. The increase in transaction flow should help the builder stay competitive.
Questions
This is my first time in the codebase. I wanted to open up this PR so I could get some feedback to ensure that with the given architecture that everything is implemented in the right locations. I will add a bunch of tests once I know that the architecture is solid, just don't want to do a bunch more work when I'm uncertain about design.
✅ I have completed the following steps:
make lintmake test