-
Notifications
You must be signed in to change notification settings - Fork 36
First draft TV2 #1151
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
base: main
Are you sure you want to change the base?
First draft TV2 #1151
Conversation
| uint256 length = teleporterMessage.receipts.length; | ||
| for (uint256 i; i < length; ++i) { | ||
| _markReceipt(blockchainID_, warpMessage.sourceChainID, teleporterMessage.receipts[i]); | ||
| _markReceipt(blockchainID, message.sourceBlockchainID, teleporterMessage.receipts[i]); |
Check notice
Code scanning / Semgrep PRO
Semgrep Finding: solidity.performance.state-variable-read-in-a-loop.state-variable-read-in-a-loop Note
| ) { | ||
| blockchainID = bytes32(0); // TODO decide how to set this for external chains | ||
| messageVerifier = IMessageVerifier(verifierSender); | ||
| messageSender = IMessageSender(verifierSender); |
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.
Shouldn't messageVerifier and messageSender result from different contract addresses rather than verifierSender?
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.
So... If we use Nick's method (and I strongly think we should), then the message sender and verifier should be in the same contract, that way they're located at the same address. Otherwise we need to do initialization which is messy. I just have these interfaces copied from Jacob's spec, because I haven't thought of a better name than verifierSender for the unified contract.
| } | ||
|
|
||
| struct ICMMessage { | ||
| TeleporterMessage unsignedMessage; |
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 TeleporterMessage is always unsigned right? I think this can just be left as message (or unattested Message) since the attestation field is what is used to authenticate it. A signature would just be a specific case of an attestation, e.g. in Warp it is a BLS aggregate signature.
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.
Yeah, good point
Why this should be merged
How this works
How this was tested
How is this documented