Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Integrate RMNRemote and OffRamp #1360
Integrate RMNRemote and OffRamp #1360
Changes from all commits
f5d823a
178f28e
fa27937
b90c9d4
93b0189
4546f02
81d9075
da570c5
336513c
5369bb1
c61c35e
3315c1a
ecab50b
c39bfc3
9201354
29fc6e9
5557b1e
d286c8b
8a79cbf
207e166
3597ed5
60ecdc0
24ab8e3
522d0e1
796523a
8b2ff48
aaed373
afec448
0e7f614
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
A basic docstring on the expected behavior of this function might be useful (e.g maybe for implementing mocks), i.e "verifies RMN signatures against provided MerkleRoot structs and reverts on invalid signatures"
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 we want to be accurate these aren't signatures on the merkle roots exactly. They're signatures on a particular RMN report that consists of "lane updates" that match the updates in the
merkleRoots
array above. Maybe reference RMNRemote.verify() for folks who want to see the exact message being signed.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.
you're right. If I make additional changes, I'll fix the comment :)
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.
q: do we want to keep this internal function? feels like it can be inlined now.
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.
+1 this is a single line
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
_verify()
function is overwritten in tests to support mocking verification results. Changing this would be a big reafactor of the test suite and would probably increase code complexity there. Opting to leave this as is unless you guys feel strongly.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.
It feels a bit wrong to have a one liner internal function just because of tests. We can punt to another PR but I'd be in favor of removing it.
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'll spend a little more time on it today and see how annoying it will be in tests. I was admittedly being a touch lazy yesterday 🙃.
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.
FYI easy solve: add a function in the offRampHelper that just sets
s_roots[sourceChainSelector][root]
to whatever you want. Should make the testing trivialThere 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'm going to push back again here. I don't think the juice is worth the squeeze. Here is an example of what these changes would look like. It's a super marginal contract size/gas savings in the offramp but a more annoying test suite with a fair amount of code duplication b/t the tests and the implementation. IMHO this increases the overall code complexity.
I think maintaining some modularity for the sake of easier testing should be seen as an intentional feature in this case!