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

Spike error ack #1299

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 11 additions & 14 deletions x/wasm/ibc.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,26 +264,23 @@ 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 {
// the state gets reverted, so we drop all captured events
types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
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
// emit all contract and submessage events on success
ctx.EventManager().EmitEvents(em.Events())
types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
return ack
}

// OnAcknowledgementPacket implements the IBCModule interface
Expand Down
34 changes: 29 additions & 5 deletions x/wasm/keeper/relay.go
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
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
return nil, err
}
// success ACK
return ContractConfirmStateAck(data), nil
Comment on lines +150 to +151
Copy link
Member

Choose a reason for hiding this comment

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

I think this code or comment is misleading because it can be either a success or and error or a non-binary acknowledgement type. We should not try to give it any meaning.

Copy link
Contributor Author

@alpe alpe Mar 31, 2023

Choose a reason for hiding this comment

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

On packet receive, success can be defined as case 1 + 2 of #697 (comment) . The contract was able to fully handle the packet. State will be committed.

}

var _ ibcexported.Acknowledgement = ContractConfirmStateAck{}

type ContractConfirmStateAck []byte

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

Choose a reason for hiding this comment

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

Is there any documentation what Success() in the ibcexported.Acknowledgement means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best source for Success() that I found is the code that calls this in ibc-go.

Copy link
Member

Choose a reason for hiding this comment

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

I raised the issue here: cosmos/ibc-go#3434


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
8 changes: 4 additions & 4 deletions x/wasm/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ func TestOnRecvPacket(t *testing.T) {
overwriteMessenger *wasmtesting.MockMessageHandler
mockReplyFn func(codeID wasmvm.Checksum, env wasmvmtypes.Env, reply wasmvmtypes.Reply, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error)
expContractGas sdk.Gas
expAck []byte
expAck ContractConfirmStateAck
expErr bool
expPanic bool
expEventTypes []string
Expand Down Expand Up @@ -370,7 +370,7 @@ func TestOnRecvPacket(t *testing.T) {
Attributes: []wasmvmtypes.EventAttribute{{Key: "Foo", Value: "Bar"}},
},
expEventTypes: []string{types.WasmModuleEventType},
expAck: []byte("myAck"),
expAck: ContractConfirmStateAck("myAck"),
},
"emit contract events on success": {
contractAddr: example.Contract,
Expand All @@ -387,7 +387,7 @@ func TestOnRecvPacket(t *testing.T) {
}},
},
expEventTypes: []string{types.WasmModuleEventType, "wasm-custom"},
expAck: []byte("myAck"),
expAck: ContractConfirmStateAck("myAck"),
},
"messenger errors returned, events stored": {
contractAddr: example.Contract,
Expand All @@ -411,7 +411,7 @@ func TestOnRecvPacket(t *testing.T) {
mockReplyFn: func(codeID wasmvm.Checksum, env wasmvmtypes.Env, reply wasmvmtypes.Reply, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error) {
return &wasmvmtypes.Response{Data: []byte("myBetterAck")}, 0, nil
},
expAck: []byte("myBetterAck"),
expAck: ContractConfirmStateAck("myBetterAck"),
expEventTypes: []string{types.EventTypeReply},
},
"unknown contract address": {
Expand Down
32 changes: 32 additions & 0 deletions x/wasm/types/events.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
package types

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

const (
// WasmModuleEventType is stored with any contract TX that returns non empty EventAttributes
WasmModuleEventType = "wasm"
Expand All @@ -17,8 +24,31 @@ const (
EventTypeGovContractResult = "gov_contract_result"
EventTypeUpdateContractAdmin = "update_contract_admin"
EventTypeUpdateCodeAccessConfig = "update_code_access_config"
EventTypePacketRecv = "ibc_packet_received"
// add new types to IsAcceptedEventOnRecvPacketErrorAck
)

// EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error
// details if any.
func EmitAcknowledgementEvent(ctx sdk.Context, contractAddr sdk.AccAddress, ack exported.Acknowledgement, err error) {
attributes := []sdk.Attribute{
sdk.NewAttribute(sdk.AttributeKeyModule, ModuleName),
sdk.NewAttribute(AttributeKeyContractAddr, contractAddr.String()),
sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
}

if err != nil {
attributes = append(attributes, sdk.NewAttribute(AttributeKeyAckError, err.Error()))
}

ctx.EventManager().EmitEvent(
sdk.NewEvent(
EventTypePacketRecv,
attributes...,
),
)
}

// event attributes returned from contract execution
const (
AttributeReservedPrefix = "_"
Expand All @@ -31,4 +61,6 @@ const (
AttributeKeyNewAdmin = "new_admin_address"
AttributeKeyCodePermission = "code_permission"
AttributeKeyAuthorizedAddresses = "authorized_addresses"
AttributeKeyAckSuccess = "success"
AttributeKeyAckError = "error"
)
3 changes: 2 additions & 1 deletion x/wasm/types/exported_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
wasmvmtypes "github.com/CosmWasm/wasmvm/types"
sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
)

// ViewKeeper provides read only operations
Expand Down Expand Up @@ -100,7 +101,7 @@ type IBCContractKeeper interface {
ctx sdk.Context,
contractAddr sdk.AccAddress,
msg wasmvmtypes.IBCPacketReceiveMsg,
) ([]byte, error)
) (ibcexported.Acknowledgement, error)
OnAckPacket(
ctx sdk.Context,
contractAddr sdk.AccAddress,
Expand Down