Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate usage of NewErrorAcknowledgement #1565

Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c264fa0
feat: include ABCI code in failed acknowledgement.
chatton Jun 14, 2022
1ad5de5
fix: removing transfertypes.NewErrorAcknowledgement and interchainacc…
chatton Jun 22, 2022
e49b35f
chore: added PR number to CHANGELOG.md
chatton Jun 22, 2022
8283fbe
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 22, 2022
2fd3b3a
chore: fixed imports
chatton Jun 22, 2022
38c89eb
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 22, 2022
2102783
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 23, 2022
a5d118e
chore: addressing PR feedback
chatton Jun 23, 2022
647d1e1
fix: displaying ack error message only if there is an error unmarshal…
chatton Jun 23, 2022
0cd0af3
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 23, 2022
ea311bd
chore: addressed PR feedback
chatton Jun 24, 2022
8e5eb6e
chore: fixing formatting, adding changes to migration doc.
chatton Jun 27, 2022
725ef86
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 27, 2022
52ede1a
Update modules/apps/27-interchain-accounts/controller/keeper/events.go
chatton Jun 27, 2022
e0a953f
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 27, 2022
c480718
Update docs/migrations/v3-to-v4.md
chatton Jun 27, 2022
d1089b9
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
seantking Jun 27, 2022
230a219
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 28, 2022
6c15291
Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…
chatton Jun 28, 2022
84d2d44
Update CHANGELOG.md
chatton Jun 28, 2022
d3e72ea
chore: wrapping errors using sdkerrors.Wrapf
chatton Jun 28, 2022
50dfd89
chore: fixing imports
chatton Jun 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)[\#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 non-deterministic writes to application state.

### State Machine Breaking

Expand Down
4 changes: 4 additions & 0 deletions docs/migrations/v3-to-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ 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.
chatton marked this conversation as resolved.
Show resolved Hide resolved
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

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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,10 @@ func (im IBCMiddleware) OnRecvPacket(
packet channeltypes.Packet,
_ sdk.AccAddress,
) ibcexported.Acknowledgement {
return channeltypes.NewErrorAcknowledgement("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
}

// OnAcknowledgementPacket implements the IBCMiddleware interface
Expand Down
31 changes: 31 additions & 0 deletions modules/apps/27-interchain-accounts/controller/keeper/events.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package keeper
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now I duplicated controller/keeper/events.go to avoid calling into a different keeper package and also to not break current conventions.


import (
"fmt"
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved

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) {
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 {
attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error()))
}

ctx.EventManager().EmitEvent(
sdk.NewEvent(
icatypes.EventTypePacket,
attributes...,
),
)
}
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down
14 changes: 8 additions & 6 deletions modules/apps/27-interchain-accounts/host/keeper/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...,
),
)
}
27 changes: 0 additions & 27 deletions modules/apps/27-interchain-accounts/host/types/ack.go

This file was deleted.

19 changes: 0 additions & 19 deletions modules/apps/27-interchain-accounts/host/types/ack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
7 changes: 4 additions & 3 deletions modules/apps/27-interchain-accounts/types/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
27 changes: 19 additions & 8 deletions modules/apps/transfer/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package transfer

import (
"fmt"
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting :)

"math"
"strings"

Expand Down Expand Up @@ -178,28 +179,38 @@ func (im IBCModule) OnRecvPacket(
ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})

var data types.FungibleTokenPacketData
var ackErr error
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data")
ackErr = sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot unmarshal ICS-20 transfer packet data")
ack = channeltypes.NewErrorAcknowledgement(ackErr)
}

// 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 = types.NewErrorAcknowledgement(err)
ack = channeltypes.NewErrorAcknowledgement(err)
ackErr = err
}
}

crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this!

}

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.AttributeKeyDenom, data.Denom),
sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
eventAttributes...,
),
)

Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/mbt_relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 0 additions & 27 deletions modules/apps/transfer/types/ack.go

This file was deleted.

71 changes: 0 additions & 71 deletions modules/apps/transfer/types/ack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +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"

"github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
ibctesting "github.com/cosmos/ibc-go/v3/testing"
)

const (
gasUsed = uint64(100)
gasWanted = uint64(100)
)

type TypesTestSuite struct {
suite.Suite

Expand All @@ -37,64 +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)
}

// TestAcknowledgementError will verify that only a constant string and
// ABCI error code are used in constructing the acknowledgement error string
func (suite *TypesTestSuite) TestAcknowledgementError() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep one of these two TestAcknowledgementError tests and move it to 04-channel?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the TestABCICodeDeterminism test should also be moved to 04-channel, its duplicated both above here and in ICA tests

// 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)
}
Loading