Skip to content

Add sequencer role to oracle#2810

Merged
tynes merged 9 commits intodevelopfrom
m/oracle-rbac
Jun 22, 2022
Merged

Add sequencer role to oracle#2810
tynes merged 9 commits intodevelopfrom
m/oracle-rbac

Conversation

@maurelian
Copy link
Copy Markdown
Contributor

@maurelian maurelian commented Jun 17, 2022

Description

Introduces a new role, so that the OutputOracle now has the following:

  • owner which can:

    • delete outputs
    • change the sequencer's address
    • transfer ownership

  • sequencer which can:

    • append new outputs

Metadata

  • Fixes ENG-2255

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jun 17, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 7b717a2

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

This PR includes changesets to release 1 package
Name Type
@eth-optimism/contracts-bedrock Minor

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

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jun 17, 2022

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

@mergify mergify bot added the conflict label Jun 17, 2022
@mergify mergify bot removed the conflict label Jun 17, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jun 17, 2022

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

@mergify mergify bot added the conflict label Jun 17, 2022
@mergify mergify bot removed the conflict label Jun 17, 2022
@maurelian maurelian marked this pull request as ready for review June 17, 2022 19:57
tynes
tynes reviewed Jun 17, 2022
View reviewed changes
@maurelian maurelian force-pushed the m/oracle-rbac branch 2 times, most recently from 72bb71b to fbc4b3f Compare June 20, 2022 17:50
@maurelian maurelian mentioned this pull request Jun 20, 2022
Merged
@tynes
Copy link
Copy Markdown
Contributor

tynes commented Jun 21, 2022

Generally looks good to me, looks like the go bindings are out of date which is causing the CI to fail

@maurelian
Copy link
Copy Markdown
Contributor Author

maurelian commented Jun 21, 2022 •
edited
Loading

@tynes:

We could add something to CI that warns on storage slot changes too if we want to

This is a good idea. WDYT of this approach?

  1. Create a directory in the bedrock package called .layouts
  2. Create a file for each relevant contract in that directory (ie. .layouts/OptimismPortal)
  3. Create simple bash script that runs forge inspect <ContractName> storage-layout .layouts/<ContractName>
  4. Fail if git diff on the file is non-empty

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Jun 21, 2022

@tynes:

We could add something to CI that warns on storage slot changes too if we want to

This is a good idea. WDYT of this approach?

  1. Create a directory in the bedrock package called .layouts
  2. Create a file for each relevant contract in that directory (ie. .layouts/OptimismPortal)
  3. Create simple bash script that runs forge inspect <ContractName> storage-layout .layouts/<ContractName>
  4. Fail if git diff on the file is non-empty

See foundry-rs/foundry#2056

This was referenced Jun 21, 2022
Closed
Merged
@maurelian maurelian force-pushed the m/oracle-rbac branch 3 times, most recently from 481dd7c to 496f7b1 Compare June 22, 2022 16:06
tuxcanfly
tuxcanfly reviewed Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

Running forge test I'm seeing a test failure. Tried rebuilding from scratch too. Weird since CI seems to pass.

[FAIL. Reason: OutputOracle: Blockhash does not match the hash at the expected height.] test_appendWithBlockhashAndHeight() (gas: 20419)

@maurelian
Copy link
Copy Markdown
Contributor Author

maurelian commented Jun 22, 2022

Odd, I can't repro that.
@tuxcanfly what if you try forge test --force to recompile the contracts?

@tuxcanfly
Copy link
Copy Markdown
Contributor

tuxcanfly commented Jun 22, 2022

Same.

@tuxcanfly
Copy link
Copy Markdown
Contributor

tuxcanfly commented Jun 22, 2022

forge 0.2.0 (123ad0a 2022-06-13T00:04:04.19891Z)

@maurelian
Copy link
Copy Markdown
Contributor Author

maurelian commented Jun 22, 2022

oh.. updating forge does indeed break it for me too... :/

Nicca42
Nicca42 suggested changes Jun 22, 2022
View reviewed changes
@maurelian maurelian requested review from Nicca42 and tynes June 22, 2022 19:16
tynes
tynes reviewed Jun 22, 2022
View reviewed changes
@maurelian
Copy link
Copy Markdown
Contributor Author

maurelian commented Jun 22, 2022

rebuilding bindings now

@tynes tynes merged commit a828da9 into develop Jun 22, 2022
@tynes tynes deleted the m/oracle-rbac branch June 22, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes approved these changes

@smartcontracts smartcontracts Awaiting requested review from smartcontracts

@Nicca42 Nicca42 Awaiting requested review from Nicca42

+2 more reviewers

@tuxcanfly tuxcanfly tuxcanfly left review comments

@mslipper mslipper mslipper approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maurelian @tynes @tuxcanfly @mslipper @Nicca42