-
Notifications
You must be signed in to change notification settings - Fork 410
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
Support self calling contract on instantiation #300
Conversation
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.
Nice abstraction and way to quickly iterate on tests of the go logic.
Added some comments. I think a bit more polish on it and it will be ready to merge.
|
||
// mock to call itself on instantiation | ||
type selfCallingInstMockWasmer struct { | ||
AlwaysFailMockWasmer |
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.
That always failed when you override Instantiate and Execute is a bit confusing. I expect this to be an error, not actually do something. Mayb just provide a Query implementation (or extend NoopMockWasmer with a positive query operation).
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.
The AlwaysFailMockWasmer
implements all the methods to satisfy the WasmEngine interface. I overwrite only 3 methods here for the test so that I can create the proper data. A call to any other method should fail the test.
An always panic wasmer engine implementation would be more accurate though....
I will refactor this.
} | ||
|
||
func (a AlwaysFailMockWasmer) Cleanup() { | ||
panic("implement me") |
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.
Huh? That seems dangerous. Where is Cleanup called, just in test code?
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.
I would make this a no-op, I don't think we ever try to handle panics there.
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.
This would have been called from tests only
Thanks for the feedback. I have revisited the test support types to apply the feedback. |
Codecov Report
@@ Coverage Diff @@
## master #300 +/- ##
==========================================
+ Coverage 17.23% 17.34% +0.11%
==========================================
Files 35 35
Lines 10463 10497 +34
==========================================
+ Hits 1803 1821 +18
- Misses 8563 8577 +14
- Partials 97 99 +2
Continue to review full report at Codecov.
|
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.
Thank you for addressing the feedback.
You can make another round if you find it useful and merge it when you are satisfied. They are just stylistic comments now.
) | ||
keepers.WasmKeeperRef.wasmer = wasmerMock | ||
wasmerMock := &MockWasmer{ |
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.
I like this constructor over extending AlwaysFailMockWasmer
. But it does seem a bit odd to inline this.. Fine as now, as it is only used once, but maybe we can make it reusable for other tests. My rough thought:
func selfCallingInstMockWasmer(executeCalled *bool) *MockWasmer {
return &MockWasmer{
} CreateFn: func(code cosmwasm.WasmCode) (cosmwasm.CodeID, error) {
anyCodeID := bytes.Repeat([]byte{0x1}, 32)
return anyCodeID, nil
},
InstantiateFn: func(code cosmwasm.CodeID, env wasmTypes.Env, info wasmTypes.MessageInfo, initMsg []byte, store cosmwasm.KVStore, goapi cosmwasm.GoAPI, querier cosmwasm.Querier, gasMeter cosmwasm.GasMeter, gasLimit uint64) (*wasmTypes.InitResponse, uint64, error) {
return &wasmTypes.InitResponse{
Messages: []wasmTypes.CosmosMsg{
{Wasm: &wasmTypes.WasmMsg{Execute: &wasmTypes.ExecuteMsg{ContractAddr: env.Contract.Address, Msg: []byte(`{}`)}}},
},
}, 1, nil
},
ExecuteFn: func(code cosmwasm.CodeID, env wasmTypes.Env, info wasmTypes.MessageInfo, executeMsg []byte, store cosmwasm.KVStore, goapi cosmwasm.GoAPI, querier cosmwasm.Querier, gasMeter cosmwasm.GasMeter, gasLimit uint64) (*wasmTypes.HandleResponse, uint64, error) {
excuteCalled = true
return &wasmTypes.HandleResponse{}, 1, nil
},
}
}
And in this test we could do:
executedCalled := false
wasmerMock = selfCallingInstMockWasmer(&executeCalled)
// ...
_, err := keepers.WasmKeeper.Instantiate(ctx, example.CodeID, example.CreatorAddr, nil, nil, "test", nil)
assert.True(t, excuteCalled)
Not a blocker and this can always be refactored later. I like the use of MockWasmer.
Resolves #292
While fixing the issue was trivial testing it was not with the current setup. This patch also introduces a new interface
WasmerEngine
that is implemented bycosmwasm.Wasmer
. This enables us to bypass the wasmer rust engine in tests for more flexibility and a faster feedback cycle.