-
Notifications
You must be signed in to change notification settings - Fork 0
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
inbox contract v2 #3
Conversation
```solidity | ||
/// @notice Updates the batch inbox address. Can only be called by the owner. | ||
/// @param _batchInbox New batch inbox address. | ||
function setBatchInbox(address _batchInbox) external onlyOwner { |
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.
What happens if _batchInbox
is set to 0
?
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.
The 0 address will be used as inbox, which is also EOA.
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.
I believe if address
is 0, then op-node will be Rollup config's inbox to derive. Please double check.
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.
Currently ProcessSystemConfigUpdateLogEvent
doesn't support dynamic update for inbox, it only supports dynamic update for batcher address. It will be modified to handle inbox change.
|
||
### SystemConfig | ||
|
||
The `SystemConfig` is the source of truth for the address of inbox. It stores information about the inbox address and passes the information to L2 as well. |
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.
We should describe how initialize
works for the address (whether the current behavior is the same or not?).
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.
Added a section for initialize
: ebddbec
specs/experimental/inbox-contract.md
Outdated
## How It Works | ||
|
||
The integration process consists of three primary components: | ||
1. Replacement of the [`BatchInboxAddress`](https://github.com/ethereum-optimism/optimism/blob/db107794c0b755bc38a8c62f11c49320c95c73db/op-chain-ops/genesis/config.go#L77) with an inbox contract: The existing `BatchInboxAddress`, which currently points to an Externally Owned Account (EOA), will be replaced by a smart contract. This new inbox contract will be responsible for verifying and enforcing batch submission conditions. |
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.
The term replacement
may be misleading as the inbox address can be initialized in the Rollup config (so no replacement is needed).
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.
Good point.
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.
The term replacement
is now gone: 2f22e44
specs/experimental/inbox-contract.md
Outdated
|
||
## Upgrade | ||
|
||
Existing OP Stack instances need to upgrade the `SystemConfig` and set an inbox contract in order to use this feature. |
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.
I don't think this step is necessary to use the inbox contract: we can use the old SystemConfig
. This is only needed when performing dynamic upgrade
(or migration).
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.
If the contract is not updated, there will be no ConfigUpdate(VERSION, UpdateType.BATCH_INBOX, data)
event.
} | ||
``` | ||
|
||
### L1Block |
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.
I would suggest following the description of custom gas token:
L1Block
The L1Block contract is upgraded with a permissioned function setGasPayingToken that is used to set information about the gas paying token. The DEPOSITOR_ACCOUNT MUST be the only account that can call the setter function. This setter is differentiated from the setL1BlockValues functions because the gas paying token is not meant to be dynamic config whereas the arguments to setL1BlockValues are generally dynamic in nature.
Any L2 contract that wants to learn the address of the gas paying token can call the getter on the L1Block contract.
|
||
Immediately before submitting a new batch, `op-batcher` fetches the current inbox address from L1 and submits to that address. After the transaction is successfully included in L1 at block `N`, `op-batcher` verifies that the inbox address hasn't changed at block `N`. If the address has changed, it resubmits the batch to the new address. | ||
|
||
## Upgrade |
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.
I would recommend to follow https://github.com/ethstorage/specs/blob/7248e23a6c93053a5cae5906c8521f30b55165c0/specs/experimental/custom-gas-token.md#upgrade for a better description of upgrade.
e360b4b
to
a062516
Compare
a062516
to
01a41b0
Compare
4b700d7
to
f47c082
Compare
Close since already submitted to upstream |
Second version for ethereum-optimism#284