E2E Test For Hook-Enabled ERC-1271 Signatures #1832
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nlordell
force-pushed
the
e2e-eip1271-pre-hook
branch
from
ffcce9e to
57948a0
Compare
fleupold
approved these changes
Sep 1, 2023
There was a problem hiding this comment.
Very cool. This allows for some very nice use cases, where users can e.g. bridge funds to a specific mainnet address and sign an order in the name of that address in which the safe is deployed in a pre-hook and proceeds are swapped back to their address on the original chain in a post hook.
| fn domain_separator(&self) -> DomainSeparator { | ||
| let mut buffer = [0_u8; 96]; | ||
| buffer[0..32].copy_from_slice(&hex!( | ||
| "47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218" |
There was a problem hiding this comment.
What are these magic numbers?
There was a problem hiding this comment.
Ah sorry - meant to include links to code but forgot. Will update.
There was a problem hiding this comment.
Added some documentation with links to the code that was ported and where the magic numbers come from.
nlordell
force-pushed
the
reader-magic
branch
from
282617c to
ba63849
Compare
nlordell
force-pushed
the
e2e-eip1271-pre-hook
branch
from
c82b060 to
04549bf
Compare
MartinquaXD
approved these changes
Sep 5, 2023
nlordell
pushed a commit
that referenced
this pull request
Sep 6, 2023
Follow up to #1827 (comment) - thanks @fleupold for the wonderful idea! This PR changes instances of `eth_call` with state override simulations **where we are only overriding the Settlement contract code** to make use of `StorageAccessible`. It does this with an additional support `Reader` contract in order to avoid needing to manage deployments on-chain for the `StorageAccessible` target (i.e. the reader logic). See the comment in the `Reader.sol` file for more details on how this works. Specifically, we changed the following two simulations to no longer require state overrides (i.e. we do not need a special node to handle those requests, and it will Just Work™ on Gnosis Chain): * Account balance simulations with pre-hooks * ERC-1271 signature validation with pre-hooks In a follow up PRs I will: 1. #1832 2. Remove the redundant `Web3` strategy for both account balance and signature verification simulation ### Release notes Hooks E2E test continues to pass **without the nasty `----account-balances-optimistic-pre-interaction-handling=true` hack** which used to disable balance checks for orders with pre-hooks. This means that not only are we E2E testing that executing orders with pre-hooks are working, we are also checking that balance simulations work 🎉. You can double check this is actually doing something by applying a patch like this: ```diff diff --git a/crates/e2e/tests/e2e/hooks.rs b/crates/e2e/tests/e2e/hooks.rs index 42d8d072..84d978b5 100644 --- a/crates/e2e/tests/e2e/hooks.rs +++ b/crates/e2e/tests/e2e/hooks.rs @@ -66,7 +66,7 @@ async fn test(web3: Web3) { full: json!({ "metadata": { "hooks": { - "pre": [permit, steal_cow], + "pre": [/*permit, */steal_cow], "post": [steal_weth], }, }, ``` And see the E2E test fail as expected with: ``` thread 'hooks::local_node_test' panicked at 'called `Result::unwrap()` on an `Err` value: (400, "{\"errorType\":\"InsufficientAllowance\",\"description\":\"order owner must give allowance to VaultRelayer\"}")', crates/e2e/tests/e2e/hooks.rs:83:41 ``` I also ran the `cargo test -p shared -- --nocapture --ignored account_balances::simulation` manual test which continues to work.
nlordell
force-pushed
the
e2e-eip1271-pre-hook
branch
from
04549bf to
27b0509
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now, thanks to #1831, we no longer depend on
eth_callwith state override support for simulating ERC-1271 signature validation for orders with pre-hooks. This means that we can finally write an E2E test for this!This PR does just that - it executes an order on behalf of a Safe that is created just-in-time for the settlement as part of a pre-hook. Note that we assert that the ERC-1271 order is created for a Safe with no code!
Test Plan
This is the test!