Simplify Acknowledgement structure #7555
Conversation
There was a problem hiding this comment.
Use RecvPacketResult purely as a type internal to the ibc-go implementation to package the app acknowledgement with other useful information for the receiver chain.
However, when packaging the acknowledgement to send over the wire; the protocol only needs the list of app acknowledgements. Thus I've simplified the types here.
| isAsync = true | ||
| // Return error if there is more than 1 payload | ||
| // TODO: Handle case where there are multiple payloads | ||
| if len(msg.Packet.Payloads) > 1 { |
There was a problem hiding this comment.
Currently just error for multipayload in async ack case
| @@ -203,14 +206,10 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem | |||
| return nil, errorsmod.Wrap(err, "acknowledge packet verification failed") | |||
| } | |||
|
|
|||
| recvResults := make(map[string]types.RecvPacketResult) | |||
There was a problem hiding this comment.
This puts the list into a map which will remove any subsequent acknowledgements from the same app. This is an error since we want to support sending multiple packets with the same portIDs.
Moreover it is unnecessary, the acknowledgement list is meant to match 1-1 to the list of payloads so we can associate each individual acknowledgement to the correct payload by looking at the portIDs in the matching payload in the payload list
There was a problem hiding this comment.
good catch, and yeah makes sense that we can just use the index.
| ack := msg.Acknowledgement.AppAcknowledgements[i] | ||
| err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, pd, ack, relayer) |
There was a problem hiding this comment.
See here: We just match using the index
| } | ||
|
|
||
| // AcknowledgementResult of identified acknowledgement. Correlating IBC application name to opaque ack bytes. | ||
| message AcknowledgementResult { |
There was a problem hiding this comment.
Unnecessary struct since the acknowledgment side only ever uses the Acknowledgement bytes inside the RecvPacketResult.
The status in RecvPacketResult is only used on the receive side. Thus we do not need to transmit it over the wire back to the sending chain. Instead we can just package each individual opaque app acknowledgement into the Acknowledgment struct and send it over.
Moreover, the commitment isn't using the status so it isn't even secure to use it on the sending side (which we don't)
| @@ -203,14 +206,10 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem | |||
| return nil, errorsmod.Wrap(err, "acknowledge packet verification failed") | |||
| } | |||
|
|
|||
| recvResults := make(map[string]types.RecvPacketResult) | |||
There was a problem hiding this comment.
good catch, and yeah makes sense that we can just use the index.
AdityaSripal
changed the title
|
Description
This is a proposal that follows the current spec and I believe is cleaner than the current approach.
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.