Builder based API for simulator crate#4372
Conversation
| // GPv2AllowListAuthentication stores `mapping(address => bool) managers` | ||
| // at storage slot 1. Solidity mapping key: keccak256(address_padded ++ | ||
| // slot_padded). | ||
| // <https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2AllowListAuthentication.sol#L22> |
There was a problem hiding this comment.
The managers are at slot 0, no?
| // GPv2AllowListAuthentication stores `mapping(address => bool) managers` | |
| // at storage slot 1. Solidity mapping key: keccak256(address_padded ++ | |
| // slot_padded). | |
| // <https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2AllowListAuthentication.sol#L22> | |
| // GPv2AllowListAuthentication stores `address manager` at slot 0 | |
| // and `mapping(address => bool) solvers` at slot 1. Solidity | |
| // mapping key: keccak256(address_padded ++ slot_padded). | |
| // <https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2AllowListAuthentication.sol#L22> |
There was a problem hiding this comment.
Yes, the one manager address is at slot 0 but that's irrelevant for this state override so I didn't mention it in the comment.
There was a problem hiding this comment.
I read the original comment as managers at storage slot 1
There was a problem hiding this comment.
Ahh, now I get it. It should be mapping(address => bool) solvers not mapping(address => bool) managers
| .metadata | ||
| .full_app_data | ||
| .clone() | ||
| .ok_or_else(|| OrderSimulationError::Other(anyhow!("can't find appdata")))?; |
There was a problem hiding this comment.
Why does it return an error now?
In any case, the error message is pretty vague.
| .ok_or_else(|| OrderSimulationError::Other(anyhow!("can't find appdata")))?; | |
| .ok_or_else(|| OrderSimulationError::Other(anyhow!( | |
| "order has no full_app_data stored, cannot simulate hooks/wrappers without it" | |
| )))?; |
There was a problem hiding this comment.
For some time now we do expect to have the full appdata for all orders in the DB. If we can't find it we can't know whether we simulate the order correctly.
Agree that the error message should be improved. Will do that.
| - signingScheme | ||
| - signature |
There was a problem hiding this comment.
OrderSimulationRequest in crates/orderbook/src/dto/mod.rs declares app_data: String, fee_amount: U256, valid_to: u32, partially_fillable: bool - none Option, none #[serde(default)]. Serde will 422 when any is missing.
| - signingScheme | |
| - signature | |
| - signingScheme | |
| - signature | |
| - feeAmount | |
| - validTo | |
| - partiallyFillable |
| app_data: AppDataHash(app_data_hash.into()), | ||
| partially_fillable: request.partially_fillable, | ||
| }) | ||
| .with_signature(request.owner, request.signature) |
There was a problem hiding this comment.
Did I get it correctly that this endpoint is now a different shape than before? The actual request.signature gets encoded, and the trader is no longer overridden with Trader::DEPLOYED_BYTECODE, so signature verification really runs. Existing callers who POSTed without a signature (relying on the old fake-EIP-1271 override) will start getting reverts.
The PR description hints at this ("not require the fake EIP 1271 user contract"), but it'd be worth being explicit in the changelog / breaking-changes section so nobody gets surprised.
There was a problem hiding this comment.
Yes, that's exactly right. The old approach was simply not able to verify a large number of sophisticated flows so it had to be updated. Will make this clearer in the description.
| self.pre_interactions = self.encode_hooks(&protocol.hooks.pre); | ||
| self.post_interactions = self.encode_hooks(&protocol.hooks.post); |
There was a problem hiding this comment.
Should this not override the interactions, but extend them?
| self.pre_interactions = self.encode_hooks(&protocol.hooks.pre); | |
| self.post_interactions = self.encode_hooks(&protocol.hooks.post); | |
| self.pre_interactions.extend(self.encode_hooks(&protocol.hooks.pre)); | |
| self.post_interactions.extend(self.encode_hooks(&protocol.hooks.post)); |
There was a problem hiding this comment.
True. To simplify things for now I dropped interactions from the order completely. This can of worms only needs to be opened when the simulator is tasked with generating the calldata for the actual settlement txs.
There was a problem hiding this comment.
If I didn't miss anything, now that Order::pre_interactions is gone, the only way to add pre/post interactions is through the builder. But the builder is inconsistent about what its setters do:
with_pre_interactions/with_post_interactionsappend to whatever is already there.parameters_from_app_datareplaces whatever is already there.
So if someone uses both, the call order silently decides what survives:
// foo is gone — parameters_from_app_data replaced it with the hooks
.with_pre_interactions([foo]).parameters_from_app_data(json)?
// foo and the hooks both end up in pre_interactions
.parameters_from_app_data(json)?.with_pre_interactions([foo])
Easiest fix is to make parameters_from_app_data also append, so order doesn't matter:
self.pre_interactions.extend(self.encode_hooks(&protocol.hooks.pre));
self.post_interactions.extend(self.encode_hooks(&protocol.hooks.post));
There was a problem hiding this comment.
True. That was an oversight in my fix. 👍
Fixed now.
| - signature | ||
| - feeAmount | ||
| - validTo | ||
| - partiallyFillable |
|
|
||
| /// Generates 1 interaction executing the given hooks via the trampoline | ||
| /// contract since executing hooks directly from the settlement contract | ||
| /// context would give them elevated priviliges that puts funds at risk. |
There was a problem hiding this comment.
| /// context would give them elevated priviliges that puts funds at risk. | |
| /// context would give them elevated privileges that put funds at risk. |
| #[error("overrides use incompatible storage strategies (state vs state_diff)")] | ||
| StateAndStateDiff, |
There was a problem hiding this comment.
Wouldn't a better variant name be: "IncompatibleStorageStrategies"?
As is, it's a bit confusing when reading code that returns MergeConflict::State and MergeConflict::StateAndStateDiff
| current_block: CurrentBlockWatcher, | ||
| tenderly: Option<Arc<dyn tenderly::Api>>, | ||
| ) -> Result<Self> { | ||
| let authenticator = Address(settlement.authenticator().call().await?.0); |
There was a problem hiding this comment.
Nit: I think .call().await? already returns Address here (see solvers/src/infra/config/dex/file.rs:155 which just uses the call result directly), so Address(... .0) is an identity wrap.
let authenticator = settlement.authenticator().call().await?;The DomainSeparator(...) on the next line is fine, that one actually needs to convert B256 -> [u8; 32].
| Full, | ||
| /// Fill whatever is still remaining on-chain (queries the settlement | ||
| /// contract for the already-filled amount and subtracts it). Falls back to | ||
| /// the full amount if the query fails. |
There was a problem hiding this comment.
Doc says it falls back to the full amount if the query fails, but executed_amount in encoding.rs propagates the error via BuildError::FilledAmountQuery(...)?. It should either probably be unwrap_or(full) in the code, or the doc needs to be updated to say it errors out.
| /// Construct with [`Order::new`] and add optional fields via the builder | ||
| /// methods. Defaults to an EIP-1271 signature (pairs with [`FakeUser`] for | ||
| /// simulations that need to bypass signature verification). | ||
| pub struct Order { |
There was a problem hiding this comment.
FakeUser doesn't exist in the workspace anymore - looks like a leftover from the old approach. Either drop the parenthetical or point at whatever replaced it (looks like callers are now expected to call with_signature themselves).
Description
This new API for the
simulatorcrate approaches the problem from the other side. Instead of starting by making it work first and foremost with the most complicated use case (i.e. trade verification) and building the simpler, more common use cases around that this API starts with the simple stuff and aims to be flexible enough to also support the more complicated things.Not only does this lead to a cleaner API (IMO) but also actual feature improvements. For example the trade verification logic replaces the user with a helper contract that we puppeteer to set up the necessary pre-conditions for quote verification. That helper contract makes the approach fundamentally incompatible with use cases where a pre-interaction deploys a smart contract that is actually the owner of the order (we did not have a way to know the state and bytecode of this helper contract to fall back to).
The new approach now allows us to easily opt-in to each "faking machinery" individually. So the simulation endpoint will only use balance overrides and allow listing a random address while the trade verification can pull out the big guns (many balance overrides, multiple fake contracts).
Additionally this PR adds the ability to correctly encode flashloans which was previously not supported by the
simulatorcrate.Changes
AnyoneAuthenticatorapproach with exactly overriding only the storage slot to allow list a specific address as a solverImportant
This PR breaks the API for debugging custom orders. Due to supporting more features the request now also has to contain the
signature,signingScheme,feeAmount,validTo,appData, andpartiallyFillable.Things to improve
Ultimately I would like the
simulatorcrate also to be the authority when it comes to encoding the calldata of actual settlements (which is why it already handles encoding the auction id). However, this is quite difficult to do nicely because now we have to think about correctly encoding many prices and managing wrappers and hooks from multiple orders.For simplicity sake the builder currently does not concern itself with that. That's why you can only add wrappers and interactions from 1 appdata string directly and there is no support for encoding prices such that orders get surplus.
How to test
updated existing e2e tests (populating the new fields in the custom order simulation request)