optimism
optimism copied to clipboard
op-chain-ops: implement withdrawal hashing
Description
Implement both the new hashing scheme and the legacy hashing for withdrawals so that the withdrawals can be migrated. This only includes tests for legacy style hashing, will follow up with an additional PR to add tests for bedrock style withdrawal hashing to prevent this PR from growing too large.
Closes ENG-2718
⚠️ No Changeset found
Latest commit: 8151bb888f0e21f9271a52f2e2b744e0f20aaaf7
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?
Can get test vectors from here: https://optimistic.etherscan.io/address/0x4200000000000000000000000000000000000000#internaltx
Recusing myself from review here; others will have more insightful feedback.
LGTM - one Q about some test setup. I really like how it compares the computed slot vs what get set in the trace.
Good call on catching that error, I have updated it to panic on error of abi deserialization
I've added some additional functionality to this PR:
- Split the withdrawal type into
LegacyWithdrawalandWithdrawal - encoding + decoding with roundtrip fuzz tests of both withdrawal types
- Test coverage of the storage slot generation for the
Withdrawaltype- Test vectors generated using foundry, this is important for the storage slot migration
i have addressed the issues, thanks for the review
This PR has been added to the merge queue, and will be merged soon.
Not sure why this is failing in CI but not locally for me
It fails for me locally as well.
@tynes is there an issue with the files checked in / something around the path where it's not able to load in the test cases?
@tynes is there an issue with the files checked in / something around the path where it's not able to load in the test cases?
Found the source of the bug, it was due to the way that the L2ToL1MessagePasser is now being handled. Instead of migrating the state at address(0x42..00) we are introducing a new predeploy at address(0x42..16) that will receive the migrated hashes and the proof on L1 will work against that account.
This PR has been added to the merge queue, and will be merged soon.
This PR is next in line to be merged, and will be merged as soon as checks pass.