-
Notifications
You must be signed in to change notification settings - Fork 36
External interop design docs #1136
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?
Conversation
e1cced2 to
d47960c
Compare
geoff-vball
left a 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.
I still have one file left to go, but leaving this feedback now
docs/external-interop/origin_avalanche/validator_set_registry.md
Outdated
Show resolved
Hide resolved
docs/external-interop/origin_avalanche/validator_set_registry.md
Outdated
Show resolved
Hide resolved
docs/external-interop/origin_avalanche/validator_set_registry.md
Outdated
Show resolved
Hide resolved
docs/external-interop/origin_avalanche/validator_set_registry.md
Outdated
Show resolved
Hide resolved
docs/external-interop/origin_avalanche/validator_set_registry.md
Outdated
Show resolved
Hide resolved
ac0f4d2 to
3a10beb
Compare
| `AvalancheValidatorSetRegistry` contracts, new validator sets will need to be relayed periodically. | ||
|
|
||
| ## Updating Validator sets | ||
|
|
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 think this paragraph is technically dense and can be expanded a bit for clarity to readers. It seems like here there are 2 main pieces which should be explicitly broken up either in list or paragraph format:
- The
ValidatorSetUpdatercomponent which is responsible for data synchronization, i.e., continuously monitoring the P-chain and updatingAvalancheValidatorSetRegistrycontracts. I think it would help to explain the trust assumptions for the updater. You can say that it must provide a valid proof, e.g. in the form of an aggregate BLS signature, for updating the validator set. - The Relayer. I don't think this paragraph should start with "Before signing messages,..." rather it should explicitly say what the relayer does. For example:
"The Relayer is responsible for delivering an ICM message from an Avalanche L1 to an external EVM chain (specified in the message). The Relayer first queries the destination registry contract on the external chain to determine the P-chain height that was last registered for the source Avalanche L1. This ensures the relayer signs the message using the correct, currently active set of validators for that L1. Once this occurs, the Relayer collects the signatures from the relevant validators..."
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, tbh I just repurposed Linguan's design doc and put it in here as a placeholder since this should be part of the design doc. I can try to clean it up, but at some point someone with more context should flesh it out.
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 I'm also happy to flesh it out myself. Should I make a PR into this branch or just push commits directly to this PR?
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 like to see this merged and then this can be addressed with PRs to main
| External EVM chains need to verify ICM messages from Avalanche L1s by checking validator signatures. | ||
|
|
||
|
|
||
| The `ValidatorSetUpdater` component polls P-chain at configurable intervals. On each iteration it performs the following: |
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 think for symmetry this step-by-step should also be present for the Relayer
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.
Hopefully some enterprising individual will flesh that out.
| Communication between Avalanche L1 (the so-called _"internal interoperability"_) uses the [ICM protocol](../../../icm-contracts/contracts/teleporter/README.md) | ||
| which is handled by a set of `Teleporter` contracts. Further protocols can be built on top of `Teleporter`, but this is | ||
| out of scope for this design doc. The goal external interoperability is to support `Teleporter` contracts on external EVM | ||
| chains to leverage existing protocols. This will require changes to the `TeleporterMessenger` contracts which will be |
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 was under the impression that these would be a new class of TeleporterMessenger contracts?
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.
Well, it could be. I had initially tried to go with slight modifications of existing contracts that would have full backwards compatibility (or as close to full as possible). I have come to realize in the past 36 hours that there isn't alignment on this at all. So we need to make some sort of decision on this and I could update the docs accordingly.
Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Jacob Turner <[email protected]>
Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Jacob Turner <[email protected]>
b627712 to
2f01e02
Compare
Co-authored-by: mdelle1 <[email protected]> Signed-off-by: Jacob Turner <[email protected]>
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: Jacob Turner <[email protected]>
| @@ -0,0 +1,28 @@ | |||
| # Authenticating ICM Messages | |||
|
|
|||
| Authenticating ICM messages will be handled by contracts implementing the `IMessageVerifier` interface, as described in the [Teleporter architecture](teleporter_contracts.md). The `Teleporter` protocol is complelely agnostic to the details of how messages are authenticated. It is up to applications to decide how to authenticate messages. | |||
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 think this paragraph should be changed to reflect the two different proposals. In the first proposal, in a sense it's not up to applications how to authenticate messages (though it is, based on which TeleporterMessenger instance they use). In either case the TeleporterMessenger contract is agnostic to the details of how messages are authenticated, but in one it is tied to a specific authentication scheme, and in the other it is not.
|
|
||
| When a TeleporterV2 contract receives a message, it checks if it originated from the same contract address as its own. If so, it knows that it is part of the same scheme and can verify the message using the hard-coded `IMessageVerifier` implementation. | ||
|
|
||
| Note that an `IMesssageVerifier` implementation for a particular scheme may need to perform different logic depending on which chain it lives. So a switch statement should be included in its logic to dispatch to the right method depending on which chain it is being called. While many of the code paths will be dead on each chain, it means that the same code is deployed on-chain for a particular scheme. The same is true of `IMessageSender` implementations. |
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 you need to change anything here - just wanted to sketch this out. The switch statement could honestly be in the constructor of a function, leaving the execution path be the same code, but calling different contracts ex: (pseudocode)
IMessageSender[] public immutable messageSenders
constructor() {
if isEth {
messageSenders = <hard-coded list>
} else {
messageSenders = <other hard-coded list>
}
}
sendMessage(TeleporterMessage calldata message) {
for messageSender in messageSenders {
messageSender.sendMessage(message);
}
}
| @@ -0,0 +1,134 @@ | |||
| Teleporter is a protocol of interoperability messaging primitives that provides at most one delivery of authenticated ICM messages. For interoperability between Avalanche L1s and a description of the actual protocol, see [here](../../icm-contracts/contracts/teleporter/README.md). We wish to extend this protocol to external EVM chains, but we cannot use the current contract (`ITeleporterMessenger`). Here we describe the new contracts we will need and the overall architecture. | |||
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.
This file seems like duplication of docs/external-interop/icm_message_authentication.md? We should try to maintain a single source of truth, and just linking other docs where appropriate.
Why this should be merged
A rough draft of the design docs for external interop. Currently only covers the avalanche-> external direction.
How this works
How this was tested
How is this documented