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
Keystone: Forwarder contract updates #12962
Keystone: Forwarder contract updates #12962
Changes from all commits
a431ed3
56a4c11
dbfef76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is the owner actually an ethereum address? Or is it just some private key? I didn't want to make assumptions here since this also won't be the same
address
type across all chainsThere 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 assume 20 bytes gives us enough space to support the "owner" concept more generally. Nothing prevents us from having a different type for other chains as long as it can fit into 20 bytes. We can also pad 🤷♂️
I didn't want to add gas costs unnecessarily for the EVM chains (happy path).
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'd type it as byte20 then to not imply an actual ethereum address -- for me that usually means a wallet or an on chain contact
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.
Nit: I typically expect to see a link to the Jira ticket if you leave the TODO in the code. Otherwise, these aren't visible in Jira.
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.
@archseer what do you mean by expiration?
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.
In OCR2Aggregator changing the config also expires any past in-flight transmissions since the config digest will change (and needs to match on transmit).
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're not sending the digest alongside our reports are we? I thought we will only rely purely on signature validation.
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 don't but the question is do we want something equivalent for expiration?
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.
Created a ticket to investigate: https://smartcontract-it.atlassian.net/browse/KS-202?atlOrigin=eyJpIjoiMGM1NGQ4ZTFmMDg1NDZlZjgxOWIyNDA4MjUzZGJjOTYiLCJwIjoiaiJ9
This file was deleted.