Skip to content

Commit

Permalink
refactor: construct ics27 error acknowledgement with determinstic ABC…
Browse files Browse the repository at this point in the history
…I code (#794)

## Description

Splitting #701 into 3 parts:
- error acknowledgement changes
- results acknowledgement changes
- ADR explaining justification

This pr handles the first case

ref: #701 

---

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.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] 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
  • Loading branch information
colin-axner authored Jan 28, 2022
1 parent 4f70554 commit 19b5b5f
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 7 deletions.
2 changes: 1 addition & 1 deletion modules/apps/27-interchain-accounts/host/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (im IBCModule) OnRecvPacket(
ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})

if err := im.keeper.OnRecvPacket(ctx, packet); err != nil {
ack = channeltypes.NewErrorAcknowledgement(icatypes.AcknowledgementError)
ack = types.NewErrorAcknowledgement(err)

// Emit an event including the error msg
keeper.EmitWriteErrorAcknowledgementEvent(ctx, packet, err)
Expand Down
27 changes: 27 additions & 0 deletions modules/apps/27-interchain-accounts/host/types/ack.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
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"
)

// AcknowledgementErrorString 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-determinstic codespace and log values

errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString)

return channeltypes.NewErrorAcknowledgement(errorString)
}
101 changes: 101 additions & 0 deletions modules/apps/27-interchain-accounts/host/types/ack_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
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/27-interchain-accounts/host/types"
ibctesting "github.com/cosmos/ibc-go/v3/testing"
)

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

type TypesTestSuite struct {
suite.Suite

coordinator *ibctesting.Coordinator

chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
}

func (suite *TypesTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)

suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
}

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() {
// 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)

}
6 changes: 0 additions & 6 deletions modules/apps/27-interchain-accounts/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,6 @@ import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

const (
// AcknowledgementError defines a string constant included in error acknowledgements
// NOTE: Changing this const is state machine breaking as acknowledgements are written into state
AcknowledgementError = "error handling packet on host chain: see events for details"
)

var (
ErrUnknownDataType = sdkerrors.Register(ModuleName, 2, "unknown data type")
ErrAccountAlreadyExist = sdkerrors.Register(ModuleName, 3, "account already exist")
Expand Down

0 comments on commit 19b5b5f

Please sign in to comment.