Feat: Enforce versioning when rollup push/consume messages #1245
Conversation
✅ Deploy Preview for preeminent-bienenstitch-606ad0 canceled.
|
| @@ -87,6 +90,9 @@ contract OutboxTest is Test { | |||
| // correctly set message.recipient to this address | |||
| _message.recipient = DataStructures.L1Actor({actor: address(this), chainId: block.chainid}); | |||
|
|
|||
| // correctly set the message.sender.version | |||
There was a problem hiding this comment.
can we add a wrong version test in outbox too?
| returns (uint256) | ||
| { | ||
| (, bool exists) = _getVersionFor(_rollup); | ||
| if (exists) revert Errors.Registry__RollupAlreadyRegistered(_rollup); |
There was a problem hiding this comment.
can we just add a dumb test to ensure this never gets deleted
There was a problem hiding this comment.
Added a registry test file 👍
| bytes32[] memory entryKeys = new bytes32[](1); | ||
| entryKeys[0] = entryKey; | ||
|
|
||
| vm.prank(wrongRollup); |
There was a problem hiding this comment.
can you add a comment which says that "this fails because the rollup version is 2 but we have sent a message for 1"?
There was a problem hiding this comment.
The test is named testRevertIfConsumingFromWrongRollup and
vm.expectRevert(
abi.encodeWithSelector(Errors.Inbox__InvalidVersion.selector, version, wrongVersion)
);makes the explicit check one line later than this.
LHerskind
force-pushed
the
lh/359-rollup-version-consumption
branch
from
a860bbb
to
8358a48
Compare
|
😭 @iAmMichaelConnor remember to format. |
|
|
||
| function _getVersionFor(address _rollup) internal view returns (uint256 version, bool exists) { | ||
| version = rollupToVersion[_rollup]; | ||
| exists = version > 0 || snapshots[0].rollup == _rollup; |
There was a problem hiding this comment.
could be me being silly, but above it states that the version should start from 1. Why do we ned to check 0. I guess its fine as the check should short circuit but interested in the explicit check
There was a problem hiding this comment.
There is still the dead one at 0. But might be useless to support returning that 🤷
There was a problem hiding this comment.
yeah thats what im thinking, we shouldnt really care whats in 0 if the protocol returns non existent for 0.
There was a problem hiding this comment.
Actually, it is also ensuring that you cannot insert the dead again. But that might also be useless so have removed it.
Whoops, wasn't aware. Can't we get a commit hook to do that? |
LHerskind
force-pushed
the
lh/359-rollup-version-consumption
branch
from
be56a7e
to
6f71507
Compare
# Description Fixes #359 and #624. # Checklist: - [ ] I have reviewed my diff in github, line by line. - [ ] Every change is related to the PR description. - [ ] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to the issue(s) that it resolves. - [ ] There are no unexpected formatting changes, superfluous debug logs, or commented-out code. - [ ] The branch has been merged or rebased against the head of its merge target. - [ ] I'm happy for the PR to be merged at the reviewer's next convenience.
Description
Fixes #359 and #624.
Checklist: