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.

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

feat: allow rollup update #83

Merged
merged 17 commits into from
Oct 24, 2023
Merged

feat: allow rollup update #83

merged 17 commits into from
Oct 24, 2023

Conversation

gzeoneth
Copy link
Member

@gzeoneth gzeoneth commented Oct 24, 2023

This PR included 2 changes that should only affect privileged methods

  • Allow rollup admin to update rollup addresses in Bridge contracts, and synchronizing those changes to Outbox, RollupEventInbox, and Sequencer Inbox. These changes are audited as part of BOLD in Added upgrade action for the bold upgrade bold#340 but for proxy admin instead of rollup admin. Allowing rollup admin to call those function make the upgrade executor being able to call those function easier (or otherwise would require a upgradeAndCall via the ProxyAdmin contract).

  • Introduce whenNotPausedOrDeprecated modifier, which allow validator to withdraw their stake when the rollup is deprecated by changing the rollup address in the bridge to something else

Superseding

@gzeoneth gzeoneth requested review from yahgwai and gvladika October 24, 2023 12:15
@cla-bot cla-bot bot added the s label Oct 24, 2023
gvladika
gvladika previously approved these changes Oct 24, 2023
Copy link
Contributor

@gvladika gvladika left a comment

Choose a reason for hiding this comment

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

lgtm, might be useful to emit RollupUpdated in Bridge

yahgwai
yahgwai previously approved these changes Oct 24, 2023
src/rollup/RollupUserLogic.sol Outdated Show resolved Hide resolved
@@ -789,6 +802,44 @@ describe('ArbRollup', () => {
await rollup.confirmNextNode(challengerNode)
})

it('allow force refund staker with pending node', async function () {
await rollupAdmin.connect(await impersonateAccount(upgradeExecutor)).pause()
Copy link
Contributor

Choose a reason for hiding this comment

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

missing .wait() - and on some other transactions below

Copy link
Member Author

Choose a reason for hiding this comment

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

that should not be necessary right? otherwise the test should be failing

Copy link
Contributor

Choose a reason for hiding this comment

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

could just be getting lucky with timing. You might get intermittent failures with this

@gzeoneth gzeoneth dismissed stale reviews from yahgwai and gvladika via e8aac68 October 24, 2023 15:36
@gzeoneth gzeoneth requested review from yahgwai and gvladika October 24, 2023 15:39
@gzeoneth gzeoneth merged commit 5dbca3c into develop Oct 24, 2023
3 checks passed
@gzeoneth gzeoneth deleted the allow-rollup-update branch October 24, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants