MessageHandlerChain DispatchMsg may be a problem

[BananaLF]

Edit (ethanfrey): To solve we just need to add validation logic than only one variant of CosmosMsg is set. Read comments below.

The MessageHandlerChain.DispatchMsg may be problem.

wasmd/x/wasm/keeper/handler_plugin.go

Lines 128 to 141 in 2886092

	func (m MessageHandlerChain) DispatchMsg(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) ([]sdk.Event, [][]byte, error) {
	for _, h := range m.handlers {
	events, data, err := h.DispatchMsg(ctx, contractAddr, contractIBCPortID, msg)
	switch {
	case err == nil:
	return events, data, nil
	case errors.Is(err, types.ErrUnknownMsg):
	continue
	default:
	return events, data, err
	}
	}
	return nil, nil, sdkerrors.Wrap(types.ErrUnknownMsg, "no handler found")
	}

This Line ignore err.unknownMsg when handler deal with unknownMsg which is correct in other handler. (like IBCRawPacketHandler deal with BankMsg ).

wasmd/x/wasm/keeper/handler_plugin.go

Line 134 in 2886092

case errors.Is(err, types.ErrUnknownMsg):

But I think This Line can not ignore ,because handler may be deal with unknownMsg which EncodeBankMsg is error, other Encode**Msg is the same.

Like fllow:

wasmd/x/wasm/keeper/handler_plugin_encoders.go

Line 109 in 2886092

func EncodeBankMsg(sender sdk.AccAddress, msg *wasmvmtypes.BankMsg) ([]sdk.Msg, error) {

wasmd/x/wasm/keeper/handler_plugin_encoders.go

Line 128 in 2886092

func NoCustomMsg(sender sdk.AccAddress, msg json.RawMessage) ([]sdk.Msg, error) {

case:

1. msg has bankmsg and ibcmsg
2. when sdkhandler deal with bankmsg got ErrunknownMsg,it will continue in for _, h := range m.handlers
3. Then IBCHandler deal with ibcmsg will success, MessageHandlerChain DispatchMsg will return success.
4. thus the bankmsg may not be execute, but smart contract may be think bankmsg is no error，and then execute other logic。

So I think MessageHandlerChain DispatchMsg is a problem.

Could your please answer my question?

[ethanfrey]

msg has bankmsg and ibcmsg

A message is one type? How can it be both? We loop through this function once for BankMsg, later for IbcMsg. I don't understand with a message being both at once?

[BananaLF]

A message is one type? How can it be both? We loop through this function once for BankMsg, later for IbcMsg. I don't understand with a message being both at once?

Then wasmvmtypes.CosmosMsg struct is fllow

type CosmosMsg struct {
	Bank         *BankMsg         `json:"bank,omitempty"`
	Custom       json.RawMessage  `json:"custom,omitempty"`
	Distribution *DistributionMsg `json:"distribution,omitempty"`
	Gov          *GovMsg          `json:"gov,omitempty"`
	IBC          *IBCMsg          `json:"ibc,omitempty"`
	Staking      *StakingMsg      `json:"staking,omitempty"`
	Stargate     *StargateMsg     `json:"stargate,omitempty"`
	Wasm         *WasmMsg         `json:"wasm,omitempty"`
}

When msg.BankMsg and msg.IBCMsg is not nil, and dispatch this msg, if SDKMessageHandler handle msg.BankMsg and return ErrUnknownMsg , after IBCRawPacketHandler handle msg.IBCMsg return success.

In this case I think SDKMessageHandler handle msg.BankMsg can not return ErrUnknownMsg, or we can not Ignore this case.

[BananaLF]

I write a unit test to prove this case. I simluate msg which have BankMsg and IBCMsg, self-define mockBankMsgHandler for handling msg.BankMsg is always return ErrUnknownMsg, mockIBCMsgHandler for handing msg.IBCMsg.

func TestMessageHandlerChainDispatch_temp(t *testing.T) {
	mockBankMsgHandler := &wasmtesting.MockMessageHandler{
		DispatchMsgFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) {
			if msg.Bank != nil {
				return nil, nil, types.ErrUnknownMsg
			} else {
				return nil, nil, sdkerrors.Wrap(types.ErrUnknownMsg, "bank is known")
			}

		},
	}
	mockIBCMsgHandler := &wasmtesting.MockMessageHandler{
		DispatchMsgFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) {
			if msg.IBC != nil {
				return []sdk.Event{sdk.NewEvent("IBC", sdk.NewAttribute("handler ibc", "success"))}, [][]byte{{1}}, nil
			} else {
				return nil, nil, types.ErrUnknownMsg
			}
		},
	}

	myMsg := wasmvmtypes.CosmosMsg{Bank: &wasmvmtypes.BankMsg{Send: nil}, IBC: &wasmvmtypes.IBCMsg{}}
	specs := map[string]struct {
		handlers  []Messenger
		expErr    *sdkerrors.Error
		expEvents []sdk.Event
	}{
		"return error when one unknownMsg and success": {
			handlers: []Messenger{
				mockBankMsgHandler, mockIBCMsgHandler,
			},
			expErr: types.ErrUnknownMsg,
		},
	}
	for name, spec := range specs {
		t.Run(name, func(t *testing.T) {
			// when
			h := MessageHandlerChain{spec.handlers}
			_, _, gotErr := h.DispatchMsg(sdk.Context{}, RandomAccountAddress(t), "anyPort", myMsg)

			// then
			require.True(t, spec.expErr.Is(gotErr), "exp %v but got %#+v", spec.expErr, gotErr)
			if spec.expErr != nil {
				return
			}
		})
	}
}

[ethanfrey]

@BananaLF The underlying JSON type comes from Rust contracts (until today) that use an enum there. This only allows exactly one of those fields to be set. We should add some validation logic in Go that exactly one field is set, if we haven't already. It is supposed to be an enum/union type, not some random set of items.

If we fix that validation step, then the rest of your issue goes away, right?

[BananaLF]

@ethanfrey Right !
If CosmosMsg only allows one of those fields to be set, I have no problem. My Problem is not exist。 If you add some validation may be much better. Thanks.

[alpe]

Thanks for reporting this with a test to run! Very much appreciated! ❤️

I think there are only some wrong expectations related to the ErrUnknownMsg type.
Technically we use this error type only to return that a handler (function) can not deal with this message content or type and let the next handler in the chain deal with it. Therefore it is some "reserved" error, that we use for flow control.

So why is your test failing? The mockBankMsgHandler returns a ErrUnknownMsg which triggers the next handler in chain to process the message, which is mockIBCMsgHandler. As msg.IBC != nil is true, no error is returns in the end.

Even in the case where we have multiple message fields in the wasmvmtypes.CosmosMsg object filled, the switch in the encoder will process only one and not multiple messages. A switch is processed top down in Go (no fallthrough defined).

To be completely on the safe side for a great dev UX, we can ensure that wasmvmtypes.CosmosMsg has only 1 message field filled.
@ethanfrey do you think this can go into wasmvm ? IMHO we can depriotize this for a future milestone

[ethanfrey]

To be completely on the safe side for a great dev UX, we can ensure that wasmvmtypes.CosmosMsg has only 1 message field filled.
@ethanfrey do you think this can go into wasmvm ? IMHO we can depriotize this for a future milestone

Yes, you are right, this can go into validation logic in wasmvm and in the next release. Not critical, but a nice to have.

I created CosmWasm/wasmvm#349 and will close this issue. You an track progress there.