From c264fa0b35362295ffeae7652e56f55293f02aef Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Tue, 14 Jun 2022 10:43:24 +0100 Subject: [PATCH 01/13] feat: include ABCI code in failed acknowledgement. --- .../core/04-channel/types/acknowledgement.go | 22 +++++++++++++++++-- .../04-channel/types/acknowledgement_test.go | 4 ++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/modules/core/04-channel/types/acknowledgement.go b/modules/core/04-channel/types/acknowledgement.go index b46de2b981d..164ec4b5f91 100644 --- a/modules/core/04-channel/types/acknowledgement.go +++ b/modules/core/04-channel/types/acknowledgement.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "reflect" "strings" @@ -8,6 +9,12 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) +const ( + // ackErrorString defines a string constant included in error acknowledgements + // NOTE: Changing this const is state machine breaking as acknowledgements are written into state. + ackErrorString = "error handling packet: see events for details" +) + // NewResultAcknowledgement returns a new instance of Acknowledgement using an Acknowledgement_Result // type in the Response field. func NewResultAcknowledgement(result []byte) Acknowledgement { @@ -22,10 +29,21 @@ func NewResultAcknowledgement(result []byte) Acknowledgement { // type in the Response field. // NOTE: Acknowledgements are written into state and thus, changes made to error strings included in packet acknowledgements // risk an app hash divergence when nodes in a network are running different patch versions of software. -func NewErrorAcknowledgement(err string) Acknowledgement { +func NewErrorAcknowledgement(errMsg string) Acknowledgement { + // the ABCI code is included in the abcitypes.ResponseDeliverTx hash + // constructed in Tendermint and is therefore deterministic + + errMsg = strings.TrimSpace(errMsg) + var err error + if errMsg != "" { + err = fmt.Errorf(errMsg) + } + + _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values + return Acknowledgement{ Response: &Acknowledgement_Error{ - Error: err, + Error: fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString), }, } } diff --git a/modules/core/04-channel/types/acknowledgement_test.go b/modules/core/04-channel/types/acknowledgement_test.go index 658ff31a8b5..a4f62703742 100644 --- a/modules/core/04-channel/types/acknowledgement_test.go +++ b/modules/core/04-channel/types/acknowledgement_test.go @@ -29,10 +29,10 @@ func (suite TypesTestSuite) TestAcknowledgement() { false, }, { - "empty faied ack", + "empty failed ack", types.NewErrorAcknowledgement(" "), false, - false, + true, }, { "nil response", From 1ad5de56381de928bdc18ee9c231507c635e213d Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Wed, 22 Jun 2022 10:30:37 +0100 Subject: [PATCH 02/13] fix: removing transfertypes.NewErrorAcknowledgement and interchainaccounts.NewErrorAcknowledgement in favour of channeltypes.NewErrorAcknowledgement --- CHANGELOG.md | 3 +++ .../controller/ibc_middleware.go | 6 ++++- .../events/emit_events.go | 19 +++++++++++++ .../27-interchain-accounts/host/ibc_module.go | 4 +-- .../host/ibc_module_test.go | 2 +- .../27-interchain-accounts/host/types/ack.go | 27 ------------------- .../host/types/ack_test.go | 19 ------------- modules/apps/transfer/ibc_module.go | 4 +-- .../apps/transfer/keeper/mbt_relay_test.go | 2 +- modules/apps/transfer/keeper/relay_test.go | 2 +- modules/apps/transfer/types/ack.go | 27 ------------------- modules/apps/transfer/types/ack_test.go | 19 ------------- .../core/04-channel/types/acknowledgement.go | 9 +------ .../04-channel/types/acknowledgement_test.go | 9 ++++--- testing/mock/mock.go | 3 ++- 15 files changed, 43 insertions(+), 112 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/events/emit_events.go delete mode 100644 modules/apps/27-interchain-accounts/host/types/ack.go delete mode 100644 modules/apps/transfer/types/ack.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ff0d73923e3..c76accbe280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`. * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`. * (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts. +* (apps/27-interchain-accounts)[\#X](https://github.com/cosmos/ibc-go/pull/X) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. +* (transfer)[\#X](https://github.com/cosmos/ibc-go/pull/X) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. +* (channel)[\#X](https://github.com/cosmos/ibc-go/pull/X) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of accidental state changes. ### State Machine Breaking diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index b0b6bd6767c..046d1169af1 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -1,9 +1,11 @@ package controller import ( + "fmt" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/events" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/keeper" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types" @@ -137,7 +139,9 @@ func (im IBCMiddleware) OnRecvPacket( packet channeltypes.Packet, _ sdk.AccAddress, ) ibcexported.Acknowledgement { - return channeltypes.NewErrorAcknowledgement("cannot receive packet on controller chain") + err := fmt.Errorf("cannot receive packet on controller chain") + events.EmitAcknowledgementErrorEvent(ctx, err) + return channeltypes.NewErrorAcknowledgement(err) } // OnAcknowledgementPacket implements the IBCMiddleware interface diff --git a/modules/apps/27-interchain-accounts/events/emit_events.go b/modules/apps/27-interchain-accounts/events/emit_events.go new file mode 100644 index 00000000000..2513e51b762 --- /dev/null +++ b/modules/apps/27-interchain-accounts/events/emit_events.go @@ -0,0 +1,19 @@ +package events + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" +) + +// EmitAcknowledgementErrorEvent emits an acknowledgement error event. +func EmitAcknowledgementErrorEvent(ctx sdk.Context, err error) { + if err != nil { + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacket, + sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), + sdk.NewAttribute(types.AttributeKeyAckError, err.Error()), + ), + ) + } +} diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 2e1ba8e0805..6e77c464afe 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -106,13 +106,13 @@ func (im IBCModule) OnRecvPacket( _ sdk.AccAddress, ) ibcexported.Acknowledgement { if !im.keeper.IsHostEnabled(ctx) { - return types.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled) + return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled) } txResponse, err := im.keeper.OnRecvPacket(ctx, packet) ack := channeltypes.NewResultAcknowledgement(txResponse) if err != nil { - ack = types.NewErrorAcknowledgement(err) + ack = channeltypes.NewErrorAcknowledgement(err) } // Emit an event indicating a successful or failed acknowledgement. diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 78a21cfee01..ea602e89b4a 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -403,7 +403,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnRecvPacket = func( ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress, ) exported.Acknowledgement { - return channeltypes.NewErrorAcknowledgement("failed OnRecvPacket mock callback") + return channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed OnRecvPacket mock callback")) } }, true, }, diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go deleted file mode 100644 index 202404fff3a..00000000000 --- a/modules/apps/27-interchain-accounts/host/types/ack.go +++ /dev/null @@ -1,27 +0,0 @@ -package types - -import ( - "fmt" - - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - - channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" -) - -const ( - // ackErrorString defines a string constant included in error acknowledgements - // NOTE: Changing this const is state machine breaking as acknowledgements are written into state - ackErrorString = "error handling packet on host chain: see events for details" -) - -// NewErrorAcknowledgement returns a deterministic error string which may be used in -// the packet acknowledgement. -func NewErrorAcknowledgement(err error) channeltypes.Acknowledgement { - // the ABCI code is included in the abcitypes.ResponseDeliverTx hash - // constructed in Tendermint and is therefore determinstic - _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-deterministic codespace and log values - - errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) - - return channeltypes.NewErrorAcknowledgement(errorString) -} diff --git a/modules/apps/27-interchain-accounts/host/types/ack_test.go b/modules/apps/27-interchain-accounts/host/types/ack_test.go index 6d2e6d1a95a..06cab3e0cc0 100644 --- a/modules/apps/27-interchain-accounts/host/types/ack_test.go +++ b/modules/apps/27-interchain-accounts/host/types/ack_test.go @@ -9,7 +9,6 @@ import ( tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" tmstate "github.com/tendermint/tendermint/state" - "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" ) @@ -80,21 +79,3 @@ func (suite *TypesTestSuite) TestABCICodeDeterminism() { suite.Require().Equal(hash, hashSameABCICode) suite.Require().NotEqual(hash, hashDifferentABCICode) } - -// TestAcknowledgementError will verify that only a constant string and -// ABCI error code are used in constructing the acknowledgement error string -func (suite *TypesTestSuite) TestAcknowledgementError() { - // same ABCI error code used - err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") - errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") - - // different ABCI error code used - errDifferentABCICode := sdkerrors.ErrNotFound - - ack := types.NewErrorAcknowledgement(err) - ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode) - ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode) - - suite.Require().Equal(ack, ackSameABCICode) - suite.Require().NotEqual(ack, ackDifferentABCICode) -} diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 07469622bfc..0bb59f1a7d0 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -179,7 +179,7 @@ func (im IBCModule) OnRecvPacket( var data types.FungibleTokenPacketData if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data") + ack = channeltypes.NewErrorAcknowledgement(fmt.Errorf("cannot unmarshal ICS-20 transfer packet data")) } // only attempt the application logic if the packet data @@ -187,7 +187,7 @@ func (im IBCModule) OnRecvPacket( if ack.Success() { err := im.keeper.OnRecvPacket(ctx, packet, data) if err != nil { - ack = types.NewErrorAcknowledgement(err) + ack = channeltypes.NewErrorAcknowledgement(err) } } diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/modules/apps/transfer/keeper/mbt_relay_test.go index 59e4869e54e..4366101f932 100644 --- a/modules/apps/transfer/keeper/mbt_relay_test.go +++ b/modules/apps/transfer/keeper/mbt_relay_test.go @@ -360,7 +360,7 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() { registerDenom() err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket( suite.chainB.GetContext(), packet, tc.packet.Data, - channeltypes.NewErrorAcknowledgement("MBT Error Acknowledgement")) + channeltypes.NewErrorAcknowledgement(fmt.Errorf("MBT Error Acknowledgement"))) default: err = fmt.Errorf("Unknown handler: %s", tc.handler) } diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index 3ad851b587e..d9d6f8a68c7 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -255,7 +255,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { var ( successAck = channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - failedAck = channeltypes.NewErrorAcknowledgement("failed packet transfer") + failedAck = channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed packet transfer")) trace types.DenomTrace amount sdk.Int path *ibctesting.Path diff --git a/modules/apps/transfer/types/ack.go b/modules/apps/transfer/types/ack.go deleted file mode 100644 index 6512f2e8371..00000000000 --- a/modules/apps/transfer/types/ack.go +++ /dev/null @@ -1,27 +0,0 @@ -package types - -import ( - "fmt" - - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - - channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" -) - -const ( - // ackErrorString defines a string constant included in error acknowledgements - // NOTE: Changing this const is state machine breaking as acknowledgements are written into state - ackErrorString = "error handling packet on destination chain: see events for details" -) - -// NewErrorAcknowledgement returns a deterministic error string which may be used in -// the packet acknowledgement. -func NewErrorAcknowledgement(err error) channeltypes.Acknowledgement { - // the ABCI code is included in the abcitypes.ResponseDeliverTx hash - // constructed in Tendermint and is therefore deterministic - _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values - - errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) - - return channeltypes.NewErrorAcknowledgement(errorString) -} diff --git a/modules/apps/transfer/types/ack_test.go b/modules/apps/transfer/types/ack_test.go index ffc09da981e..06cab3e0cc0 100644 --- a/modules/apps/transfer/types/ack_test.go +++ b/modules/apps/transfer/types/ack_test.go @@ -9,7 +9,6 @@ import ( tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" tmstate "github.com/tendermint/tendermint/state" - "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" ) @@ -80,21 +79,3 @@ func (suite *TypesTestSuite) TestABCICodeDeterminism() { suite.Require().Equal(hash, hashSameABCICode) suite.Require().NotEqual(hash, hashDifferentABCICode) } - -// TestAcknowledgementError will verify that only a constant string and -// ABCI error code are used in constructing the acknowledgement error string -func (suite *TypesTestSuite) TestAcknowledgementError() { - // same ABCI error code used - err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") - errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") - - // different ABCI error code used - errDifferentABCICode := sdkerrors.ErrNotFound - - ack := types.NewErrorAcknowledgement(err) - ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode) - ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode) - - suite.Require().Equal(ack, ackSameABCICode) - suite.Require().NotEqual(ack, ackDifferentABCICode) -} diff --git a/modules/core/04-channel/types/acknowledgement.go b/modules/core/04-channel/types/acknowledgement.go index 164ec4b5f91..49c795d0d55 100644 --- a/modules/core/04-channel/types/acknowledgement.go +++ b/modules/core/04-channel/types/acknowledgement.go @@ -29,16 +29,9 @@ func NewResultAcknowledgement(result []byte) Acknowledgement { // type in the Response field. // NOTE: Acknowledgements are written into state and thus, changes made to error strings included in packet acknowledgements // risk an app hash divergence when nodes in a network are running different patch versions of software. -func NewErrorAcknowledgement(errMsg string) Acknowledgement { +func NewErrorAcknowledgement(err error) Acknowledgement { // the ABCI code is included in the abcitypes.ResponseDeliverTx hash // constructed in Tendermint and is therefore deterministic - - errMsg = strings.TrimSpace(errMsg) - var err error - if errMsg != "" { - err = fmt.Errorf(errMsg) - } - _, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values return Acknowledgement{ diff --git a/modules/core/04-channel/types/acknowledgement_test.go b/modules/core/04-channel/types/acknowledgement_test.go index a4f62703742..8ff7adf221d 100644 --- a/modules/core/04-channel/types/acknowledgement_test.go +++ b/modules/core/04-channel/types/acknowledgement_test.go @@ -1,6 +1,9 @@ package types_test -import "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" +import ( + "fmt" + "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" +) // tests acknowledgement.ValidateBasic and acknowledgement.GetBytes func (suite TypesTestSuite) TestAcknowledgement() { @@ -18,7 +21,7 @@ func (suite TypesTestSuite) TestAcknowledgement() { }, { "valid failed ack", - types.NewErrorAcknowledgement("error"), + types.NewErrorAcknowledgement(fmt.Errorf("error")), false, true, }, @@ -30,7 +33,7 @@ func (suite TypesTestSuite) TestAcknowledgement() { }, { "empty failed ack", - types.NewErrorAcknowledgement(" "), + types.NewErrorAcknowledgement(fmt.Errorf(" ")), false, true, }, diff --git a/testing/mock/mock.go b/testing/mock/mock.go index b621a05e9f7..a4ac465353d 100644 --- a/testing/mock/mock.go +++ b/testing/mock/mock.go @@ -2,6 +2,7 @@ package mock import ( "encoding/json" + "fmt" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" @@ -28,7 +29,7 @@ const ( var ( MockAcknowledgement = channeltypes.NewResultAcknowledgement([]byte("mock acknowledgement")) - MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement("mock failed acknowledgement") + MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement(fmt.Errorf("mock failed acknowledgement")) MockPacketData = []byte("mock packet data") MockFailPacketData = []byte("mock failed packet data") MockAsyncPacketData = []byte("mock async packet data") From e49b35fc08852ade67bcbb861d45e5e2ca8f2a0d Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Wed, 22 Jun 2022 10:36:34 +0100 Subject: [PATCH 03/13] chore: added PR number to CHANGELOG.md --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c76accbe280..302a2ebc403 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,9 +47,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`. * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`. * (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts. -* (apps/27-interchain-accounts)[\#X](https://github.com/cosmos/ibc-go/pull/X) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. -* (transfer)[\#X](https://github.com/cosmos/ibc-go/pull/X) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. -* (channel)[\#X](https://github.com/cosmos/ibc-go/pull/X) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of accidental state changes. +* (apps/27-interchain-accounts)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. +* (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. +* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of accidental state changes. ### State Machine Breaking From 2fd3b3a94a100039c907f89337ef09f9e7d1d86b Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Wed, 22 Jun 2022 17:56:15 +0100 Subject: [PATCH 04/13] chore: fixed imports --- .../apps/27-interchain-accounts/controller/ibc_middleware.go | 3 ++- modules/core/04-channel/types/acknowledgement_test.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index 046d1169af1..58abab9b619 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -2,13 +2,14 @@ package controller import ( "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" - "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/events" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/keeper" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types" + "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/events" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" diff --git a/modules/core/04-channel/types/acknowledgement_test.go b/modules/core/04-channel/types/acknowledgement_test.go index 8ff7adf221d..aaaef959e2d 100644 --- a/modules/core/04-channel/types/acknowledgement_test.go +++ b/modules/core/04-channel/types/acknowledgement_test.go @@ -2,6 +2,7 @@ package types_test import ( "fmt" + "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ) From a5d118eeaa9b2ac9cb55b1d34bd4f6fa521f8306 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Thu, 23 Jun 2022 12:05:54 +0100 Subject: [PATCH 05/13] chore: addressing PR feedback --- .../controller/ibc_middleware.go | 4 +- .../controller/keeper/events.go | 27 +++++++ .../events/emit_events.go | 19 ----- modules/apps/transfer/ibc_module.go | 6 +- modules/apps/transfer/types/ack_test.go | 52 -------------- .../04-channel/types/acknowledgement_test.go | 71 +++++++++++++++++++ 6 files changed, 105 insertions(+), 74 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/events.go delete mode 100644 modules/apps/27-interchain-accounts/events/emit_events.go diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index 58abab9b619..81f6b65e05c 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -9,7 +9,6 @@ import ( "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/keeper" "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types" - "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/events" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" @@ -141,7 +140,8 @@ func (im IBCMiddleware) OnRecvPacket( _ sdk.AccAddress, ) ibcexported.Acknowledgement { err := fmt.Errorf("cannot receive packet on controller chain") - events.EmitAcknowledgementErrorEvent(ctx, err) + ack := channeltypes.NewErrorAcknowledgement(err) + keeper.EmitAcknowledgementEvent(ctx, packet, ack, err) return channeltypes.NewErrorAcknowledgement(err) } diff --git a/modules/apps/27-interchain-accounts/controller/keeper/events.go b/modules/apps/27-interchain-accounts/controller/keeper/events.go new file mode 100644 index 00000000000..116fb1cc3f7 --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/events.go @@ -0,0 +1,27 @@ +package keeper + +import ( + "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" + icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" + "github.com/cosmos/ibc-go/v3/modules/core/exported" +) + +// EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error +// details if any. +func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) { + var errorMsg string + if err != nil { + errorMsg = err.Error() + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + icatypes.EventTypePacket, + sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), + sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg), + sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), + sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + ), + ) +} diff --git a/modules/apps/27-interchain-accounts/events/emit_events.go b/modules/apps/27-interchain-accounts/events/emit_events.go deleted file mode 100644 index 2513e51b762..00000000000 --- a/modules/apps/27-interchain-accounts/events/emit_events.go +++ /dev/null @@ -1,19 +0,0 @@ -package events - -import ( - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" -) - -// EmitAcknowledgementErrorEvent emits an acknowledgement error event. -func EmitAcknowledgementErrorEvent(ctx sdk.Context, err error) { - if err != nil { - ctx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypePacket, - sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), - sdk.NewAttribute(types.AttributeKeyAckError, err.Error()), - ), - ) - } -} diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 0bb59f1a7d0..c4bb4fe5a39 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -178,8 +178,11 @@ func (im IBCModule) OnRecvPacket( ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) var data types.FungibleTokenPacketData + var ackErrorMsg string if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ack = channeltypes.NewErrorAcknowledgement(fmt.Errorf("cannot unmarshal ICS-20 transfer packet data")) + ackError := fmt.Errorf("cannot unmarshal ICS-20 transfer packet data") + ackErrorMsg = ackError.Error() + ack = channeltypes.NewErrorAcknowledgement(ackError) } // only attempt the application logic if the packet data @@ -197,6 +200,7 @@ func (im IBCModule) OnRecvPacket( sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver), + sdk.NewAttribute(types.AttributeKeyAckError, ackErrorMsg), sdk.NewAttribute(types.AttributeKeyDenom, data.Denom), sdk.NewAttribute(types.AttributeKeyAmount, data.Amount), sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), diff --git a/modules/apps/transfer/types/ack_test.go b/modules/apps/transfer/types/ack_test.go index 06cab3e0cc0..2fc13a3290e 100644 --- a/modules/apps/transfer/types/ack_test.go +++ b/modules/apps/transfer/types/ack_test.go @@ -3,20 +3,11 @@ package types_test import ( "testing" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/stretchr/testify/suite" - abcitypes "github.com/tendermint/tendermint/abci/types" - tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" - tmstate "github.com/tendermint/tendermint/state" ibctesting "github.com/cosmos/ibc-go/v3/testing" ) -const ( - gasUsed = uint64(100) - gasWanted = uint64(100) -) - type TypesTestSuite struct { suite.Suite @@ -36,46 +27,3 @@ func (suite *TypesTestSuite) SetupTest() { func TestTypesTestSuite(t *testing.T) { suite.Run(t, new(TypesTestSuite)) } - -// The safety of including ABCI error codes in the acknowledgement rests -// on the inclusion of these ABCI error codes in the abcitypes.ResposneDeliverTx -// hash. If the ABCI codes get removed from consensus they must no longer be used -// in the packet acknowledgement. -// -// This test acts as an indicator that the ABCI error codes may no longer be deterministic. -func (suite *TypesTestSuite) TestABCICodeDeterminism() { - // same ABCI error code used - err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") - errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") - - // different ABCI error code used - errDifferentABCICode := sdkerrors.ErrNotFound - - deliverTx := sdkerrors.ResponseDeliverTx(err, gasUsed, gasWanted, false) - responses := tmprotostate.ABCIResponses{ - DeliverTxs: []*abcitypes.ResponseDeliverTx{ - &deliverTx, - }, - } - - deliverTxSameABCICode := sdkerrors.ResponseDeliverTx(errSameABCICode, gasUsed, gasWanted, false) - responsesSameABCICode := tmprotostate.ABCIResponses{ - DeliverTxs: []*abcitypes.ResponseDeliverTx{ - &deliverTxSameABCICode, - }, - } - - deliverTxDifferentABCICode := sdkerrors.ResponseDeliverTx(errDifferentABCICode, gasUsed, gasWanted, false) - responsesDifferentABCICode := tmprotostate.ABCIResponses{ - DeliverTxs: []*abcitypes.ResponseDeliverTx{ - &deliverTxDifferentABCICode, - }, - } - - hash := tmstate.ABCIResponsesResultsHash(&responses) - hashSameABCICode := tmstate.ABCIResponsesResultsHash(&responsesSameABCICode) - hashDifferentABCICode := tmstate.ABCIResponsesResultsHash(&responsesDifferentABCICode) - - suite.Require().Equal(hash, hashSameABCICode) - suite.Require().NotEqual(hash, hashDifferentABCICode) -} diff --git a/modules/core/04-channel/types/acknowledgement_test.go b/modules/core/04-channel/types/acknowledgement_test.go index aaaef959e2d..530d288bb32 100644 --- a/modules/core/04-channel/types/acknowledgement_test.go +++ b/modules/core/04-channel/types/acknowledgement_test.go @@ -3,9 +3,19 @@ package types_test import ( "fmt" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + abcitypes "github.com/tendermint/tendermint/abci/types" + tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state" + tmstate "github.com/tendermint/tendermint/state" + "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ) +const ( + gasUsed = uint64(100) + gasWanted = uint64(100) +) + // tests acknowledgement.ValidateBasic and acknowledgement.GetBytes func (suite TypesTestSuite) TestAcknowledgement() { testCases := []struct { @@ -72,3 +82,64 @@ func (suite TypesTestSuite) TestAcknowledgement() { }) } } + +// The safety of including ABCI error codes in the acknowledgement rests +// on the inclusion of these ABCI error codes in the abcitypes.ResposneDeliverTx +// hash. If the ABCI codes get removed from consensus they must no longer be used +// in the packet acknowledgement. +// +// This test acts as an indicator that the ABCI error codes may no longer be deterministic. +func (suite *TypesTestSuite) TestABCICodeDeterminism() { + // same ABCI error code used + err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") + errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") + + // different ABCI error code used + errDifferentABCICode := sdkerrors.ErrNotFound + + deliverTx := sdkerrors.ResponseDeliverTx(err, gasUsed, gasWanted, false) + responses := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTx, + }, + } + + deliverTxSameABCICode := sdkerrors.ResponseDeliverTx(errSameABCICode, gasUsed, gasWanted, false) + responsesSameABCICode := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTxSameABCICode, + }, + } + + deliverTxDifferentABCICode := sdkerrors.ResponseDeliverTx(errDifferentABCICode, gasUsed, gasWanted, false) + responsesDifferentABCICode := tmprotostate.ABCIResponses{ + DeliverTxs: []*abcitypes.ResponseDeliverTx{ + &deliverTxDifferentABCICode, + }, + } + + hash := tmstate.ABCIResponsesResultsHash(&responses) + hashSameABCICode := tmstate.ABCIResponsesResultsHash(&responsesSameABCICode) + hashDifferentABCICode := tmstate.ABCIResponsesResultsHash(&responsesDifferentABCICode) + + suite.Require().Equal(hash, hashSameABCICode) + suite.Require().NotEqual(hash, hashDifferentABCICode) +} + +// TestAcknowledgementError will verify that only a constant string and +// ABCI error code are used in constructing the acknowledgement error string +func (suite *TypesTestSuite) TestAcknowledgementError() { + // same ABCI error code used + err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1") + errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2") + + // different ABCI error code used + errDifferentABCICode := sdkerrors.ErrNotFound + + ack := types.NewErrorAcknowledgement(err) + ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode) + ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode) + + suite.Require().Equal(ack, ackSameABCICode) + suite.Require().NotEqual(ack, ackDifferentABCICode) +} From 647d1e1d78ab9d8bb8e4110dd89ff153524ece22 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Thu, 23 Jun 2022 12:15:49 +0100 Subject: [PATCH 06/13] fix: displaying ack error message only if there is an error unmarshalling. --- modules/apps/transfer/ibc_module.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index c4bb4fe5a39..9681c3c2a08 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -178,11 +178,22 @@ func (im IBCModule) OnRecvPacket( ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) var data types.FungibleTokenPacketData - var ackErrorMsg string + var ackErr error if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ackError := fmt.Errorf("cannot unmarshal ICS-20 transfer packet data") - ackErrorMsg = ackError.Error() - ack = channeltypes.NewErrorAcknowledgement(ackError) + ackErr = fmt.Errorf("cannot unmarshal ICS-20 transfer packet data") + ack = channeltypes.NewErrorAcknowledgement(ackErr) + } + + eventAttributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), + 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())), + } + + if ackErr != nil { + eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error())) } // only attempt the application logic if the packet data @@ -197,13 +208,7 @@ func (im IBCModule) OnRecvPacket( ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypePacket, - sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), - sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), - sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver), - sdk.NewAttribute(types.AttributeKeyAckError, ackErrorMsg), - sdk.NewAttribute(types.AttributeKeyDenom, data.Denom), - sdk.NewAttribute(types.AttributeKeyAmount, data.Amount), - sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + eventAttributes..., ), ) From ea311bd5f79573a3560e9d0ed03885e5c0dfc981 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Fri, 24 Jun 2022 09:06:34 +0100 Subject: [PATCH 07/13] chore: addressed PR feedback --- .../controller/ibc_middleware.go | 2 +- .../controller/keeper/events.go | 14 ++++++++------ .../27-interchain-accounts/host/keeper/events.go | 14 ++++++++------ .../apps/27-interchain-accounts/types/events.go | 7 ++++--- modules/apps/transfer/ibc_module.go | 9 +++++---- 5 files changed, 26 insertions(+), 20 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index eaf3207abf9..6599f820250 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -143,7 +143,7 @@ func (im IBCMiddleware) OnRecvPacket( err := fmt.Errorf("cannot receive packet on controller chain") ack := channeltypes.NewErrorAcknowledgement(err) keeper.EmitAcknowledgementEvent(ctx, packet, ack, err) - return channeltypes.NewErrorAcknowledgement(err) + return ack } // OnAcknowledgementPacket implements the IBCMiddleware interface diff --git a/modules/apps/27-interchain-accounts/controller/keeper/events.go b/modules/apps/27-interchain-accounts/controller/keeper/events.go index 116fb1cc3f7..be7446a6d19 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/events.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/events.go @@ -10,18 +10,20 @@ import ( // EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error // details if any. func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) { - var errorMsg string + attributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), + sdk.NewAttribute(icatypes.AttributeKeyControllerChannelID, packet.GetDestChannel()), + sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + } + if err != nil { - errorMsg = err.Error() + attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error())) } ctx.EventManager().EmitEvent( sdk.NewEvent( icatypes.EventTypePacket, - sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), - sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg), - sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), - sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + attributes..., ), ) } diff --git a/modules/apps/27-interchain-accounts/host/keeper/events.go b/modules/apps/27-interchain-accounts/host/keeper/events.go index 116fb1cc3f7..2429acf18a3 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/events.go +++ b/modules/apps/27-interchain-accounts/host/keeper/events.go @@ -10,18 +10,20 @@ import ( // EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error // details if any. func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) { - var errorMsg string + attributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), + sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), + sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + } + if err != nil { - errorMsg = err.Error() + attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error())) } ctx.EventManager().EmitEvent( sdk.NewEvent( icatypes.EventTypePacket, - sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), - sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg), - sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), - sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + attributes..., ), ) } diff --git a/modules/apps/27-interchain-accounts/types/events.go b/modules/apps/27-interchain-accounts/types/events.go index 9bfd1df3049..3ca4b783b55 100644 --- a/modules/apps/27-interchain-accounts/types/events.go +++ b/modules/apps/27-interchain-accounts/types/events.go @@ -4,7 +4,8 @@ package types const ( EventTypePacket = "ics27_packet" - AttributeKeyAckError = "error" - AttributeKeyHostChannelID = "host_channel_id" - AttributeKeyAckSuccess = "success" + AttributeKeyAckError = "error" + AttributeKeyHostChannelID = "host_channel_id" + AttributeKeyControllerChannelID = "controller_channel_id" + AttributeKeyAckSuccess = "success" ) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 9681c3c2a08..bcae5483e01 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -192,19 +192,20 @@ func (im IBCModule) OnRecvPacket( sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), } - if ackErr != nil { - eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error())) - } - // only attempt the application logic if the packet data // was successfully decoded if ack.Success() { err := im.keeper.OnRecvPacket(ctx, packet, data) if err != nil { ack = channeltypes.NewErrorAcknowledgement(err) + ackErr = err } } + if ackErr != nil { + eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error())) + } + ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypePacket, From 8e5eb6e8e839512f717fa4b35510ca471d82c311 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 27 Jun 2022 08:39:53 +0100 Subject: [PATCH 08/13] chore: fixing formatting, adding changes to migration doc. --- docs/migrations/v3-to-v4.md | 3 +++ .../controller/keeper/events.go | 2 ++ modules/apps/transfer/ibc_module.go | 16 ++++++++-------- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index 49f311a844e..ef61ba88e64 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -26,6 +26,9 @@ This is an API breaking change and as such IBC application developers will have The `OnChanOpenInit` application callback has been modified. The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629). +The `NewErrorAcknowledgement` method signature has changed. +It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes. + ### ICS27 - Interchain Accounts The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets. diff --git a/modules/apps/27-interchain-accounts/controller/keeper/events.go b/modules/apps/27-interchain-accounts/controller/keeper/events.go index be7446a6d19..dab19a846bc 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/events.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/events.go @@ -2,8 +2,10 @@ package keeper import ( "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" + "github.com/cosmos/ibc-go/v3/modules/core/exported" ) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index bcae5483e01..ec55f714bbf 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -184,14 +184,6 @@ func (im IBCModule) OnRecvPacket( ack = channeltypes.NewErrorAcknowledgement(ackErr) } - eventAttributes := []sdk.Attribute{ - sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), - 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())), - } - // only attempt the application logic if the packet data // was successfully decoded if ack.Success() { @@ -202,6 +194,14 @@ func (im IBCModule) OnRecvPacket( } } + eventAttributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeySender, data.Sender), + 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())), + } + if ackErr != nil { eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error())) } From 52ede1a9cc29b4b5ddccecf6e28b06daff909397 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 27 Jun 2022 17:04:50 +0100 Subject: [PATCH 09/13] Update modules/apps/27-interchain-accounts/controller/keeper/events.go Co-authored-by: Damian Nolan --- modules/apps/27-interchain-accounts/controller/keeper/events.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/events.go b/modules/apps/27-interchain-accounts/controller/keeper/events.go index dab19a846bc..3c820c74cb0 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/events.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/events.go @@ -4,8 +4,8 @@ import ( "fmt" sdk "github.com/cosmos/cosmos-sdk/types" + icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" - "github.com/cosmos/ibc-go/v3/modules/core/exported" ) From c48071850a42dd9bea0e66ed263b3395d6ac1267 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 27 Jun 2022 17:31:42 +0100 Subject: [PATCH 10/13] Update docs/migrations/v3-to-v4.md Co-authored-by: Damian Nolan --- docs/migrations/v3-to-v4.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index ef61ba88e64..73905e77695 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -28,6 +28,7 @@ The return signature now includes the application version as detailed in the lat The `NewErrorAcknowledgement` method signature has changed. It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes. +All error acknowledgements now contain a deterministic ABCI code and error message. It is the responsibility of the application developer to emit error details in events. ### ICS27 - Interchain Accounts From 84d2d446f645e409c7d5a55a1c1bdd71d0990388 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Tue, 28 Jun 2022 10:31:42 +0100 Subject: [PATCH 11/13] Update CHANGELOG.md Co-authored-by: Damian Nolan --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f4d3912661..900ef224f5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,7 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts. * (apps/27-interchain-accounts)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. * (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`. -* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of accidental state changes. +* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of non-deterministic writes to application state. ### State Machine Breaking From d3e72ea36c64a5a8e6250ba91bf54fa353be7924 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Tue, 28 Jun 2022 12:52:51 +0100 Subject: [PATCH 12/13] chore: wrapping errors using sdkerrors.Wrapf --- .../apps/27-interchain-accounts/controller/ibc_middleware.go | 4 +--- modules/apps/transfer/ibc_module.go | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index 6599f820250..cbcafbd248c 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -1,8 +1,6 @@ package controller import ( - "fmt" - sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" @@ -140,7 +138,7 @@ func (im IBCMiddleware) OnRecvPacket( packet channeltypes.Packet, _ sdk.AccAddress, ) ibcexported.Acknowledgement { - err := fmt.Errorf("cannot receive packet on controller chain") + err := sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot receive packet on controller chain") ack := channeltypes.NewErrorAcknowledgement(err) keeper.EmitAcknowledgementEvent(ctx, packet, ack, err) return ack diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index ec55f714bbf..bc8ce5462c7 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -2,6 +2,7 @@ package transfer import ( "fmt" + icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" "math" "strings" @@ -180,7 +181,7 @@ func (im IBCModule) OnRecvPacket( var data types.FungibleTokenPacketData var ackErr error if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ackErr = fmt.Errorf("cannot unmarshal ICS-20 transfer packet data") + ackErr = sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot unmarshal ICS-20 transfer packet data") ack = channeltypes.NewErrorAcknowledgement(ackErr) } From 50dfd891f872bf25e03ee4a7f6ae0385fe94d1a6 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Tue, 28 Jun 2022 13:46:34 +0100 Subject: [PATCH 13/13] chore: fixing imports --- modules/apps/transfer/ibc_module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index bc8ce5462c7..d79ec986aab 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -2,7 +2,6 @@ package transfer import ( "fmt" - icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" "math" "strings" @@ -10,6 +9,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" "github.com/cosmos/ibc-go/v3/modules/apps/transfer/keeper" "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"