-
Notifications
You must be signed in to change notification settings - Fork 193
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
chore(evm): Augment the Wasm msg handler so that wasm contracts cannot send MsgEthereumTx #2159
base: main
Are you sure you want to change the base?
Changes from 1 commit
5ce2608
3d41225
be62c0a
3b0387a
d64647e
8507e06
bd6e0d5
f44ce1e
837a6a0
ae354d6
e9ab63d
84e4bf6
4cb566c
3b420a3
aec26d0
1962805
4f9d897
6d082cc
4d92d5b
edf959e
e94abc7
e95f809
396afb4
39fdac9
8445c92
b4f3ac8
ff27306
0d26abc
ab913ba
ecfd2fe
4117272
997a946
cfc3bbe
b775d16
a8dd8a6
deb378f
eb47f0a
d871c0d
755a030
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,25 @@ | ||
package wasmext | ||
|
||
import ( | ||
"github.com/NibiruChain/nibiru/v2/x/evm" | ||
|
||
"cosmossdk.io/errors" | ||
wasmkeeper "github.com/CosmWasm/wasmd/x/wasm/keeper" | ||
wasm "github.com/CosmWasm/wasmd/x/wasm/types" | ||
wasmvmtypes "github.com/CosmWasm/wasmvm/types" | ||
"github.com/cosmos/cosmos-sdk/baseapp" | ||
"github.com/cosmos/cosmos-sdk/codec" | ||
sdkcodec "github.com/cosmos/cosmos-sdk/codec/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
) | ||
|
||
// NibiruWasmOptions: Wasm Options are extension points to instantiate the Wasm | ||
// keeper with non-default values | ||
func NibiruWasmOptions( | ||
grpcQueryRouter *baseapp.GRPCQueryRouter, | ||
appCodec codec.Codec, | ||
msgHandlerArgs MsgHandlerArgs, | ||
) []wasmkeeper.Option { | ||
wasmQueryOption := wasmkeeper.WithQueryPlugins(&wasmkeeper.QueryPlugins{ | ||
Stargate: wasmkeeper.AcceptListStargateQuerier( | ||
|
@@ -20,5 +29,110 @@ | |
), | ||
}) | ||
|
||
return []wasmkeeper.Option{wasmQueryOption} | ||
wasmMsgHandlerOption := wasmkeeper.WithMessageHandler(WasmMessageHandler(msgHandlerArgs)) | ||
|
||
return []wasmkeeper.Option{ | ||
wasmQueryOption, | ||
wasmMsgHandlerOption, | ||
} | ||
} | ||
|
||
func (h SDKMessageHandler) handleSdkMessage(ctx sdk.Context, contractAddr sdk.Address, msg sdk.Msg) (*sdk.Result, error) { | ||
if err := msg.ValidateBasic(); err != nil { | ||
return nil, err | ||
} | ||
|
||
// make sure this account can send it | ||
for _, acct := range msg.GetSigners() { | ||
if !acct.Equals(contractAddr) { | ||
return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "contract doesn't have permission") | ||
} | ||
} | ||
|
||
msgTypeUrl := sdk.MsgTypeURL(msg) | ||
if msgTypeUrl == sdk.MsgTypeURL(new(evm.MsgEthereumTx)) { | ||
return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "Wasm VM to EVM call pattern is not yet supported") | ||
} | ||
|
||
// find the handler and execute it | ||
if handler := h.router.Handler(msg); handler != nil { | ||
// ADR 031 request type routing | ||
msgResult, err := handler(ctx, msg) | ||
return msgResult, err | ||
} | ||
Comment on lines
+58
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add test coverage for message routing logic The message handling within 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 58-62: app/wasmext/wasm.go#L58-L62 |
||
// legacy sdk.Msg routing | ||
// Assuming that the app developer has migrated all their Msgs to | ||
// proto messages and has registered all `Msg services`, then this | ||
// path should never be called, because all those Msgs should be | ||
// registered within the `msgServiceRouter` already. | ||
return nil, errors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add test for unknown message types The error path when a message cannot be routed (unknown request) is not covered by tests. Include a test case to ensure that appropriate errors are returned for unrecognized messages. 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 68-68: app/wasmext/wasm.go#L68 |
||
} | ||
|
||
type MsgHandlerArgs struct { | ||
Router MessageRouter | ||
Ics4Wrapper wasm.ICS4Wrapper | ||
ChannelKeeper wasm.ChannelKeeper | ||
CapabilityKeeper wasm.CapabilityKeeper | ||
BankKeeper wasm.Burner | ||
Unpacker sdkcodec.AnyUnpacker | ||
PortSource wasm.ICS20TransferPortSource | ||
} | ||
|
||
// SDKMessageHandler can handles messages that can be encoded into sdk.Message types and routed. | ||
type SDKMessageHandler struct { | ||
router MessageRouter | ||
encoders msgEncoder | ||
} | ||
|
||
// MessageRouter ADR 031 request type routing | ||
type MessageRouter interface { | ||
Handler(msg sdk.Msg) baseapp.MsgServiceHandler | ||
} | ||
|
||
// msgEncoder is an extension point to customize encodings | ||
type msgEncoder interface { | ||
// Encode converts wasmvm message to n cosmos message types | ||
Encode(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) ([]sdk.Msg, error) | ||
} | ||
|
||
// WasmMessageHandler is a replacement constructor for | ||
// [wasmkeeper.NewDefaultMessageHandler] inside of [wasmkeeper.NewKeeper]. | ||
func WasmMessageHandler( | ||
args MsgHandlerArgs, | ||
) wasmkeeper.Messenger { | ||
encoders := wasmkeeper.DefaultEncoders(args.Unpacker, args.PortSource) | ||
return wasmkeeper.NewMessageHandlerChain( | ||
NewSDKMessageHandler(args.Router, encoders), | ||
wasmkeeper.NewIBCRawPacketHandler(args.Ics4Wrapper, args.ChannelKeeper, args.CapabilityKeeper), | ||
wasmkeeper.NewBurnCoinMessageHandler(args.BankKeeper), | ||
) | ||
} | ||
|
||
func NewSDKMessageHandler(router MessageRouter, encoders msgEncoder) SDKMessageHandler { | ||
return SDKMessageHandler{ | ||
router: router, | ||
encoders: encoders, | ||
} | ||
} | ||
|
||
func (h SDKMessageHandler) DispatchMsg(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) { | ||
sdkMsgs, err := h.encoders.Encode(ctx, contractAddr, contractIBCPortID, msg) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
for _, sdkMsg := range sdkMsgs { | ||
res, err := h.handleSdkMessage(ctx, contractAddr, sdkMsg) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
// append data | ||
data = append(data, res.Data) | ||
// append events | ||
sdkEvents := make([]sdk.Event, len(res.Events)) | ||
for i := range res.Events { | ||
sdkEvents[i] = sdk.Event(res.Events[i]) | ||
} | ||
events = append(events, sdkEvents...) | ||
Comment on lines
+129
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add test coverage for event and data aggregation The code segment that appends 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 129-135: app/wasmext/wasm.go#L129-L135 |
||
} | ||
return | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
package wasmext_test | ||
|
||
import ( | ||
"math/big" | ||
"testing" | ||
|
||
wasmvm "github.com/CosmWasm/wasmvm/types" | ||
sdkcodec "github.com/cosmos/cosmos-sdk/codec/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/stretchr/testify/suite" | ||
|
||
"github.com/NibiruChain/nibiru/v2/app/wasmext" | ||
"github.com/NibiruChain/nibiru/v2/x/evm" | ||
"github.com/NibiruChain/nibiru/v2/x/evm/evmtest" | ||
) | ||
|
||
type Suite struct { | ||
suite.Suite | ||
} | ||
|
||
func TestWasmExtSuite(t *testing.T) { | ||
suite.Run(t, new(Suite)) | ||
} | ||
|
||
// WasmVM to EVM call pattern is not yet supported. This test verifies the | ||
// Nibiru's [wasmkeeper.Option] function as expected. | ||
func (s *Suite) TestEvmFilter() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add here some happy paths too to ensure the functionality not regressed? |
||
deps := evmtest.NewTestDeps() | ||
// wk := wasmkeeper.NewDefaultPermissionKeeper(deps.App.WasmKeeper) | ||
wasmMsgHandler := wasmext.WasmMessageHandler(deps.App.WasmMsgHandlerArgs) | ||
|
||
s.T().Log("Create a valid Ethereum tx msg") | ||
|
||
to := evmtest.NewEthPrivAcc() | ||
ethTxMsg, err := evmtest.TxTransferWei{ | ||
Deps: &deps, | ||
To: to.EthAddr, | ||
AmountWei: evm.NativeToWei(big.NewInt(420)), | ||
}.Build() | ||
s.NoError(err) | ||
|
||
s.T().Log("Validate Eth tx msg proto encoding as wasmvm.StargateMsg") | ||
wasmContractAddr := deps.Sender.NibiruAddr | ||
protoValueBz, err := deps.EncCfg.Codec.Marshal(ethTxMsg) | ||
s.Require().NoError(err, "expect ethTxMsg to proto marshal", protoValueBz) | ||
|
||
_, ok := deps.EncCfg.Codec.(sdkcodec.AnyUnpacker) | ||
s.Require().True(ok, "codec must be an AnyUnpacker") | ||
|
||
pbAny, err := sdkcodec.NewAnyWithValue(ethTxMsg) | ||
s.NoError(err) | ||
pbAnyBz, err := pbAny.Marshal() | ||
s.NoError(err, pbAnyBz) | ||
|
||
var sdkMsg sdk.Msg | ||
err = deps.EncCfg.Codec.UnpackAny(pbAny, &sdkMsg) | ||
s.Require().NoError(err) | ||
s.Equal("/eth.evm.v1.MsgEthereumTx", sdk.MsgTypeURL(sdkMsg)) | ||
|
||
s.T().Log("Dispatch the Eth tx msg from Wasm (unsuccessfully)") | ||
_, _, err = wasmMsgHandler.DispatchMsg( | ||
deps.Ctx, | ||
wasmContractAddr, | ||
"ibcport-unused", | ||
wasmvm.CosmosMsg{ | ||
Stargate: &wasmvm.StargateMsg{ | ||
TypeURL: sdk.MsgTypeURL(ethTxMsg), | ||
Value: protoValueBz, | ||
}, | ||
}, | ||
) | ||
s.Require().ErrorContains(err, "Wasm VM to EVM call pattern is not yet supported") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add test coverage for validation error handling
The error handling when
msg.ValidateBasic()
fails in thehandleSdkMessage
function is not covered by tests. Consider adding a test case to ensure that messages failing basic validation are properly rejected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-43: app/wasmext/wasm.go#L42-L43
Added lines #L42 - L43 were not covered by tests