Add standard acknowledgement to channels and apply usage in ibc transfer #7263

colin-axner · 2020-09-08T13:50:23Z

Description

Adds standard acknowledgement definition as defined by ICS 04 and updates ibc-transfer to use this acknowledgement

closes: #6963

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.

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
Review Codecov Report in the comment section below once CI passes

…-standard-ack

codecov · 2020-09-08T13:55:09Z

Codecov Report

Merging #7263 into master will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #7263      +/-   ##
==========================================
+ Coverage   55.13%   55.17%   +0.03%     
==========================================
  Files         584      584              
  Lines       40506    40535      +29     
==========================================
+ Hits        22335    22366      +31     
+ Misses      16301    16300       -1     
+ Partials     1870     1869       -1

colin-axner · 2020-09-08T13:54:02Z

x/ibc-transfer/module.go

-		Success: true,
-		Error:   "",
-	}
+	acknowledgement := channeltypes.NewResultAcknowledgement([]byte{byte(1)})


I wasn't sure what to put here since it is not specified in ICS20 at the moment.

I don't know either, but this seems like a fair non-empty success flag.
(Also 1 is a nice replacement for true and auto-convert in many languages)

colin-axner · 2020-09-08T13:56:40Z

x/ibc-transfer/module.go

@@ -356,19 +352,10 @@ func (am AppModule) OnAcknowledgementPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, fmt.Sprintf("%d", data.Amount)),
-			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)),
+			sdk.NewAttribute(types.AttributeKeyAck, fmt.Sprintf("%v", ack)),


thoughts? or should I add a switch to check the type and emit a bool?

yeah we should

I will have it emit the result or error message as well

instead of the bool

updated, let me know if you agree, with specs as well

I like the usage above:

sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", err != nil))

We could use %v instead (or as well), but it would be nice to have them both the same (the attributes in the two different functions)

ah yea, I agree. Will update in a followup. We discussed standardizing IBC events on the IBC call which I think would be very useful in cases like this where there are multiple way to represent the same information

colin-axner · 2020-09-08T13:58:30Z

x/ibc-transfer/module.go

@@ -336,7 +332,7 @@ func (am AppModule) OnAcknowledgementPacket(
 	packet channeltypes.Packet,
 	acknowledgement []byte,
 ) (*sdk.Result, error) {
-	var ack types.FungibleTokenPacketAcknowledgement
+	var ack channeltypes.Acknowledgement
 	if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {


shouldn't we use the cdc in the keeper?

the keeper codec is a BinaryMarshaler, thus it doesn't support JSON unmarshal

…k into colin/6963-standard-ack

ethanfrey

Good stuff.
Thanks @colin-axner

ethanfrey · 2020-09-08T20:38:44Z

x/ibc-transfer/module.go

-		Success: true,
-		Error:   "",
-	}
+	acknowledgement := channeltypes.NewResultAcknowledgement([]byte{byte(1)})


I don't know either, but this seems like a fair non-empty success flag.
(Also 1 is a nice replacement for true and auto-convert in many languages)

ethanfrey · 2020-09-08T20:45:54Z

x/ibc-transfer/module.go

@@ -356,19 +352,10 @@ func (am AppModule) OnAcknowledgementPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, fmt.Sprintf("%d", data.Amount)),
-			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)),
+			sdk.NewAttribute(types.AttributeKeyAck, fmt.Sprintf("%v", ack)),


I like the usage above:

sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", err != nil))

We could use %v instead (or as well), but it would be nice to have them both the same (the attributes in the two different functions)

ethanfrey · 2020-09-08T20:46:32Z

x/ibc/04-channel/types/channel.go

+}
+
+// ValidateBasic performs a basic validation of the acknowledgement
+func (ack Acknowledgement) ValidateBasic() error {


fedekunze

Looks good! 🙂

fedekunze · 2020-09-08T21:54:53Z

x/ibc-transfer/keeper/relay.go

-	if !ack.Success {
+func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketData, ack channeltypes.Acknowledgement) error {
+	switch ack.Response.(type) {
+	case *channeltypes.Acknowledgement_Error:


can we test this somehow?

yes I think it should be tested by the commented tests. I wanted to uncomment the tests in a followup pr to keep this pr lightweight. There is an issue for this #7261

fedekunze · 2020-09-08T21:55:19Z

x/ibc-transfer/keeper/relay.go

 		return k.refundPacketToken(ctx, packet, data)
+	default:
+		return nil


nit, can you add a note on why we return nil instead of an error?

fedekunze · 2020-09-08T21:57:34Z

x/ibc-transfer/module.go

@@ -336,7 +332,7 @@ func (am AppModule) OnAcknowledgementPacket(
 	packet channeltypes.Packet,
 	acknowledgement []byte,
 ) (*sdk.Result, error) {
-	var ack types.FungibleTokenPacketAcknowledgement
+	var ack channeltypes.Acknowledgement
 	if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {


the keeper codec is a BinaryMarshaler, thus it doesn't support JSON unmarshal

x/ibc-transfer/spec/01_concepts.md

AdityaSripal

LGTM!

…k into colin/6963-standard-ack

cwgoes

ACK

proto/ibc/channel/channel.proto

colin-axner added 6 commits September 8, 2020 11:40

gen ack proto type

2b443d8

remove transfer ack

6fa65b2

change ics20 ack to use standard type

5302fcc

update commented tests

23950b9

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/6963…

4e77eba

…-standard-ack

small typo fix

50f55db

colin-axner added the x/ibc label Sep 8, 2020

colin-axner added 2 commits September 8, 2020 15:56

revert back to module cdc

b5fa71f

update docs

f159861

colin-axner commented Sep 8, 2020

View reviewed changes

colin-axner and others added 4 commits September 8, 2020 16:16

fix lint

500af59

Merge branch 'master' into colin/6963-standard-ack

7223cc4

nit

07a08fa

Merge branch 'colin/6963-standard-ack' of github.com:cosmos/cosmos-sd…

4782685

…k into colin/6963-standard-ack

colin-axner marked this pull request as ready for review September 8, 2020 14:19

colin-axner requested review from aaronc, alessio and alexanderbez as code owners September 8, 2020 14:19

colin-axner requested review from cwgoes, fedekunze, AdityaSripal and ethanfrey September 8, 2020 14:19

update ack event emission

5eeefa3

ethanfrey approved these changes Sep 8, 2020

View reviewed changes

fedekunze approved these changes Sep 8, 2020

View reviewed changes

Update x/ibc-transfer/spec/01_concepts.md

73a4404

AdityaSripal approved these changes Sep 8, 2020

View reviewed changes

colin-axner added 2 commits September 9, 2020 10:45

add comment for onacknowledgement

25bc225

Merge branch 'colin/6963-standard-ack' of github.com:cosmos/cosmos-sd…

9dd92de

…k into colin/6963-standard-ack

colin-axner and others added 2 commits September 9, 2020 10:57

merge master

d37f45b

Merge branch 'master' into colin/6963-standard-ack

76a56f9

cwgoes approved these changes Sep 9, 2020

View reviewed changes

proto/ibc/channel/channel.proto Outdated Show resolved Hide resolved

Update proto/ibc/channel/channel.proto

a0bd8a4

colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 9, 2020

mergify bot merged commit b4f146b into master Sep 9, 2020

mergify bot deleted the colin/6963-standard-ack branch September 9, 2020 10:01

colin-axner mentioned this pull request Dec 1, 2020

x/ibc: implement ADR 032 typed events #7762

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add standard acknowledgement to channels and apply usage in ibc transfer #7263

Add standard acknowledgement to channels and apply usage in ibc transfer #7263

colin-axner commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading

colin-axner Sep 8, 2020

ethanfrey Sep 8, 2020

colin-axner Sep 8, 2020

fedekunze Sep 8, 2020

colin-axner Sep 8, 2020

colin-axner Sep 8, 2020

colin-axner Sep 8, 2020

ethanfrey Sep 8, 2020

colin-axner Sep 9, 2020

colin-axner Sep 8, 2020

fedekunze Sep 8, 2020

ethanfrey left a comment

ethanfrey Sep 8, 2020

ethanfrey Sep 8, 2020

ethanfrey Sep 8, 2020

fedekunze left a comment

fedekunze Sep 8, 2020

colin-axner Sep 9, 2020

fedekunze Sep 8, 2020

fedekunze Sep 8, 2020

AdityaSripal left a comment

cwgoes left a comment

Add standard acknowledgement to channels and apply usage in ibc transfer #7263

Add standard acknowledgement to channels and apply usage in ibc transfer #7263

Conversation

colin-axner commented Sep 8, 2020 • edited Loading

Description

codecov bot commented Sep 8, 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

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

ethanfrey 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

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

AdityaSripal left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

colin-axner commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading