Conversation
tynes
force-pushed
the
spec/predeploys
branch
4 times, most recently
from
b86d5cf to
4b5c5d9
Compare
Codecov Report
@@ Coverage Diff @@
## main #379 +/- ##
==========================================
+ Coverage 51.50% 51.66% +0.15%
==========================================
Files 70 70
Lines 7582 7582
==========================================
+ Hits 3905 3917 +12
+ Misses 3154 3145 -9
+ Partials 523 520 -3
Continue to review full report at Codecov.
|
|
The following script was used to find the existing predeploys: import { providers, utils } from 'ethers'
import * as fs from 'fs'
const outfile = 'predeploys.json'
;(async function () {
const provider = new providers.StaticJsonRpcProvider('https://mainnet.optimism.io')
const prefixes = [
'0x420000000000000000000000000000000000',
'0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD'
]
// we don't need to search the entire space
// const MAX_U16 = 0xffff
const predeploys = []
for (const prefix of prefixes) {
for (let i = 0; i <= 100; i++) {
const buf = Buffer.alloc(2);
buf.writeUInt16BE(i, 0);
const addr = utils.hexConcat([prefix, utils.hexZeroPad(buf, 2)])
predeploys.push(addr)
}
}
const detected = []
for (const predeploy of predeploys) {
const code = await provider.getCode(predeploy)
if (code !== '0x') {
console.log(`Predeploy detected at ${predeploy}`)
detected.push({
address: predeploy,
code: code,
})
}
}
fs.writeFileSync(outfile, JSON.stringify(detected, null, 2))
})()Results: $ cat predeploys.json | jq -r '.[].address'
0x4200000000000000000000000000000000000000
0x4200000000000000000000000000000000000002
0x4200000000000000000000000000000000000006
0x4200000000000000000000000000000000000007
0x420000000000000000000000000000000000000f
0x4200000000000000000000000000000000000010
0x4200000000000000000000000000000000000011
0x4200000000000000000000000000000000000012
0x4200000000000000000000000000000000000013
0xdeaddeaddeaddeaddeaddeaddeaddeaddead0000
|
maurelian
left a comment
maurelian
left a comment
There was a problem hiding this comment.
Looks good. Left a couple nits worth fixing. I'd also suggest updating the glossary here:
with this diff:
- Optimism has the following predeploys:
-
-- [L1 Attributes Predeployed Contract][l1-attr-predeploy]
+ All predeploy contracts are specified in the [predeploys specification][./predeploys.md].
|
Thank you for the review! Addressed your nits |
|
For some reason there is a CircleCI build error |
specs/predeploys.md
Outdated
| * @notice The Withdrawer contract facilitates sending both ETH value and data from L2 to L1. | ||
| * It is predeployed in the L2 state at address 0x4200000000000000000000000000000000000016. | ||
| */ | ||
| contract Withdrawer { |
There was a problem hiding this comment.
nit: Can we embed contracts directly? My concern is them getting out of date. Another question is will we want to upgrade the pre-deploys? This is pretty out of scope and doesn't need to be addressed as part of this PR.
There was a problem hiding this comment.
The point here is that its all interfaces and that we can transparently change the predeploys in a backwards compatible way. I would much rather link to actual solidity files instead of embedding the source code in markdown, so I could add an interfaces directory if preferred
There was a problem hiding this comment.
Oh nice. That makes a bunch of sense (I just skimmed an saw solidity, not the fact that it was only interfaces).
norswap
left a comment
norswap
left a comment
There was a problem hiding this comment.
Quick not super in depth review. Biggest comment is we should be clearer about the guarantees we're giving, esp. for the deprecated stuff.
|
|
||
| Predeployed smart contracts exist on Optimism at predetermined addresses in | ||
| the genesis state. They are similar to precompiles but instead run directly | ||
| in the EVM instead of running native code outside of the EVM. |
There was a problem hiding this comment.
It could be said that some of these contracts don't really exist in the "genesis" state since historical transaction would not have been able to use them. It's as though they were included in the genesis but act as empty addresses until they become active at a certain block.
I guess with the regenesis, we only really have two class: (1) true (re)genesis contracts and (2) new bedrock contracts.
There was a problem hiding this comment.
Once we know the height at which we switch to bedrock, we could add that to this document. Maybe it could go into a file dedicated to backwards compatibility?
specs/predeploys.md
Outdated
| ## OVM\_L2ToL1MessagePasser | ||
|
|
||
| The `OVM_L2ToL1MessagePasser` is part of the legacy bridge. It is being | ||
| deprecated as part of the bedrock upgrade. |
There was a problem hiding this comment.
What does deprecation mean? Will it still be usable / work correctly but use is discourage & it could be removed in the future?
specs/predeploys.md
Outdated
| /** | ||
| * @title OVM_L2ToL1MessagePasser | ||
| * @dev The L2 to L1 Message Passer is a utility contract which facilitate an L1 proof of the | ||
| * of a message on L2. The L1 Cross Domain Messenger performs this proof in its |
There was a problem hiding this comment.
"of the of a message"
Would suggest rewriting. e.g. it's not clear here that an "L1 proof" is a proof that happens on L1, or why/how this facilitates that.
There was a problem hiding this comment.
Once the contracts are in the same repo as the specs, we should add links to the contracts themselves as to not need to maintain these here
specs/predeploys.md
Outdated
| legacy system, this contract was hooked into during `CREATE` and `CREATE2` to | ||
| ensure that the deployer was allowlisted. In the bedrock system, this will | ||
| be removed from the `CREATE` codepath since arbitrary contract deployment | ||
| has already been enabled. |
There was a problem hiding this comment.
I think we need to clarify (and this could be done up top) that some of these contracts are unused / might not function anymore, but need to be preserved as predeploys to guarantee the execution of historic blocks.
A good thing would be to come up with a taxonomy of these contracts, e.g.
- functional
- deprecated / functional (list preferred alternative)
- deprecated / non-functional or useless
There was a problem hiding this comment.
I've added a note at the top of the document saying that deprecated predeploys should not be used as well as adding a column to the table that includes whether or not a contract is deprecated
| The `OVM_ETH` contains the ERC20 represented balances of ETH that has been | ||
| deposited to L2. As part of the bedrock upgrade, the balances will be migrated | ||
| from this contract to the actual Ethereum level accounts to preserve EVM | ||
| equivalence. |
There was a problem hiding this comment.
What happens to the public functions here, will they still work?
There was a problem hiding this comment.
I've added a note that this contract's usage should be avoided, some important functions (transfer) do not work currently
|
@norswap great feedback, thank you! Will address |
|
This PR will need to be rebased and updated based on #421 |
|
I've pushed a fix to this PR to address comments. I've removed the majority of the interfaces so that we don't need to maintain the interfaces here as well as in the repo. We can link directly to the contracts once they are all in the same repo |
|
Closing in favor of ethereum-optimism/optimism#2568 |
5 participants
Description
PR to add specs for the predeploys. These need to be upgraded in a backwards compatible way for bedrock, its worth specifying their interfaces since they will always be in the state.