Add Warp Payload Types #2116
Add Warp Payload Types #2116
Conversation
nytzuga
force-pushed
the
move-warp-payload-to-avalanchego
branch
from
d621a43
to
c67537f
Compare
|
Could we move this into |
nytzuga
force-pushed
the
move-warp-payload-to-avalanchego
branch
from
b1ab332
to
31125c9
Compare
Move warp/payload types to this repo so it can be easily reused from other subnets.
nytzuga
force-pushed
the
move-warp-payload-to-avalanchego
branch
from
31125c9
to
f13427e
Compare
| ## BlockHashPayload | ||
|
|
||
| BlockHashPayload: |
There was a problem hiding this comment.
Is there a reason this is BlockHashPayload rather than a Hash payload? Seems unnecessarily specific.
There was a problem hiding this comment.
This is meant to be specific to the BlockHash as opposed to an arbitrary hash. If we changed to an arbitrary hash, the caller would need to have a different way to differentiate between hash types 🤷 .
I don't think there's a need for an arbitrary hash payload at this time, so lmk if you'd prefer I change it anyways.
There was a problem hiding this comment.
Going to leave this as BlockHash after removing the Payload suffix.
|
|
||
| // AddressedPayload defines the format for delivering a point to point message across VMs | ||
| // ie. (ChainA, AddressA) -> (ChainB, AddressB) | ||
| type AddressedPayload struct { |
There was a problem hiding this comment.
These types seem to stutter payload.AddressedPayload, payload.BlockHashPayload. I think we could improve the naming here.
There was a problem hiding this comment.
Could go with AddressedCaller and BlockHash. I'll think a bit more about it and push a change here today.
There was a problem hiding this comment.
Going with AddressedCall and BlockHash
vms/platformvm/warp/payload/codec.go
Outdated
There was a problem hiding this comment.
Should we have some way to parse either payload? It seems a bit odd to me that we expect the user to know which payload format to expect
There was a problem hiding this comment.
This is currently used in Subnet-EVM to serve two execution functions in the Warp precompile. In that case, the function returns an error if the warp message includes anything other than the expected payload and does not differentiate between an invalid signature and requesting the wrong type, so it was not needed.
Will add as a convenience function and so we can add a check when verifying warp messages that it matches a valid payload type rather than just checking the signature's validity.
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Cesar <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Cesar <[email protected]>
| // AddressedPayload defines the format for delivering a point to point message across VMs | ||
| // ie. (ChainA, AddressA) -> (ChainB, AddressB) | ||
| type AddressedPayload struct { | ||
| SourceAddress []byte `serialize:"true"` |
There was a problem hiding this comment.
I guess we want to make this generic for all possible chains? (EVM chains use 20 bytes addresses)
There was a problem hiding this comment.
Yup, the intent here is to generalize this so that other VMs can use the same format. This will require changing the unpacking in Subnet-EVM / Teleporter as well.
| - `sourceAddress` is the address that sent this message from the source chain | ||
| - `payload` is an arbitrary byte array payload | ||
|
|
||
| ## BlockHash |
There was a problem hiding this comment.
Should we call this ProofHash or ProofRoot?
There was a problem hiding this comment.
As we discussed, this may not be strictly a block hash
There was a problem hiding this comment.
This is currently used as a BlockHash and making it more generic means that the caller would need to know what kind of hash it's referring to (some context is already needed ofc), so I opted to leave this as BlockHash for now.
If we want to change the name in the future, it is backwards compatible to do so, so I'd prefer to leave this until we actually use it as something other than a BlockHash.
There was a problem hiding this comment.
As you alluded to, blocks aren't "universal" so the same problem applies to them.
I think locking this terminology in when we communicate it broadly will make us appear a little short-sighted, whereas using the Proof/ProofRoot moniker is forward-looking.
There was a problem hiding this comment.
Tbh I'd expect using a BlockHash and using that to prove out the rest of the header contents to be simpler.
Do you see this as a more targeted? If a VM would support multiple different types of proofs, how do you foresee them providing context as to what it's proving? Do you foresee them being registered under different typeIDs?
There was a problem hiding this comment.
I guess I was really excited by the generality/simplicity of using an arbitrary proof structure.
For example, it may be way easier to write a solidity contract to verify a trie of warp message IDs rather than a HyperSDK block. That approach also provides a generic interface that any VM can implement and use previously tested/implemented contracts (which I think will be a huge unlock).
We can enshrine "type 0" of the type to be "block hash" if you want to call that out.
There was a problem hiding this comment.
In terms of using "hash" in the name, I'm not sure what it should be called. I'm not sure all "proof roots" are "hashes" (maybe they are)?
There was a problem hiding this comment.
Ok I guess the context will be up to the Solidity contracts to know what they are talking to based on the chainID, so that they know how it should be interpreted.
As for naming, I'm fine with Hash or Proof
StephenButtolph
changed the title
* Integrating Warp Payload Types into AvalancheGo for Cross-VM Compatibility This fixes ava-labs/avalanchego#2050 and ava-labs/avalanchego#2116 * Update types * Do not update go.sum * Fixed more type changes from the migration * Drop the alias `avalancheWarpPayload` The alias is not longer needed --------- Co-authored-by: Aaron Buchwald <[email protected]>
Why this should be merged
#2050 Integrating Warp Payload Types into AvalancheGo for Cross-VM Compatibility
How this works
How this was tested