Skip to content
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

Allow customising WasmQuerier in MockQuerier #1050

Closed
ethanfrey opened this issue Aug 18, 2021 · 15 comments · Fixed by #1137
Closed

Allow customising WasmQuerier in MockQuerier #1050

ethanfrey opened this issue Aug 18, 2021 · 15 comments · Fixed by #1137

Comments

@ethanfrey
Copy link
Member

ethanfrey commented Aug 18, 2021

I wanted to extend a unit test to provide mocks for some wasm queries and found myself copying much code from MockQuerier as I couldn't customise it. See:

    const MEMBERS: Map<&Addr, u64> = Map::new(cw4::MEMBERS_KEY);

    struct GroupQuerier {
        contract: String,
        storage: MockStorage,
    }

    impl GroupQuerier {
        pub fn new(contract: &Addr, members: &[(&Addr, u64)]) -> Self {
            let mut storage = MockStorage::new();
            for (member, weight) in members {
                MEMBERS.save(&mut storage, member, weight).unwrap();
            }
            GroupQuerier {
                contract: contract.to_string(),
                storage,
            }
        }

        fn handle_query(&self, request: QueryRequest<Empty>) -> QuerierResult {
            match request {
                QueryRequest::Wasm(WasmQuery::Raw { contract_addr, key }) => {
                    self.query_wasm(contract_addr, key)
                }
                QueryRequest::Wasm(WasmQuery::Smart { .. }) => {
                    SystemResult::Err(SystemError::UnsupportedRequest {
                        kind: "WasmQuery::Smart".to_string(),
                    })
                }
                _ => SystemResult::Err(SystemError::UnsupportedRequest {
                    kind: "not wasm".to_string(),
                }),
            }
        }

        // TODO: we should be able to add a custom wasm handler to MockQuerier from cosmwasm_std::mock
        fn query_wasm(&self, contract_addr: String, key: Binary) -> QuerierResult {
            if contract_addr != self.contract {
                SystemResult::Err(SystemError::NoSuchContract {
                    addr: contract_addr,
                })
            } else {
                let bin = self.storage.get(&key).unwrap_or_default();
                SystemResult::Ok(ContractResult::Ok(bin.into()))
            }
        }
    }

    impl Querier for GroupQuerier {
        fn raw_query(&self, bin_request: &[u8]) -> QuerierResult {
            let request: QueryRequest<Empty> = match from_slice(bin_request) {
                Ok(v) => v,
                Err(e) => {
                    return SystemResult::Err(SystemError::InvalidRequest {
                        error: format!("Parsing query request: {}", e),
                        request: bin_request.into(),
                    })
                }
            };
            self.handle_query(request)
        }
    }

when I just would like to implement new and query_wasm

This is commented twice is cosmwasm_std as a TODO/FIXME:

struct NoWasmQuerier {
// FIXME: actually provide a way to call out
}
and
/// TODO: also allow querying contracts
pub struct MockQuerier<C: DeserializeOwned = Empty> {
bank: BankQuerier,
#[cfg(feature = "staking")]
staking: StakingQuerier,
// placeholder to add support later
wasm: NoWasmQuerier,

It would be good to allow someway to customise the wasm handler there to easily mock out contracts.

@abstruse-goose
Copy link

Yes, I also miss the ability to configure the wasm-querier. I wanted to test a contract on Terra. It queries both prices/swap rates both on-chain and on dexes. For now I use a copy of mock.rs, but clearly would prefer a configurable querier.

@ethanfrey
Copy link
Member Author

you want a mock querier for wasm and for custom messages, right?

@maurolacy
Copy link
Contributor

maurolacy commented Oct 15, 2021

I was thinking about this, as yesterday I needed to mock a raw query, and ended up copying and adapting a good chunk of code.

Didn't saw the associated PR (#1137) yet, so excuse me if I say something that's already been done / addressed.

Why not just provide a builder-like pattern for the Querier, in which a series of values (or rather, key/value pairs) can be specified? The querier code is mostly the same. It's just that we want some data to be found when we use the querier for other addresses, by example.

So, we could do something like QuerierBuilder::new().with_map_data(addr, map_namespace, map).with_item_data(addr, items).build().

map and items can be plain old Rust Maps, or vectors of (namespace, value) tuples.

Just an idea. Will take a look at the PR later, and comment back in any case.

@webmaster128
Copy link
Member

webmaster128 commented Oct 15, 2021

Both Ethan's example and @maurolacy's comment focus on raw queries only. The solution in #1137 is more geneeral and handles raw, smart and contract info queries in one handler. Maybe we should allow the user to set one per query type such that the implementation gets shorter.

@ethanfrey
Copy link
Member Author

Yes, I think often we will only want to override one. Allowing to do so simply would be nice.

Once the manner to plug in support is solid, we could add some helpers to do what Mauro says, but I would say something like.

MockQuerier::with_raw_query(raw_querier_from_map(data));

@maurolacy
Copy link
Contributor

maurolacy commented Oct 15, 2021

Nevertheless, it would be nice that, with a simple setup, you could affect all queries consistently. Not sure the associated PR does that. Will take a look at it for sure.

@webmaster128
Copy link
Member

webmaster128 commented Oct 15, 2021

If we split the queries in 3 configurations, we can make them configurable in a different way.

  • The raw querier can be configured with an address -> key -> value map (similar to how the balances are configured). More customization should not be needed there.
  • Smart query probably needs a handler function.
  • Contract info query can be a address -> conftract info map.

@ethanfrey
Copy link
Member Author

The raw querier can be configured with an address -> key -> value map (similar to how the balances are configured). More customization should not be needed there.

Not sure about this one... the use case is to simulate some data from a real contract. Which will come from cw_storage_plus::Item or cw_storage_plus::Map most likely. I am not sure how to get a key->value store from them as a simple args. They are designed to write to some storage.

We could hard-code some raw_querier but allow writing to the storage. eg.

let mut querier = MockQuerier::new();
querier.write_contract(&contract_addr, |storage: &mut dyn Storage| {
    CONFIG.save(storage, &Config{foo: 42}).unwrap();
});

This would set up some data on the RawQuerier handler. Basically chopping out parts of some instantiate functions you should be able to write the mock data here.

@maurolacy
Copy link
Contributor

maurolacy commented Oct 15, 2021

You are right, both keys and values are not just plain strings or byte slices, but structured data. This will require passing something like a Vec<dyn Serializable> and implementing a serialize() or so over the required data. And then writing it in raw form.

Maybe not so complex, but probably not worth the effort for such a limited / specific use case.

What about an initial implementation when keys are strings, and values are byte slices? What about leveraging already existing serializations for keys and values?

@ethanfrey
Copy link
Member Author

What about an initial implementation when keys are strings, and values are byte slices?

This is not the case of most real contract we try to mock

What about leveraging already existing serializations for keys and values?

Yup. That is what my write_contract idea would do. It manages a map of MockStorage under the hood.

I think all of these could be implemented in contracts and when standardised could come here. For now just allowing registering mocks on those 3 entry points separately would be good. And maybe a simple implementation to mock ContractInfo query with a HashMap

@maurolacy
Copy link
Contributor

maurolacy commented Jan 6, 2022

I was thinking about this, and here's another idea: why not providing a MockStorage extension to simply write raw values?

Something like write_raw or save_raw, that allows you to write a (binary / serialized) value under an entire address + namespace + key.

That, and then modifying / extending MockQuerier to also work with those values directly, i.e. without requiring a specific handler function / closure.

We just write the raw values we need in the mock storage, and then the mock querier works transparently over them.

And we complement that by providing some helpers / examples / tests on how to properly build those raw keys (along the lines of namespace_with_key, joined_key, and their combinations).

Is this feasible? Is it a good idea?

@ethanfrey
Copy link
Member Author

Hmm... so we do something like

const CONFIG: Item<Config> = Item::new("config");
let mut contract_space = deps.storage.namespaced(contract_addr); 
CONFIG.save(&mut contract_space, &config).unwrap();

If you make the namespacing easy like that and it provides a dyn Storage implementation that works with Item and Map, this could be a nice alternative

@maurolacy
Copy link
Contributor

maurolacy commented Jan 8, 2022

I was thinking more along the lines of

deps.storage.save_raw(namespaced(contract_addr, "config"), &config);

The problem here is config is typed. Perhaps

deps.storage.save_raw(namespaced(contract_addr, "config"), to_binary(config));

or similar. But whatever works / is more or less intuitive / is simple to use.

If we make this work seamlessly over MockQuerier, then that's all we need.

@ethanfrey
Copy link
Member Author

If it does not work with Item and Map, it will be tricky to use and ensure it works like the original contract.

I would provide to directly use the state.rs items from the original contract. Maybe even some of the lower level logic functions could be called to set this properly.

I assume the goal is to make setup of a mock state for an external contract easy to do.
But then I ask... how often do we do QueryRaw and when do we do QuerySmart?
This will only help with the first case, correct? I am fine with doing this as well and make it easy to reuse helpers, but it won't solve all the mocking we try to do.

@maurolacy
Copy link
Contributor

maurolacy commented Jan 9, 2022

If it does not work with Item and Map, it will be tricky to use and ensure it works like the original contract.

I agree using the Map and Item containers could be better, but still, this is simple enough IMO. Map is just another namespace element:

deps.storage.save_raw(namespaced_map(contract_addr, "map_name", "map_key"), to_binary(map_value));

I would provide to directly use the state.rs items from the original contract. Maybe even some of the lower level logic functions could be called to set this properly.

I assume the goal is to make setup of a mock state for an external contract easy to do. But then I ask... how often do we do QueryRaw and when do we do QuerySmart? This will only help with the first case, correct?

Unless we can build something like a global map of storage mocks, and honour it across all query functions, this would only work for raw queries, yes. Need to take a more detailed look at the mock smart query impl, but, what prevents doing that global map?

I am fine with doing this as well and make it easy to reuse helpers, but it won't solve all the mocking we try to do.

Agreed, seems this will solve just part of the problem. For smart queries, perhaps what @hashedone suggests, of using them only in the context of multi tests, is the way to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants