Better OnAcknowledgementPacket handling

[joe-bowman]

Summary

The docs at https://github.com/cosmos/ibc-go/blob/main/docs/apps/interchain-accounts/auth-modules.md suggest that decoding the Acknowledgement is as simple as:

Begin by unmarshaling the acknowledgement into sdk.TxMsgData

(N.B. channeltypes here refers to github.com/cosmos/ibc-go/v3/modules/core/04-channel/types)

However, the acknowledgement byte slice that is referred to is not an sdk.TxMsgData but the JSON-encoded Response field from channeltypes.Acknowledgement - either a channeltypes.Acknowledgement_Result or channeltypes.Acknowledgement_Error`.

This needs to be unmarshalled from JSON (cannot be done using the Keeper's Codec; so unintuitively we have to use json.Unmarshal - unless there is a better way?) before we can proto.Unmarshal the actual bytestring the docs allude to.

Problem Definition

Docs / spec do not reflect reality.

Proposal

Not sure - but docs/spec should reflect reality.

The manual unmarshalling of the Acknowledgement struct feels clumsy - maybe there ought to be a NewAcknowledgementFromResponse() helper in channeltypes (or have this handled internally and have OnAcknowledgementPacket acknowledgement argument of type channeltypes.Acknowledgement so we just have to called GetResult() or GetError() as appropriate).

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[joe-bowman]

For reference, this is currently required to handle Acknowledgements - this differs from my interpretation of the docs:

// OnAcknowledgementPacket implements the IBCModule interface
func (im IBCModule) OnAcknowledgementPacket(
	ctx sdk.Context,
	packet channeltypes.Packet,
	acknowledgement []byte,
	relayer sdk.AccAddress,
) error {
	ack := channeltypes.Acknowledgement_Result{}
	err := json.Unmarshal(acknowledgement, &ack)
	if err != nil {
		im.keeper.Logger(ctx).Error("Unable to unmarshal acknowledgement result", "error", err, "data", acknowledgement)
		return err
	}
	txMsgData := &sdk.TxMsgData{}
	err = proto.Unmarshal(ack.Result, txMsgData)
	if err != nil {
		im.keeper.Logger(ctx).Error("Unable to unmarshal acknowledgement", "error", err, "ack", ack.Result)
		return err
	}
	for _, msgData := range txMsgData.Data {
		switch msgData.MsgType {
		case ...
                }
        }
}

[colin-axner]

Nice catch and thanks for opening the issue. The docs are incomplete/incorrect.

This:

if err := proto.Unmarshal(ack.Acknowledgement(), txMsgData); err != nil {

should be

if err := proto.Unmarshal(ack.Result, txMsgData); err != nil {

like you reference.

cannot be done using the Keeper's Codec

IBC modules typically have access to a keeper (see that the ica demo repo does). It is a little clumsy, but you can move this code into a keeper.OnAcknowledgementPacket() to get access to the codec. This explanation should also be added to the docs (although I guess using json.Unmarshal should be fine?)

[crodriguezvega]

@joe-bowman we have also tried this ourselves now (for SDK 0.45.x) in the interchain-account-demo repo, in case you want to have a look.

[joe-bowman]

Perfect, thanks for your help gents :)

…

On Wed, 16 Mar 2022 at 11:44, Carlos Rodriguez ***@***.***> wrote: @joe-bowman <https://github.com/joe-bowman> we have also tried this ourselves now (for SDK 0.45.x) in the interchain-account-demo repo <https://github.com/cosmos/interchain-accounts-demo/pull/88/files>, in case you want to have a look. — Reply to this email directly, view it on GitHub <#1114 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA7QYDHVK5MVZWUYXPHP6DVAHCSLANCNFSM5QS3JFJQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>