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

Add a batch poster manager for the sequencer inbox #74

Merged
merged 21 commits into from
Jan 24, 2024
Merged

Add a batch poster manager for the sequencer inbox #74

merged 21 commits into from
Jan 24, 2024

Conversation

yahgwai
Copy link
Contributor

@yahgwai yahgwai commented Oct 5, 2023 •
edited by gzeoneth
Loading

  • At this point we should consider inheriting openzepplin.AccessControl as we now have three roles
    • Rollup owner
    • batch poster
    • batch poster manager

@cla-bot cla-bot bot added the s label Oct 5, 2023
gzeoneth
gzeoneth previously approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM

DZGoldman
DZGoldman previously approved these changes Oct 24, 2023
View reviewed changes
@yahgwai yahgwai changed the base branch from develop to seq-inbox-opt October 26, 2023 12:36
gzeoneth
gzeoneth previously approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM
Agree that inheriting OZ AccessControl can be nice, but that would mess up storage. One potential solution is we have a BridgeStorage base contract that inherit the current storage layout so it doesn't get shifted downward.

This was referenced Nov 2, 2023
Closed
Draft
DZGoldman
DZGoldman suggested changes Jan 10, 2024
View reviewed changes
src/bridge/SequencerInbox.sol Show resolved Hide resolved
src/bridge/SequencerInbox.sol Outdated Show resolved Hide resolved
@DZGoldman
Copy link
Contributor

as discussed: probably makes sense get #94 & #107 merged and then include batch poster management as a role on the Bridge

@gzeoneth gzeoneth self-requested a review January 19, 2024 11:39
@gzeoneth gzeoneth changed the base branch from seq-inbox-opt to 4844-only January 19, 2024 12:11
@gzeoneth
Copy link
Member

Pushed changes to base this on 4844-only, some additional changes includes:

  1. Rollup Owner is always a batch poster manager
  2. Batch poster manager control both isSequencer and isBatchPoster
  3. Revert to proxy pattern instead of immutable, batch poster manager can be set during initialization

@yahgwai @gzeoneth
Update src/bridge/SequencerInbox.sol
b43718a 
Co-authored-by: gzeon <hng@offchainlabs.com>
yahgwai
yahgwai commented Jan 22, 2024
View reviewed changes
Copy link
Contributor Author

@yahgwai yahgwai left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from 4844-only to develop January 24, 2024 13:04
@gzeoneth gzeoneth dismissed their stale review January 24, 2024 13:04

The base branch was changed.

gzeoneth
gzeoneth previously approved these changes Jan 24, 2024
View reviewed changes
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM

@gzeoneth gzeoneth merged commit c755485 into develop Jan 24, 2024
5 checks passed
@gzeoneth gzeoneth deleted the batch-poster-manager branch January 24, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PlasmaPower PlasmaPower PlasmaPower left review comments

@gzeoneth gzeoneth gzeoneth left review comments

@DZGoldman DZGoldman DZGoldman approved these changes

Assignees
No one assigned
Labels
s seq-inbox
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yahgwai @DZGoldman @gzeoneth @PlasmaPower