-
Notifications
You must be signed in to change notification settings - Fork 628
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
feat: add mockVM for wasm clients testing #4804
Conversation
// MockWasmEngine implements types.WasmEngine for testing purpose. One or multiple messages can be stubbed. | ||
// Without a stub function a panic is thrown. | ||
type MockWasmEngine struct { |
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.
https://github.com/CosmWasm/wasmd/blob/main/x/wasm/keeper/wasmtesting/mock_engine.go#L19, also very similar to how we mock ibc apps
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.
maybe we can add this link in the code? Will be nice as an easy reference for making changes in the future.
@@ -281,6 +281,7 @@ func NewSimApp( | |||
traceStore io.Writer, | |||
loadLatest bool, | |||
appOpts servertypes.AppOptions, | |||
mockVM wasmtypes.WasmEngine, |
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.
quick hack, didn't want to spend so much time on this
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.
We could maybe just add an additional constructor NewSimAppWithVm
(also no ideal and would be reworked later) but I'm fine with just adding this as is for now.
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.
Will leave as is for now, I figure once we get time to reduce the redundant simapps we will solve this issue, but also happy to see a followup with a nicer solution
return m.StoreCodeFn(codeID) | ||
} | ||
|
||
func (m *MockWasmEngine) Instantiate(codeID wasmvm.Checksum, env wasmvmtypes.Env, info wasmvmtypes.MessageInfo, initMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error) { |
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.
Some of these funcs are only expected to perform a single action (initialize the contract or not), so I think a simple override of functionality makes sense for those
return m.InstantiateFn(codeID, env, info, initMsg, store, goapi, querier, gasMeter, gasLimit, deserCost) | ||
} | ||
|
||
func (m *MockWasmEngine) Query(codeID wasmvm.Checksum, env wasmvmtypes.Env, queryMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) ([]byte, uint64, error) { |
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.
Other funcs like query are expected to handle a subset of queries, so we may actually want to make the subset actions overrideable, not the entire query fn itself
I could remove the QueryFn field and replace with overrides for each subset action, then the QueryFn would use code magic to determine what the request is and then delegate to the appropriate response handler?
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.
could we utilize amap[someKey]QueryFn
to handle different message types?
Maybe expose a mockVm.OverrideQueryFn(key, func(){...})
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.
Happy to leave as is for the initial run and see if we can make improvements later 👍
// CreateClient creates an IBC client on the endpoint. It will update the | ||
// clientID for the endpoint if the message is successfully executed. | ||
// NOTE: a solo machine client will be created with an empty diversifier. | ||
func (endpoint *WasmEndpoint) CreateClient() (err error) { |
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.
took the quick route. I think the testing pkg will need to define either an interface for the Endpoint type or an interface that the endpoint needs to construct messages (such as a QueryProof interface fn). I think this will be easier to do reason about after we start the testing pkg improvement issues, so for now, I thought it was easier to duplicate the code a bit by implementing the fn's that require wasm client specific logic
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.
opened #4807 and #4808 for the direction I think makes the most sense. For now, I think we should move forward with continuing to create this override type. Essentially the WasmEndpoint is a mock cometbft chain with a wasm client on it, and the endpoint represents that client and its connection/channel.
Once we implement interfaces for the light client module to plug into the endpoint, we could implement that interface and remove this type
contractClientState = []byte{1} | ||
contractConsensusState = []byte{2} |
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.
can be changed, just wrote something down for them
clientState.LatestHeight = latestHeight | ||
path.EndpointA.SetClientState(clientState) | ||
suite.mockVM.QueryFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, queryMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) ([]byte, uint64, error) { | ||
resp := fmt.Sprintf(`{"status":"%s"}`, exported.Expired) |
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.
can also do something like:
statusResult := new(types.StatusResult)
statusResult.Status = exported.Active
resp, err := json.Marshal(&statusResult)
suite.Require().NoError(err)
I just chose something
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 actually thing the one liner you have is super readable. I think the other approach you suggested would be preferable for larger types.
That, or pass an fn that acts on the type and handle the marshalling in separately.
|
||
func (suite *TypesTestSuite) setupWasmWithMockVM() (ibctesting.TestingApp, map[string]json.RawMessage) { | ||
suite.mockVM = &wasmtesting.MockWasmEngine{} | ||
// TODO: move default functionality required for wasm client testing to the mock VM |
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 think it'd be easier to address this in a followup when we better define how the mock VM will be used
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.
aye, I'll be adding some funcs here since we'd need to define ones for storing code and pinning (relevant to issue #4849). I'll open another issue to track this TODO. issue was already opened!
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.
It's looking good. Thank you, @colin-axner!
I just left one minor comment for now. Looking forward to seeing the progress on this one!
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…in/4798-mock-wasmvm
// CreateClient creates an IBC client on the endpoint. It will update the | ||
// clientID for the endpoint if the message is successfully executed. | ||
// NOTE: a solo machine client will be created with an empty diversifier. | ||
func (endpoint *WasmEndpoint) CreateClient() (err error) { |
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.
opened #4807 and #4808 for the direction I think makes the most sense. For now, I think we should move forward with continuing to create this override type. Essentially the WasmEndpoint is a mock cometbft chain with a wasm client on it, and the endpoint represents that client and its connection/channel.
Once we implement interfaces for the light client module to plug into the endpoint, we could implement that interface and remove this type
if types.WasmVM != nil && !reflect.DeepEqual(types.WasmVM, vm) { | ||
panic(errors.New("global Wasm VM instance should not be set to a different instance")) | ||
} |
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 check doesn't work with mock vm when you have other testing also uses the wasmvm, so I decided to remove it, especially since we are going to manage global assignment internally
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.
TBH I like this quite a bit, even though we're still dealing with global variables, removing the scope at which we assign them can never be a bad thing.
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.
Overall I really like the direction here. The function override pattern aligns well with the mock module we use in our other tests.
I left a few suggestions, but I think one of the bigger ones would be that maybe we actually can have default functionality baked into the mock vm, and replace it only if an override function is defined.
I'm happy with how it is now though, happy to make changes in a follow up.
if types.WasmVM != nil && !reflect.DeepEqual(types.WasmVM, vm) { | ||
panic(errors.New("global Wasm VM instance should not be set to a different instance")) | ||
} |
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.
TBH I like this quite a bit, even though we're still dealing with global variables, removing the scope at which we assign them can never be a bad thing.
app := simapp.NewSimApp(log.NewNopLogger(), db, nil, true, simtestutil.EmptyAppOptions{}, nil) | ||
return app, simapp.NewDefaultGenesisState(encCdc.Codec) |
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.
we'll run into some conflicts here with the sync to main. I guess we can hold off on this PR until after that is in.
// MockWasmEngine implements types.WasmEngine for testing purpose. One or multiple messages can be stubbed. | ||
// Without a stub function a panic is thrown. | ||
type MockWasmEngine struct { |
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.
maybe we can add this link in the code? Will be nice as an easy reference for making changes in the future.
return m.InstantiateFn(codeID, env, info, initMsg, store, goapi, querier, gasMeter, gasLimit, deserCost) | ||
} | ||
|
||
func (m *MockWasmEngine) Query(codeID wasmvm.Checksum, env wasmvmtypes.Env, queryMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) ([]byte, uint64, error) { |
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.
could we utilize amap[someKey]QueryFn
to handle different message types?
Maybe expose a mockVm.OverrideQueryFn(key, func(){...})
if m.PinFn == nil { | ||
panic("mock engine is not properly initialized") | ||
} |
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 wonder maybe some of these fns can have defualts, for Pin
I think in all of our tests we'd be happy with a no-op.
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 think so, I decided to hold off for now until we run into the reason they need a default. I attempted to bring in the existing default functionality for initialize and status to the mock engine, but it requires exposing access to a cdc and other fields the suite has, so I decided it was better to handle that later when we have more of the testing pattern defined
{ | ||
"client is frozen", | ||
func() { | ||
suite.mockVM.QueryFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, queryMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) ([]byte, uint64, error) { |
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 is super nice, one thing that might be useful for readability is a function alias, but editors will auto fill this stuff for you so I don't feel super strongly about it.
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 don't feel strongly either, so I'll let it be a followup if we so chose
clientState.LatestHeight = latestHeight | ||
path.EndpointA.SetClientState(clientState) | ||
suite.mockVM.QueryFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, queryMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) ([]byte, uint64, error) { | ||
resp := fmt.Sprintf(`{"status":"%s"}`, exported.Expired) |
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 actually thing the one liner you have is super readable. I think the other approach you suggested would be preferable for larger types.
That, or pass an fn that acts on the type and handle the marshalling in separately.
|
||
// clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID) | ||
// clientState = path.EndpointA.GetClientState().(*types.ClientState) | ||
endpoint := &wasmtesting.WasmEndpoint{Endpoint: ibctesting.NewDefaultEndpoint(suite.chainA)} |
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.
worth wrapping this in a wasmtesting.NewDefaultEndpoint(suite.chainA)
? which would call ibctesting.NewDefaultEndpoint(suite.chainA)
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 can add a wasmtesting.NewWasmEndpoint
constructor to perform this
func (chain *TestChain) SetWasm(wasm bool) *TestChain { | ||
chain.UseWasmClient = wasm | ||
return chain | ||
} |
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.
🥳
…in/4798-mock-wasmvm
Co-authored-by: Cian Hatton <cian@interchain.io>
…o colin/4798-mock-wasmvm
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.
Super nice! Awesome job, thanks for getting this together. I feel happy enough about moving forward with tests which leverage this!
🚀 🚀 🚀
return m.InstantiateFn(codeID, env, info, initMsg, store, goapi, querier, gasMeter, gasLimit, deserCost) | ||
} | ||
|
||
func (m *MockWasmEngine) Query(codeID wasmvm.Checksum, env wasmvmtypes.Env, queryMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) ([]byte, uint64, error) { |
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.
Happy to leave as is for the initial run and see if we can make improvements later 👍
type WasmEndpoint struct { | ||
*ibctesting.Endpoint | ||
} |
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.
Big +
mega nit: should we add a basic godoc?
Description
Initial work on bringing a mock VM. Likely should be merged after:
As this pr should likely handle any extra logic required after those changes
closes: #4798
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.