-
Notifications
You must be signed in to change notification settings - Fork 109
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
refactor: move hardcoded value to config #270
Conversation
require(_delaySeconds == delaySeconds, "DelayBuffer: delaySeconds"); | ||
require(_futureBlocks == futureBlocks, "DelayBuffer: futureBlocks"); | ||
require(_futureSeconds == futureSeconds, "DelayBuffer: futureSeconds"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have to remove these checks for contract code size, I don't think there are too much risk but arguable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - just one (or two) small change
Co-authored-by: Henry <[email protected]>
@@ -433,17 +442,6 @@ contract BOLDUpgradeAction { | |||
PROXY_ADMIN_SEQUENCER_INBOX.upgrade(sequencerInbox, IMPL_SEQUENCER_INBOX); | |||
} | |||
|
|||
// verify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there manual checklist somewhere that we could add these to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet but we can make something
Note that the 4bytes of
RollupCreator.createRollup
andRollupAdminLogic.initialize
will be changed, won't be an issue if we have the change with the v3 release because they was already different from v2.