-
Notifications
You must be signed in to change notification settings - Fork 336
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
Add support for configuring Wasm handling to MockQuerier #1137
Conversation
1585bc1
to
5865049
Compare
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 have experience in this part of code, but code itself looks solid and functionality is backed by tests.
My only "complain" is to a very long UT, which handles all the cases at once (so it's not a really UNIT test).
Otherwise, lgtm.
#[test] | ||
fn wasm_querier_works() { | ||
let mut querier = WasmQuerier::default(); |
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 a very long test for a UT. Consider splitting 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.
This is a good first step to allowing some customizability.
In the issue we discuss ways to make this easier... standard implementations for handling RawQuery for example. But that can build on this original implementation as opt-in. Maybe even in cw-plus if we want. Just allowing an easy way to plug in a query handler is a great addition for extensibility and maybe all we need in the standard library.
/// always errors by default. Update it via `with_custom_handler`. | ||
/// | ||
/// Use box to avoid the need of generic type. | ||
handler: Box<dyn for<'a> Fn(&'a WasmQuery) -> QuerierResult>, |
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 allow for a closure with some immutable data, but not mutable data, right?
I guess all mutability needs to happen before storing it in the WasmQuerier.
err => panic!("Unexpected error: {:?}", err), | ||
} | ||
|
||
querier.update_handler(|request| { |
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 the problem with this approach is, this is still a lot of custom code to provide. Just to be able to set and find some values in the mock storage.
Isn't there a better / more generic way to do that for these mocks?
Is there any particular reason why this review is still up? |
ede6132
to
26b4d74
Compare
Thank you |
- cosmwasm-std: `MockQuerier` now supports adding custom behaviour for handling | ||
Wasm queries via `MockQuerier::update_wasm` ([#1050]). | ||
|
||
[#1050]: https://github.com/CosmWasm/cosmwasm/pull/1050 |
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.
Ah, damn, this was added to the already released 1.0.0-beta7 section. Will fix on main branch.
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.
Oh sorry, it's because it's under all other math related stuff I've been adding lately - didn't pay attention to the actual date.
Good catch, couple more things in changelog to shuffle
Closes #1050