Unify Recv/Ack fungible_token_packet event

[minxylynx]

Summary

The fungible_token_packet event emitted by OnRecvPacket and by OnAcknowledgementPacket handle the success/failure status differently. It would be nice to have them handle the status in the same way.

Problem Definition

For the fungible_token_packet event emitted during OnRecvPacket processing (https://github.com/cosmos/ibc-go/blob/v2.2.0/modules/apps/transfer/module.go#L344-L353), the success or failure of the packet is distilled into a single, consistent key 'success'. This is nice because the user actually sees a true or false value.

For the fungible_token_packet event emitted during OnAcknowledgementPacket processing (https://github.com/cosmos/ibc-go/blob/v2.2.0/modules/apps/transfer/module.go#L379-L405), the success or failure of the packet is broken into either a success key or an error key. This is confusing because the value is the response object rather than a true boolean.

Trying to figure out the success condition for either of the msg types is confusing due to the difference in how the success/failure state is presented.

Proposal

Change how OnAcknowledgementPacket processes the success state to mirror that of OnRecvPacket, where there is a single consistent field (success), populated by the ack.Success() function.

This would make most sense as the true state is clear to the user without having to decipher any combination of things.

Unless there is a reason why OnAcknowledgementPacket needs both of the success/error keys, it makes sense to transition to a more unified and clearer picture of the success state.

---

For Admin Use

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

[crodriguezvega]

Thanks for opening this issue, @minxylynx!

I see that in case of successful acknowledgement the value for the AttributeKeyAckSuccess attribute will be just the string representation of []byte{byte(1)}, so I guess it makes sense to do here fmt.Sprintf("%t", ack.Success()) as well instead.

But I didn't understand completely if with your proposal you would like to still keep emitting the error message in case of failure. I think it's handy to keep this to know why exactly receiving the packet failed, right?

Looking at the code now I also wonder why we emit 2 separate events: first this one and then another one depending on whether the acknowledgment is successful or not. @colin-axner, would it be possible (and/or desirable) to emit only one event and include in that event both attributes AttributeKeyAckSuccess and AttributeKeyAckError (which would be empty in case of success). Any thoughts?

[colin-axner]

Not emitting the error message on unsuccessful acknowledgements during OnRecvPacket was likely an oversight. We can change the AttributeKeyAckSuccess in OnAcknowledgement to match the event attribute in OnRecv

[minxylynx]

I would absolutely be fine with a combination of the two emissions:

1. In OnAcknowledgement , have AttributeKeyAckSuccess be set to fmt.Sprintf("%t", ack.Success()) (like is being done for OnRecvPacket)
2. In OnRecvPacket , set AttributeKeyAckError in the case of an error message (like is being done for OnAcknowledgement)

I primarily wanted the success/failure boolean to be present for both, as that makes it easier to spot the true transfers.

[crodriguezvega]

Just for clarification... So then both in OnRecvPacket and in OnAcknowledgementPacket we would like to have:

ctx.EventManager().EmitEvent(
	sdk.NewEvent(
		types.EventTypePacket,
		sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
		sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
		sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
		sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
		sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
	),
)

switch resp := ack.Response.(type) {
case *channeltypes.Acknowledgement_Error:
        ctx.EventManager().EmitEvent(
	        sdk.NewEvent(
		        types.EventTypePacket,
		        sdk.NewAttribute(types.AttributeKeyAckError, resp.Error),
	       ),
        )
}

Is this correct?

[minxylynx]

I think that looks correct form my understanding. Thank you again for addressing this, I really appreciate it.

[crodriguezvega]

@minxylynx we have actually implemented by coincidence this change in PR #1565. Check out what the code looks like now: the fungible_token_packet event in onRecvPacket contains the types.AttributeKeyAckError in case the ack is an error acknowledgment.

However, this will be released only in v4.0.0, since #1565 introduces an API breaking change (which is not related to the issue here). Is that ok? Or would you also need this change back ported to previous release lines like v2 and v3? If that's the case, then we need to maybe cherry pick the code changes from #1565.

[minxylynx]

Ill push to have my core team upgrade when they can. If you want to backport, no arguments here, but I can also wait for the 4.0.0 release.

[crodriguezvega]

Thanks for the reply, @minxylynx. Then I think we will release this only when v4.0.0 comes out (which should happen in a couple of weeks max), and we will therefore not backport it to v2 and v3.

[minxylynx]

Awesome. Thanks again for getting this change in. It will help downstream for sure.

[crodriguezvega]

The fix for this issue has now been released in v4.0.0.