Implement ICS-20 Callbacks #5984

AdityaSripal · 2020-04-10T21:45:07Z

Closes: #5923 and #5947

Description

Implement acknowledgements in ICS20 as described in updated spec
Disallow user-initiated channel closing in ICS20
Remove unnecessary indirection
Add event attributes for acknowledging packet

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2020-04-10T21:47:58Z

Codecov Report

Merging #5984 into master will decrease coverage by 0.06%.
The diff coverage is 17.28%.

@@            Coverage Diff             @@
##           master    #5984      +/-   ##
==========================================
- Coverage   58.19%   58.12%   -0.07%     
==========================================
  Files         396      396              
  Lines       23669    23709      +40     
==========================================
+ Hits        13773    13780       +7     
- Misses       8933     8966      +33     
  Partials      963      963

x/ibc/04-channel/keeper/timeout.go

AdityaSripal · 2020-04-10T23:53:17Z

x/ibc/20-transfer/module.go

+		Error:   "",
+	}
+	if err := am.keeper.OnRecvPacket(ctx, packet, data); err != nil {
+		acknowledgement = FungibleTokenPacketAcknowledgement{


Currently any error returned by keeper.OnRecvPacket will cause a failed acknowledgement to be committed rather than reverting the transaction. Is this the behavior we want?

Yes, that is the behaviour we want. Reverting the transaction would mean that the sequence number (e.g. in an ordered channel) would never proceed, which we don't want - IBC packets always have to be executed.

AdityaSripal · 2020-04-10T23:54:44Z

x/ibc/20-transfer/types/expected_keepers.go

@@ -22,7 +22,6 @@ type ChannelKeeper interface {
 	SendPacket(ctx sdk.Context, channelCap *capability.Capability, packet channelexported.PacketI) error
 	PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement []byte) error
 	ChanCloseInit(ctx sdk.Context, portID, channelID string, chanCap *capability.Capability) error
-	TimeoutExecuted(ctx sdk.Context, packet channelexported.PacketI) error


OnTimeoutPacket no longer calls TimeoutExecuted. TimeoutExecuted is called by the ibc handler after running callback

AdityaSripal · 2020-04-10T23:55:28Z

x/ibc/handler.go

+
+				return nil, err
+			}
+			err = k.ChannelKeeper.TimeoutExecuted(ctx, cap, msg.Packet)


NOTE: ibc handler now calls this function rather than callback

fedekunze

👍

fedekunze · 2020-04-11T23:10:58Z

x/ibc/20-transfer/types/packet.go

 // See spec for onAcknowledgePacket: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay
-type AckDataTransfer struct{}
+type FungibleTokenPacketAcknowledgement struct {
+	Success bool   `json:"success" yaml:"success"`


nit, can we make these fields private and have getter functions for them? Just to prevent them from being reset.

fedekunze · 2020-04-11T23:11:18Z

x/ibc/20-transfer/types/packet.go

-type AckDataTransfer struct{}
+type FungibleTokenPacketAcknowledgement struct {
+	Success bool   `json:"success" yaml:"success"`
+	Error   string `json:"error" yaml:"error"`


q: why not an error type instead? then you can remove the success field and do:

if ack.Error() != nil { return ack.Error() } // success

Just went off the spec. Agreed this has the same behavior.

@cwgoes any danger in switching to the above?

No, no danger, it just needs to be serialised correctly.

fedekunze · 2020-04-11T23:18:11Z

x/ibc/20-transfer/module.go

+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			sdk.EventTypeMessage,


This event type is only for msg handlers afaik. This should instead be something like EventTimeout

x/ibc/20-transfer/module.go

fedekunze · 2020-04-11T23:22:09Z

x/ibc/20-transfer/module.go

+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory),
+			sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)),


use ack.Error() == nil

fedekunze · 2020-04-11T23:22:48Z

x/ibc/20-transfer/module.go

+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory),
+			sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)),
+			sdk.NewAttribute(AttributeKeyAckError, ack.Error),


return error message if ack.Error() != nil

ctx.EventManager().EmitEvent( sdk.NewEvent( EventTypePacket, sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory), sdk.NewAttribute(AttributeKeyReceiver, data.Receiver.String()), sdk.NewAttribute(AttributeKeyValue, data.Amount.String()), sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)), ), ) if ack.Error != "" { ctx.EventManager().EmitEvent( sdk.NewEvent( EventTypePacket, sdk.NewAttribute(AttributeKeyAckError, ack.Error), ), ) }

x/ibc/04-channel/keeper/timeout.go

fedekunze · 2020-04-11T23:25:51Z

x/ibc/20-transfer/module.go

+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			sdk.EventTypeMessage,


ditto, this should be EventTypeChannelClosed

x/ibc/20-transfer/module.go

fedekunze · 2020-04-11T23:27:29Z

x/ibc/20-transfer/module.go

+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			sdk.EventTypeMessage,


ditto event could be packet acknowledged and attributes should be packet type, and transfer details

cwgoes

Nice start; see comments.

x/ibc/04-channel/keeper/timeout.go

cwgoes · 2020-04-12T07:15:30Z

x/ibc/20-transfer/module.go

+		Error:   "",
+	}
+	if err := am.keeper.OnRecvPacket(ctx, packet, data); err != nil {
+		acknowledgement = FungibleTokenPacketAcknowledgement{


Yes, that is the behaviour we want. Reverting the transaction would mean that the sequence number (e.g. in an ordered channel) would never proceed, which we don't want - IBC packets always have to be executed.

cwgoes · 2020-04-12T07:16:21Z

x/ibc/20-transfer/module.go

+		// TODO: handle packet receipt that due to an error (specify)
+		// the receiving chain couldn't process the transfer
+
+		// source chain sent invalid packet, shutdown our channel end


We definitely don't want to do this, not sure when/where this was from, but invalid packets should not close the channel

… into aditya/ics20-callbacks

fedekunze

LGTM. Pending comments from @cwgoes and changes to FungibleTokenPacketAcknowledgement fields

fedekunze · 2020-04-14T13:34:27Z

x/ibc/20-transfer/module.go

+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory),
+			sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)),
+			sdk.NewAttribute(AttributeKeyAckError, ack.Error),


ctx.EventManager().EmitEvent( sdk.NewEvent( EventTypePacket, sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory), sdk.NewAttribute(AttributeKeyReceiver, data.Receiver.String()), sdk.NewAttribute(AttributeKeyValue, data.Amount.String()), sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)), ), ) if ack.Error != "" { ctx.EventManager().EmitEvent( sdk.NewEvent( EventTypePacket, sdk.NewAttribute(AttributeKeyAckError, ack.Error), ), ) }

cwgoes

utACK

cwgoes · 2020-04-14T14:31:35Z

x/ibc/20-transfer/types/packet.go

-type AckDataTransfer struct{}
+type FungibleTokenPacketAcknowledgement struct {
+	Success bool   `json:"success" yaml:"success"`
+	Error   string `json:"error" yaml:"error"`


No, no danger, it just needs to be serialised correctly.

AdityaSripal · 2020-04-14T19:37:41Z

Believe this should be merged as-is. Tried to change the Acknowledgement struct to have just an error, but ran into a serialization panic since the error interface isn't registered.

Seems like there isn't a native JSON marshalling of error interface. I'm sure there's a way around this, but figure it's probably not worth the complication of implementing some custom way to marshal an error

jackzampolin

This looks ready to go. When should I start integrating this into the relayer?

AdityaSripal added 2 commits April 11, 2020 02:44

fix acknowledgements, timeouts, and chancloseinit

f418531

fix tests

bc37e95

AdityaSripal requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners April 10, 2020 21:45

add acknowledge test

eed72f6

AdityaSripal commented Apr 10, 2020

View reviewed changes

AdityaSripal added WIP labels Apr 10, 2020

AdityaSripal requested review from cwgoes and jackzampolin April 10, 2020 23:56

add 20 ack events

02d2925

AdityaSripal added R4R and removed WIP labels Apr 11, 2020

fedekunze reviewed Apr 11, 2020

View reviewed changes

cwgoes suggested changes Apr 12, 2020

View reviewed changes

AdityaSripal added WIP and removed R4R labels Apr 14, 2020

AdityaSripal added 5 commits April 14, 2020 08:58

add cap authentication to PacketExecuted

8d9083a

fix events

009399a

Merge branch 'master' into aditya/ics20-callbacks

f3a2221

return error

b1d3e9b

Merge branch 'aditya/ics20-callbacks' of github.com:cosmos/cosmos-sdk…

84a2331

… into aditya/ics20-callbacks

AdityaSripal requested a review from cwgoes April 14, 2020 04:08

fedekunze approved these changes Apr 14, 2020

View reviewed changes

cwgoes approved these changes Apr 14, 2020

View reviewed changes

Merge branch 'master' into aditya/ics20-callbacks

e6105b6

emit error event attribute only on failure

39e25bf

AdityaSripal requested a review from fedekunze April 14, 2020 19:37

AdityaSripal added R4R and removed WIP labels Apr 14, 2020

Merge branch 'master' into aditya/ics20-callbacks

d49904b

jackzampolin approved these changes Apr 15, 2020

View reviewed changes

Merge branch 'master' into aditya/ics20-callbacks

5f6c2b6

fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 15, 2020

mergify bot merged commit ca26587 into master Apr 15, 2020

mergify bot deleted the aditya/ics20-callbacks branch April 15, 2020 23:37

AdityaSripal mentioned this pull request Apr 16, 2020

Prevent channels from being arbitrarily closed in ICS 20 #5947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ICS-20 Callbacks #5984

Implement ICS-20 Callbacks #5984

AdityaSripal commented Apr 10, 2020 •

edited

Loading

codecov bot commented Apr 10, 2020 •

edited

Loading

AdityaSripal Apr 10, 2020

cwgoes Apr 12, 2020

AdityaSripal Apr 10, 2020

AdityaSripal Apr 10, 2020

fedekunze left a comment

fedekunze Apr 11, 2020

fedekunze Apr 11, 2020

AdityaSripal Apr 14, 2020

cwgoes Apr 14, 2020

fedekunze Apr 11, 2020

fedekunze Apr 11, 2020

fedekunze Apr 11, 2020

fedekunze Apr 14, 2020

fedekunze Apr 11, 2020

fedekunze Apr 11, 2020

cwgoes left a comment

cwgoes Apr 12, 2020

cwgoes Apr 12, 2020

fedekunze left a comment

fedekunze Apr 14, 2020

cwgoes left a comment

cwgoes Apr 14, 2020

AdityaSripal commented Apr 14, 2020

jackzampolin left a comment

Implement ICS-20 Callbacks #5984

Implement ICS-20 Callbacks #5984

Conversation

AdityaSripal commented Apr 10, 2020 • edited Loading

Description

codecov bot commented Apr 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Apr 14, 2020

jackzampolin left a comment

Choose a reason for hiding this comment

AdityaSripal commented Apr 10, 2020 •

edited

Loading

codecov bot commented Apr 10, 2020 •

edited

Loading