Skip to content

feat(ctb): Two Step Withdrawals V2#3836

Merged
mslipper merged 30 commits intodevelopfrom
@clabby/two-step-withdrawals
Nov 11, 2022
Merged

feat(ctb): Two Step Withdrawals V2#3836
mslipper merged 30 commits intodevelopfrom
@clabby/two-step-withdrawals

Conversation

@clabby
Copy link
Contributor

@clabby clabby commented Nov 2, 2022 •
edited
Loading

#Overview

Description

Adds an implementation of the Two Step Withdrawals V2 proposal inspired by #3677.

Checklist

  • Contract changes
    • Sanity check uint128 struct packing.
    • Split up finalizeWithdrawTransaction
    • Port and double-check integrity of tests from feat(ctb): introduces two step withdrawals #3677
      • Add explicit test_proveWithdrawalTransaction_success test
      • Add explicit test for when the output proposal is modified after proving the withdrawal hash. (this branch)
  • SDK changes.
    • Update devnet tests
  • Update bedrock integration tests
  • Update E2E tests
    • Update @tynes' new TestCrossLayerUser test
  • Update Indexer (Will be done in a later PR)
    • Update indexer tests
  • Final cleanups / resolve conflicts.

Tests

Contracts:

  • Tests modified in accordance with @smartcontracts' draft implementation tests:
    • CommonTest.FFIInterface
    • OptimismPortal_FinalizeWithdrawal_Test
      Added:
      • test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts
      • test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts

SDK Tasks

  • Modified deposit-erc20.ts task
  • Modified deposit-eth.ts task

Bedrock Integration Tests:

  • Modified 000_withdraw.spec.ts

op-e2e tests:

  • Modified TestWithdrawals
  • Modified TestCrossLayerUser

Indexer tests

  • Modified TestBedrockIndexer

Additional context

n/a

Metadata

n/a

@changeset-bot
Copy link

changeset-bot bot commented Nov 2, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: d200b02

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@clabby clabby marked this pull request as draft November 2, 2022 00:12
@github-actions github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Nov 2, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

tynes
tynes reviewed Nov 2, 2022
View reviewed changes
@tynes
Copy link
Contributor

tynes commented Nov 2, 2022

These changes will prob break a bunch of tests, happy to help out with fixing them. We have tests in op-e2e that will probably break, this will likely change the api of the sdk which will break the devnet tests. Just let me know when you are ready to work on those things

@tynes
Copy link
Contributor

tynes commented Nov 3, 2022

We will also need to add to the readme docs for installing the rust toolchain with versions if we do decide to start using rust. I have a slight preference for migrating our fuzz testing infra to go because we already have all of the primitives written in go so we don't need to write them again in rust and it'll improve coverage of our go primitives, which are used throughout the op-node and various tests

maurelian
maurelian reviewed Nov 3, 2022
View reviewed changes
Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Some minor comments.

@mergify
Copy link
Contributor

mergify bot commented Nov 3, 2022

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Nov 3, 2022
@github-actions github-actions bot added the A-pkg-sdk Area: packages/sdk label Nov 3, 2022
@mergify mergify bot added the sdk label Nov 3, 2022
@mergify mergify bot requested review from nickbalestra and roninjin10 November 3, 2022 19:39
@mergify mergify bot removed the conflict label Nov 3, 2022
@clabby clabby force-pushed the @clabby/two-step-withdrawals branch from 73a732e to f1ce43a Compare November 3, 2022 20:34
@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2022

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Nov 4, 2022
@mergify mergify bot added the A-indexer Area: indexer label Nov 4, 2022
@clabby clabby force-pushed the @clabby/two-step-withdrawals branch from 06a22e2 to e9223c4 Compare November 10, 2022 19:18
@tynes
Copy link
Contributor

tynes commented Nov 10, 2022

If we can confirm that the devnet failure is a flake, I think this is ready for merge
https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/9473/workflows/a087a594-099c-44d6-87e0-be66130b42e7/jobs/211631

protolambda
protolambda reviewed Nov 10, 2022
View reviewed changes
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Go part of the PR looks good to me. One small thing about the withdrawal finalization time in the action testing though, and there is more testing we should follow up with in later in PRs.

@clabby
Copy link
Contributor Author

clabby commented Nov 10, 2022 •
edited
Loading

If we can confirm that the devnet failure is a flake, I think this is ready for merge https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/9473/workflows/a087a594-099c-44d6-87e0-be66130b42e7/jobs/211631

I believe this is a flake- spinning up my local devnet to double check.

EDIT: Appears to have been a flake, re-running CI now.
Screen Shot 2022-11-10 at 5 22 35 PM

@clabby clabby force-pushed the @clabby/two-step-withdrawals branch from e98e579 to 609751f Compare November 10, 2022 23:34
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022 •
edited
Loading

Codecov Report

Merging #3836 (d200b02) into develop (d188e79) will decrease coverage by 0.85%.
The diff coverage is 15.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3836      +/-   ##
===========================================
- Coverage    43.77%   42.92%   -0.86%     
===========================================
  Files          328      307      -21     
  Lines        17070    16507     -563     
  Branches       767      665     -102     
===========================================
- Hits          7473     7086     -387     
+ Misses        9094     8914     -180     
- Partials       503      507       +4
Flag Coverage Δ
bedrock-go-tests 37.89% <0.00%> (-0.11%) ⬇️
contracts-bedrock-tests 38.61% <100.00%> (+0.71%) ⬆️
contracts-governance-tests 83.05% <ø> (ø)
contracts-periphery-tests 96.49% <ø> (ø)
contracts-tests 98.92% <ø> (ø)
core-utils-tests ?
dtl-tests 47.15% <ø> (ø)
fault-detector-tests 34.18% <ø> (ø)
sdk-tests 40.01% <1.29%> (-1.85%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-node/withdrawals/utils.go 4.26% <0.00%> (ø)
packages/sdk/src/cross-chain-messenger.ts 54.21% <0.00%> (-3.28%) ⬇️
packages/sdk/tasks/deposit-erc20.ts 6.28% <0.00%> (-0.38%) ⬇️
packages/sdk/tasks/deposit-eth.ts 5.40% <0.00%> (-0.53%) ⬇️
packages/sdk/tasks/finalize-withdrawal.ts 14.00% <0.00%> (-3.08%) ⬇️
.../contracts-bedrock/contracts/L1/OptimismPortal.sol 94.44% <100.00%> (+1.58%) ⬆️
packages/sdk/src/interfaces/types.ts 100.00% <100.00%> (ø)
op-node/p2p/discovery.go 65.97% <0.00%> (-4.13%) ⬇️
op-node/sources/batching.go 83.92% <0.00%> (-1.79%) ⬇️
...ges/core-utils/src/optimism/deposit-transaction.ts
... and 20 more

@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2022

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

mergify bot added a commit that referenced this pull request Nov 11, 2022
@mergify
Merge of #3836
e6a7c14
@mergify mergify bot mentioned this pull request Nov 11, 2022
Closed
91 tasks
@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2022

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

mergify bot added a commit that referenced this pull request Nov 11, 2022
@mergify
Merge of #3836
7cc0d05
@mergify mergify bot mentioned this pull request Nov 11, 2022
Closed
91 tasks
mergify bot added a commit that referenced this pull request Nov 11, 2022
@mergify
Merge of #3836
f5efded
@mergify mergify bot mentioned this pull request Nov 11, 2022
Closed
91 tasks
@mslipper mslipper merged commit 1bfe79f into develop Nov 11, 2022
@mslipper mslipper deleted the @clabby/two-step-withdrawals branch November 11, 2022 02:03
@mergify mergify bot removed the on-merge-train label Nov 11, 2022
This was referenced Nov 21, 2022
Closed
Merged
Merged
This was referenced Dec 7, 2022
Merged
Merged
nitaliano pushed a commit that referenced this pull request May 20, 2024
@clabby
feat(ctb): Two Step Withdrawals V2 (#3836)
6ff8c71 
* Start contract changes for two step withdrawals v2

* Fix maurelian's nits

* Refactor Kelvin's SDK changes; SDK/integration test time

* Merge w/ `develop`

* Add tests for changed output proposal *after* proving the withdrawal hash

Whoops

* Gas snapshot / comments

* Regenerate bindings; Fix E2E Withdrawal test; Add extra indexed params to `WithdrawalProven`

* Start fixing indexer integration tests

* Fix conflicts; Start updating mark's new `op-e2e` withdrawal action tests

* Remove proposal timestamp >= withdrawal timestamp check

* Fix mark's `op-e2e` test + add docs to `proveMessage` in SDK

* Update changeset

* Lint contracts

* Merge with `develop`

* Re-order mapping declarations so that `finalizedWithdrawals` retains its old storage slot

* Merge with `develop`

* Start updating devnet tests

* Fix devnet tests

* Update ERC20 binding

* Clean up SDK

* Merge with `develop`

* Remove `integration-tests-bedrock` package

* Add check for equality between locally computed withdrawal hash vs. on-chain withdrawal hash

* Add Kelvin's check + complimentary test

Update bindings

* Fix finalization period in `TestCrossLayerUser`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartcontracts smartcontracts smartcontracts left review comments

@maurelian maurelian maurelian left review comments

@tynes tynes tynes approved these changes

@nickbalestra nickbalestra Awaiting requested review from nickbalestra

@roninjin10 roninjin10 Awaiting requested review from roninjin10

+2 more reviewers

@protolambda protolambda protolambda left review comments

@mslipper mslipper mslipper approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-indexer Area: indexer A-pkg-contracts-bedrock Area: packages/contracts-bedrock A-pkg-sdk Area: packages/sdk

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@clabby @tynes @mslipper @codecov-commenter @smartcontracts @protolambda @maurelian