Skip to content

Comments

refactor[contracts]: Port OVM_DeployerWhitelist to use ovm-solc#548

Merged
smartcontracts merged 3 commits intomasterfrom
ovm-solc/deployer-whitelist
Apr 22, 2021
Merged

refactor[contracts]: Port OVM_DeployerWhitelist to use ovm-solc#548
smartcontracts merged 3 commits intomasterfrom
ovm-solc/deployer-whitelist

Conversation

@smartcontracts
Copy link
Contributor

@smartcontracts smartcontracts commented Apr 21, 2021

Description
Aaaand another continuation of #475. Moving OVM_DeployerWhitelist over to ovm-solc. We're touching such critical parts of the contracts that I think it's important we review each contract individually.

@changeset-bot
Copy link

changeset-bot bot commented Apr 21, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 1ad5fd2

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 Patch

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

@smartcontracts smartcontracts marked this pull request as ready for review April 21, 2021 23:17
@smartcontracts smartcontracts changed the title Port OVM_DeployerWhitelist to use ovm-solc refactor[contracts]: Port OVM_DeployerWhitelist to use ovm-solc Apr 21, 2021
@tynes
Copy link
Contributor

tynes commented Apr 21, 2021 •
edited
Loading

This contract can't have a constructor that sets the owner? We can't forget to call this on L2 post regenesis
Other solutions:

  • take-dump.ts calls initialize() with a configurable value
  • We use the geth config option address-manager-owner and set the storage slot on startup

ben-chain
ben-chain approved these changes Apr 21, 2021
View reviewed changes
Copy link
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmCREATE.spec.ts
const NON_WHITELISTED_DEPLOYER = '0x1234123412341234123412341234123412341234'
const NON_WHITELISTED_DEPLOYER_KEY =
'0x0000000000000000000000001234123412341234123412341234123412341234'
const NON_WHITELISTED_DEPLOYER_KEY = ethers.utils.keccak256(
Copy link
Collaborator

@ben-chain ben-chain Apr 21, 2021

Choose a reason for hiding this comment

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

Assuming that this can't be easily done because it lives in smock right now, but this should really be a call to a utility function. Took me a sec to realize this was here to handle the new solidity storage layout.

Copy link
Contributor Author

@smartcontracts smartcontracts Apr 21, 2021

Choose a reason for hiding this comment

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

Yeah, very true!

tynes
tynes requested changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Please add a changeset and is this safe to merge to master?

@smartcontracts
Copy link
Contributor Author

smartcontracts commented Apr 21, 2021

Added changeset. Only place where this might be backwards incompatible is if we slapped the code on the existing storage layout. And users would have to have their accounts "re-whitelisted". I think we already have to do this for each regenesis though, right?

@tynes
Copy link
Contributor

tynes commented Apr 22, 2021

Still wondering about my questions above in #548 (comment), thanks for adding the changeset. Added regenesis comment here: ethereum-optimism/scripts#12 (comment)

maurelian
maurelian requested changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

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

All good except the one question about allowing arbitrary deployments before initialization.

@smartcontracts @maurelian
Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploy…
1ad5fd2 
…s/OVM_DeployerWhitelist.sol

Co-authored-by: Maurelian <maurelian@protonmail.ch>
@tynes
Copy link
Contributor

tynes commented Apr 22, 2021

Will approve pending open issue regarding removal of the initialize()

@smartcontracts smartcontracts mentioned this pull request Apr 22, 2021
Closed
@smartcontracts
Copy link
Contributor Author

smartcontracts commented Apr 22, 2021

Will approve pending open issue regarding removal of the initialize()

Created #560 to cover this issue.

maurelian
maurelian approved these changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

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

LGTM

@smartcontracts smartcontracts merged commit d4ee2d7 into master Apr 22, 2021
@smartcontracts smartcontracts deleted the ovm-solc/deployer-whitelist branch April 22, 2021 18:29
@ben-chain ben-chain mentioned this pull request Apr 23, 2021
Closed
2 tasks
InoMurko referenced this pull request in omgnetwork/optimism May 25, 2021
@smartcontracts @maurelian
refactor[contracts]: Port OVM_DeployerWhitelist to use ovm-solc (#548)
c986f5b 
* Port OVM_DeployerWhitelist to use ovm-solc

* chore[contracts]: Add changeset

* Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_DeployerWhitelist.sol

Co-authored-by: Maurelian <maurelian@protonmail.ch>

Co-authored-by: Maurelian <maurelian@protonmail.ch>
agusduha pushed a commit that referenced this pull request Sep 25, 2025
@0xDiscotech
Merge pull request #548 from defi-wonderland/chore/sync-fee-splitter-…
652c4cc 
…system
theochap pushed a commit that referenced this pull request Dec 10, 2025
@clabby
chore(workspace): Allow stdlib in cfg(test) (#548)
6830edb 
* chore(workspace): Allow stdlib in `cfg(test)`

* rebase

* rebase
theochap pushed a commit that referenced this pull request Jan 15, 2026
@RomanHodulak
feat(consensus): Add OpTransaction trait (#548)
1843d37 
Closes #543 

## Motivation

To accept extended transactions composed of `OpTransaction`s

## Solution

Implement the trait for `OpTxEnvelope` and `Extended`

## PR Checklist

- [ ] Added Tests
- [ ] Added Documentation
- [ ] Breaking changes
theochap pushed a commit that referenced this pull request Jan 15, 2026
@RomanHodulak
feat(consensus): Add as_deposit into OpTransaction trait (#549)
ca2e5b9 
Follow-up on #548 

## Motivation

`OpTransaction` trait can tell if its deposit, but that is not very
useful if I also need to extract it.

## Solution

Add `as_deposit` function into the trait for the extraction of
`TxDeposit`.

## PR Checklist

- [ ] Added Tests
- [ ] Added Documentation
- [ ] Breaking changes
theochap pushed a commit that referenced this pull request Jan 21, 2026
@RomanHodulak
feat(consensus): Add as_deposit into OpTransaction trait (alloy-r…
175f8b9 
…s/op-alloy#549)

Follow-up on #548 

## Motivation

`OpTransaction` trait can tell if its deposit, but that is not very
useful if I also need to extract it.

## Solution

Add `as_deposit` function into the trait for the extraction of
`TxDeposit`.

## PR Checklist

- [ ] Added Tests
- [ ] Added Documentation
- [ ] Breaking changes
emhane added a commit that referenced this pull request Feb 4, 2026
@emhane @Himess
chore(trie): return OpProofsStorageError from unwind_history (#548)
78984ef 
Cherry picks commit from op-rs/op-reth#540

Co-authored-by: Himess <semihcvlk53@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maurelian maurelian maurelian approved these changes

@tynes tynes tynes approved these changes

@ben-chain ben-chain ben-chain 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.

4 participants

@smartcontracts @tynes @ben-chain @maurelian