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.

Sign up for GitHub

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

Redesign IBC on packet recv error/ result.Err handling #1353

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Redesign IBC on packet recv error/ result.Err handling #1353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 17 additions & 17 deletions x/wasm/ibc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,26 +264,26 @@ func (i IBCHandler) OnRecvPacket(
) ibcexported.Acknowledgement {
contractAddr, err := ContractFromPortID(packet.DestinationPort)
if err != nil {
return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(err, "contract port id"))
// this must not happen as ports were registered before
panic(errorsmod.Wrapf(err, "contract port id"))
}

em := sdk.NewEventManager()
msg := wasmvmtypes.IBCPacketReceiveMsg{Packet: newIBCPacket(packet), Relayer: relayer.String()}
ack, err := i.keeper.OnRecvPacket(ctx, contractAddr, msg)
ack, err := i.keeper.OnRecvPacket(ctx.WithEventManager(em), contractAddr, msg)
if err != nil {
return channeltypes.NewErrorAcknowledgement(err)
}
return ContractConfirmStateAck(ack)
}

var _ ibcexported.Acknowledgement = ContractConfirmStateAck{}

type ContractConfirmStateAck []byte

func (w ContractConfirmStateAck) Success() bool {
return true // always commit state
}

func (w ContractConfirmStateAck) Acknowledgement() []byte {
return w
ack = channeltypes.NewErrorAcknowledgement(err)
types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
// the state gets reverted, so we drop all captured events
return ack
}
if ack == nil || ack.Success() {
// emit all contract and submessage events on success
// nil ack is a success case, see: https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/keeper/msg_server.go#L453
ctx.EventManager().EmitEvents(em.Events())
}
types.EmitAcknowledgementEvent(ctx, contractAddr, ack, nil)
return ack
}

// OnAcknowledgementPacket implements the IBCModule interface
Expand Down
128 changes: 128 additions & 0 deletions x/wasm/ibc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,125 @@ import (
"testing"

wasmvmtypes "github.com/CosmWasm/wasmvm/types"
abci "github.com/cometbft/cometbft/abci/types"
"github.com/cometbft/cometbft/libs/rand"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/CosmWasm/wasmd/x/wasm/keeper"
"github.com/CosmWasm/wasmd/x/wasm/types"
)

func TestOnRecvPacket(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good tests! 👍

anyRelayerAddr := sdk.AccAddress(rand.Bytes(address.Len))
anyContractIBCPkg := IBCPacketFixture(func(p *channeltypes.Packet) {
p.DestinationPort = "wasm.cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f"
})
myCustomEvent := sdk.NewEvent("testing")
specs := map[string]struct {
ibcPkg channeltypes.Packet
contractRsp ibcexported.Acknowledgement
contractErr error
expEvents sdk.Events
expPanic bool
expAck ibcexported.Acknowledgement
}{
"contract returns success response": {
ibcPkg: anyContractIBCPkg,
contractRsp: keeper.ContractConfirmStateAck([]byte{1}),
expAck: keeper.ContractConfirmStateAck([]byte{1}),
expEvents: sdk.Events{
myCustomEvent,
{
Type: "ibc_packet_received",
Attributes: []abci.EventAttribute{
{Key: "module", Value: "wasm"},
{Key: "_contract_address", Value: "cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f"},
{Key: "success", Value: "true"},
},
},
},
},
"contract returns err response": {
ibcPkg: anyContractIBCPkg,
contractRsp: channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
expAck: channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
expEvents: sdk.Events{
{
Type: "ibc_packet_received",
Attributes: []abci.EventAttribute{
{Key: "module", Value: "wasm"},
{Key: "_contract_address", Value: "cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f"},
{Key: "success", Value: "false"},
},
},
},
},
"nil considered success response": { // regression only
ibcPkg: anyContractIBCPkg,
expEvents: sdk.Events{
myCustomEvent,
{
Type: "ibc_packet_received",
Attributes: []abci.EventAttribute{
{Key: "module", Value: "wasm"},
{Key: "_contract_address", Value: "cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f"},
{Key: "success", Value: "true"},
},
},
},
},
"unknown contract port": {
ibcPkg: IBCPacketFixture(func(p *channeltypes.Packet) {
p.DestinationPort = "not-a-contract-port"
}),
expPanic: true,
},
"contract executed with error": {
ibcPkg: anyContractIBCPkg,
contractErr: types.ErrInvalid.Wrap("testing"),
expAck: channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
expEvents: sdk.Events{{
Type: "ibc_packet_received",
Attributes: []abci.EventAttribute{
{Key: "module", Value: "wasm"},
{Key: "_contract_address", Value: "cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f"},
{Key: "success", Value: "false"},
{Key: "error", Value: "testing: invalid"}, // not redacted
},
}},
},
}
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
mock := IBCContractKeeperMock{
OnRecvPacketFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error) {
// additional custom event to confirm event handling on state commit/ rollback
ctx.EventManager().EmitEvent(myCustomEvent)
return spec.contractRsp, spec.contractErr
},
}
h := NewIBCHandler(mock, nil, nil)
em := &sdk.EventManager{}
ctx := sdk.Context{}.WithEventManager(em)
if spec.expPanic {
require.Panics(t, func() {
_ = h.OnRecvPacket(ctx, spec.ibcPkg, anyRelayerAddr)
})
return
}
gotAck := h.OnRecvPacket(ctx, spec.ibcPkg, anyRelayerAddr)
assert.Equal(t, spec.expAck, gotAck)
assert.Equal(t, spec.expEvents, em.Events())
})
}
}

func TestMapToWasmVMIBCPacket(t *testing.T) {
var myTimestamp uint64 = 1
specs := map[string]struct {
Expand Down Expand Up @@ -80,3 +194,17 @@ func IBCPacketFixture(mutators ...func(p *channeltypes.Packet)) channeltypes.Pac
}
return r
}

var _ types.IBCContractKeeper = &IBCContractKeeperMock{}

type IBCContractKeeperMock struct {
types.IBCContractKeeper
OnRecvPacketFn func(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error)
}

func (m IBCContractKeeperMock) OnRecvPacket(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error) {
if m.OnRecvPacketFn == nil {
panic("not expected to be called")
}
return m.OnRecvPacketFn(ctx, contractAddr, msg)
}
34 changes: 29 additions & 5 deletions x/wasm/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package keeper
import (
"time"

channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"

errorsmod "cosmossdk.io/errors"
wasmvmtypes "github.com/CosmWasm/wasmvm/types"
"github.com/cosmos/cosmos-sdk/telemetry"
Expand Down Expand Up @@ -116,7 +119,7 @@ func (k Keeper) OnRecvPacket(
ctx sdk.Context,
contractAddr sdk.AccAddress,
msg wasmvmtypes.IBCPacketReceiveMsg,
) ([]byte, error) {
) (ibcexported.Acknowledgement, error) {
defer telemetry.MeasureSince(time.Now(), "wasm", "contract", "ibc-recv-packet")
contractInfo, codeInfo, prefixStore, err := k.contractInstance(ctx, contractAddr)
if err != nil {
Expand All @@ -130,13 +133,34 @@ func (k Keeper) OnRecvPacket(
res, gasUsed, execErr := k.wasmVM.IBCPacketReceive(codeInfo.CodeHash, env, msg, prefixStore, cosmwasmAPI, querier, ctx.GasMeter(), gas, costJSONDeserialization)
k.consumeRuntimeGas(ctx, gasUsed)
if execErr != nil {
panic(execErr)
panic(execErr) // let contract fully abort IBC receive in certain case
}
if res.Err != "" { // handle error case as before https://github.com/CosmWasm/wasmvm/commit/c300106fe5c9426a495f8e10821e00a9330c56c6
return nil, errorsmod.Wrap(types.ErrExecuteFailed, res.Err)
if res.Err != "" {
// return error ACK with non-redacted contract message, state will be reverted
return channeltypes.Acknowledgement{
Response: &channeltypes.Acknowledgement_Error{Error: res.Err},
}, nil
}
// note submessage reply results can overwrite the `Acknowledgement` data
return k.handleContractResponse(ctx, contractAddr, contractInfo.IBCPortID, res.Ok.Messages, res.Ok.Attributes, res.Ok.Acknowledgement, res.Ok.Events)
data, err := k.handleContractResponse(ctx, contractAddr, contractInfo.IBCPortID, res.Ok.Messages, res.Ok.Attributes, res.Ok.Acknowledgement, res.Ok.Events)
if err != nil {
// submessage errors result in error ACK with state reverted. Error message is redacted
return nil, err
}
// success ACK, state will be committed
return ContractConfirmStateAck(data), nil
}

var _ ibcexported.Acknowledgement = ContractConfirmStateAck{}

type ContractConfirmStateAck []byte

func (w ContractConfirmStateAck) Success() bool {
return true // always commit state
}

func (w ContractConfirmStateAck) Acknowledgement() []byte {
return w
}

// OnAckPacket calls the contract to handle the "acknowledgement" data which can contain success or failure of a packet
Expand Down
Loading