04-Packet-Flow-v2 #1148
04-Packet-Flow-v2 #1148
Conversation
There was a problem hiding this comment.
Great start!! I really like the explanations you gave for the Counterparty
A couple major tweaks as you continue working on this:
- We need to decide on a new name for Counterparty and make Channel a clear use again in the specification
- We need to not tie client id to channel id (aliasing approach)
- The ICS24 paths and commitments need to follow their new v2 specification as defined in ICS24 README and PACKET.md
| - The `nextSequenceSend`, stored separately, tracks the sequence number for the next packet to be sent. | ||
|
|
||
| ```typescript | ||
| function nextSequenceSendPath(sourceID: bytes, destID: bytes): Path { |
There was a problem hiding this comment.
This shouldn't be keyed on the string clients.
And there's no need for there to be both source and destId. Since we only need our sides' identifier to uniquely key on the channel.
It can just be
nextSequenceSend/{sourceId}
| @@ -190,10 +167,23 @@ The architecture of clients, connections, channels and packets: | |||
|
|
|||
| #### Store paths | |||
|
|
|||
| NOTE do we stil need this, or we can retrieve the sequence from the commitment path associated with the clientID? | |||
There was a problem hiding this comment.
We still need this. We want to keep a track of the nonce
| Channel structures are stored under a store path prefix unique to a combination of a port identifier and channel identifier: | ||
|
|
||
| ```typescript | ||
| function channelPath(portIdentifier: Identifier, channelIdentifier: Identifier): Path { | ||
| function channelPath(portIdentifier: bytes, channelIdentifier: bytes): Path { |
|
|
||
| Constant-size commitments to packet data fields are stored under the packet sequence number: | ||
|
|
||
| ```typescript | ||
| function packetCommitmentPath(portIdentifier: Identifier, channelIdentifier: Identifier, sequence: uint64): Path { | ||
| return "commitments/ports/{portIdentifier}/channels/{channelIdentifier}/sequences/{sequence}" | ||
| function packetCommitmentPath(sourceId: bytes, sequence: uint64): Path { |
There was a problem hiding this comment.
the sequence needs to be in bigEndian format. See ICS24 for details
| } | ||
| ``` | ||
|
|
||
| Packet acknowledgement data are stored under the `packetAcknowledgementPath`: | ||
|
|
||
| ```typescript | ||
| function packetAcknowledgementPath(portIdentifier: Identifier, channelIdentifier: Identifier, sequence: uint64): Path { | ||
| return "acks/ports/{portIdentifier}/channels/{channelIdentifier}/sequences/{sequence}" | ||
| function packetAcknowledgementPath(sourceId: bytes, sequence: uint64): Path { |
| sourceClientId: bytes, | ||
| destClientId: bytes, |
| provableStore.set( | ||
| packetCommitmentPath(sourcePort, sourceChannel, sequence), | ||
| hash(hash(data), timeoutHeight, timeoutTimestamp) | ||
| // Note do we need to keep the channelStore? Should this be instead the counterParty store or something similar? Do we keep it for backward compatibility? |
There was a problem hiding this comment.
It should just be the provableStore
| // Note do we need to keep the channelStore? Should this be instead the counterParty store or something similar? Do we keep it for backward compatibility? | ||
| channelStore.set( | ||
| packetCommitmentPath(sourceClientId sequence), | ||
| hash(hash(data), timeoutTimestamp) |
There was a problem hiding this comment.
This is wrong, we need to use the commitment function defined in PACKET.md
| break; | ||
| } | ||
| merklePath, | ||
| hash(packet.data, packet.timeoutTimestamp) |
There was a problem hiding this comment.
need to use Commitment function defined in PACKET.md
|
|
||
| channelStore.set( | ||
| packetReceiptPath(packet.sourceChannelId, packet.sequence), | ||
| SUCCESSFUL_RECEIPT |
sangier
requested review from
angbrav,
colin-axner and
damiannolan
as code owners
There was a problem hiding this comment.
Nice start @sangier !
We should also check the counterparty channel id is as expected in the packet handlers
| An `OpaquePacket` is a packet, but cloaked in an obscuring data type by the host state machine, such that a module cannot act upon it other than to pass it to the IBC handler. The IBC handler can cast a `Packet` to an `OpaquePacket` and vice versa. | ||
|
|
||
| ```typescript | ||
| type OpaquePacket = object | ||
| ``` |
There was a problem hiding this comment.
Don't think this is necessary
There was a problem hiding this comment.
Let's remove all references to OpaquePacket and just use Packet
|
|
||
| ```typescript | ||
| interface Acknowledgement { | ||
| appAcknowledgement: [bytes] // array of bytes. Each element of the array contains an acknowledgement from a specific application |
There was a problem hiding this comment.
This is not an array of bytes, but an array of an array of bytes
byte[][] not byte[]
|
|
||
| Given a scenario where we are sending a packet from a sender chain `A` to a receiver chain `B` the protocol follows the following rules: | ||
|
|
||
| - Sender `A` can call either {`sendPacket`,`acknowledgePacket`,`timeoutPacket`} |
There was a problem hiding this comment.
Hmm don't think this is so clear since it sounds like you can call either in any order at any time
There was a problem hiding this comment.
At the beginning, Sender A can only call sendPacket
| function sendPacket( | ||
| sourceChannelId: bytes, | ||
| timeoutTimestamp: uint64, | ||
| payloads: []byte |
There was a problem hiding this comment.
This should be a list of Payload concrete type
| // Executes Application logic ∀ Payload | ||
| payload=packet.data[0] | ||
| cbs = router.callbacks[payload.destPort] | ||
| ack,success = cbs.onReceivePacket(packet.channelDestId,payload,relayer,packet.sequence) // Note that payload includes the version. The application is required to inspect the version to route the data to the proper callback |
This PR introduces the draft specification for the IBC version 2 ICS-04: Channel and Packet Semantics.
It formalizes the semantics for channel creation, counterparty registration, and packet flow management in IBC version 2 taking into account all the v2 spec changes.
Closes #1140