Skip to content

op-chain-ops: legacy withdrawal script refactoring#5474

Merged
OptimismBot merged 5 commits intodevelopfrom
fix/debug-migrated-withdrawals
Apr 18, 2023
Merged

op-chain-ops: legacy withdrawal script refactoring#5474
OptimismBot merged 5 commits intodevelopfrom
fix/debug-migrated-withdrawals

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented Apr 17, 2023

Description

Clean up the script that is used to migrate legacy withdrawals.
This script is very important for testing the legacy withdrawal
migration.

A bug was introduced with #4911
which was a sherlock audit fix. This was a bug because the change
resulted in the withdrawal hashes changing, meaning that the storage
slots computed client side also changed. This resulted in breaking the
script. This commit reverts this change. The changes landing in
#5470 will cause the
buffer in the gas limit for the migrated withdrawals to be too small,
so we can reintroduce this code behind a switch with the network.
Its not ideal that the migration code isn't going to match 1:1
with goerli but thats ok because we will be able to thoroughly
test it.

@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2023

⚠️ No Changeset found

Latest commit: 0ac98a2

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

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 0ac98a2
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/643eab63875acf0008f4efe0

Clean up the script that is used to migrate legacy withdrawals.
This script is very important for testing the legacy withdrawal
migration.

A bug was introduced with #4911
which was a sherlock audit fix. This was a bug because the change
resulted in the withdrawal hashes changing, meaning that the storage
slots computed client side also changed. This resulted in breaking the
script. This commit reverts this change. The changes landing in
#5470 will cause the
buffer in the gas limit for the  migrated withdrawals to be too small,
so we can reintroduce this code behind a switch with the network.
Its not ideal that the migration code isn't going to match 1:1
with goerli but thats ok because we will be able to thoroughly
test it.
@tynes tynes force-pushed the fix/debug-migrated-withdrawals branch from d199051 to 7bf4d54 Compare April 17, 2023 21:39
@mergify mergify bot added the sdk label Apr 17, 2023
@mergify mergify bot requested a review from roninjin10 April 17, 2023 21:39
@tynes
Copy link
Contributor Author

tynes commented Apr 17, 2023

If these fixes are merged and then #5459 is rebased on top of this, it should fix @roninjin10's issue. Will need to actually see it work but feeling pretty sure that the combo of these fixes will work

@tynes tynes marked this pull request as ready for review April 17, 2023 21:44
@tynes tynes requested review from a team as code owners April 17, 2023 21:44
@tynes tynes requested a review from sebastianst April 17, 2023 21:44
@sebastianst sebastianst requested review from mslipper and removed request for sebastianst April 17, 2023 21:50
@roninjin10
Copy link
Contributor

Amazing!

@mslipper
Copy link
Collaborator

Is this a pure revert, or was additional functionality introduced?

@tynes
Copy link
Contributor Author

tynes commented Apr 17, 2023

Is this a pure revert, or was additional functionality introduced?

I left in the unit tests and modularization that enabled the unit tests but the underlying functionally was reverted

@tynes
Copy link
Contributor Author

tynes commented Apr 18, 2023

Can I get a review from @ethereum-optimism/ecopod ?

@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@OptimismBot OptimismBot merged commit 2bdbeee into develop Apr 18, 2023
@OptimismBot OptimismBot deleted the fix/debug-migrated-withdrawals branch April 18, 2023 16:06
@mergify mergify bot removed the on-merge-train label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants