Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

e2e test for withdrawals L2-initiate -> forking -> L1-finalize #143

Merged
merged 22 commits into from
Jan 3, 2024
Merged

e2e test for withdrawals L2-initiate -> forking -> L1-finalize #143

merged 22 commits into from
Jan 3, 2024

Conversation

josojo
Copy link
Collaborator

@josojo josojo commented Jan 2, 2024 •
edited
Loading

/**

  • This test simulates the withdrawal process after a fork
  • At chainstart the genesis is similar to the output of the genesis script form this repo
  • In the next block a test account makes a deposit of 10 native tokens (forkonomic tokens)
  • on L2 into the bridge. The transaction was created with the script despoitForkonomicTokenIntoBridgeInL2.js.
  • Then this state advancement with the deposit is verified with a snark proof and submitted. See this PR for the actual state advancement:
  • preparing proof inputs for withdrawal e2e test zkevm-commonjs#2
  • After that the fork is initiated and executed. We then test the withdrawal process.

*/

@josojo josojo changed the base branch from noremappingForHardhat to main January 3, 2024 07:59
josojo
josojo commented Jan 3, 2024
View reviewed changes
contracts/ForkableBridge.sol
@@ -185,7 +185,7 @@ contract ForkableBridge is
uint256 amount,
bool useFirstChild,
bool useChildTokenAllowance
) public onlyForkManger onlyAfterForking {
) public onlyAfterForking {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since all tokens are no longer accessible in the old bridge contract and they need to be withdrawn in the new contract, I think its fine if everyone can push them to the new chain. But I will rework this in one of my next PRs anyway

edmundedgar
edmundedgar approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@edmundedgar edmundedgar left a comment

Choose a reason for hiding this comment

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

Approved apart from my minor quibble with the wording of that comment

contracts/ForkableBridge.sol Outdated
@@ -175,7 +175,7 @@ contract ForkableBridge is
}

/**
* @dev Allows the forkmanager to take out the forkonomic tokens
* @dev Allows to take out the forkonomic tokens
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: Allows -> Allows anyone

Copy link
Contributor

Choose a reason for hiding this comment

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

Come to think of it I would write this as
Allows anyone to take out their forkonomic tokens

@josojo josojo merged commit ad52d54 into main Jan 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmundedgar edmundedgar edmundedgar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@josojo @edmundedgar