Refactor events #586
Refactor events #586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #586 +/- ##
==========================================
+ Coverage 59.74% 59.89% +0.15%
==========================================
Files 45 45
Lines 5212 5197 -15
==========================================
- Hits 3114 3113 -1
+ Misses 1873 1861 -12
+ Partials 225 223 -2
|
Uh oh... is this broken in cosmwasm-plus??? and I just tagged. I will fix that |
There was a problem hiding this comment.
Nice start.
Most of my comments are about ordering.
The other issue is it seems the message events aren't all stripped (or only stripped at the msg_server level). We need to strip them inside the keeper, especially before passing the events into reply, so we don't expose it.
I just saw one test case below with 2 message type events and was quite surprised these were not already removed.
x/wasm/keeper/keeper.go
Outdated
| @@ -299,6 +311,11 @@ func (k Keeper) instantiate(ctx sdk.Context, codeID uint64, creator, admin sdk.A | |||
| return nil, nil, sdkerrors.Wrap(err, "dispatch") | |||
| } | |||
|
|
|||
| ctx.EventManager().EmitEvent(sdk.NewEvent( | |||
There was a problem hiding this comment.
Please emit this before calling k.wasmVM.Instantiate.
We have all needed info there, and this is the ordering from the spec.
|
I fixed the ibc reflect contract in CosmWasm/cosmwasm#1046 and copied it here so the test now works. With my review comments above, this is ready to merge in my eyes. You can also download the "official" contract builds here: https://github.com/CosmWasm/cosmwasm/releases/tag/v0.16.0%2Bcontracts (not just the one I built locally) |
| case err != nil: | ||
| return nil, sdkerrors.Wrap(err, "submessages") | ||
| case rsp != nil: | ||
| result = rsp | ||
| } | ||
| // emit non message type events only |
There was a problem hiding this comment.
This is good to clear, but in the spec, we need to clear the messages before we possibly place them inside reply.
I believe you need to do that inside DispatchSubmessages
There was a problem hiding this comment.
TODO: I will raise this as a new issue (and to add tests if it works or not)
| @@ -530,6 +545,12 @@ func TestExecute(t *testing.T) { | |||
| require.NotNil(t, contractAcct) | |||
| assert.Equal(t, sdk.Coins(nil), bankKeeper.GetAllBalances(ctx, contractAcct.GetAddress())) | |||
|
|
|||
| // and events emitted | |||
| require.Len(t, em.Events(), 5) | |||
| expEvt := sdk.NewEvent("execute", | |||
There was a problem hiding this comment.
event 0 is the message and the other ones?
I find it nice to test all the types at least, makes it clear what is (expected to) happen
| expEvt sdk.Events | ||
| }{ | ||
| "all good": { | ||
| rsp: wasmvmtypes.Response{Data: []byte("foo")}, |
There was a problem hiding this comment.
I would add a case where reply returns an attribute or event and ensure those get returned as well (I am sure they are, but if you want to write table tests to cover the use cases...)
Resolves #440
Implements EVENTS.md
Contains:
bankmodule event of typemessagewhen tokens are transferred to a contractgov_contract_resultfor contractexecuteandmigratein the gov proposal handlerLeft to do:
TestIBCReflectContractby a new contract version that reads from the right event