Rethink Extern

[ethanfrey]

Problem

In order to pass in all the external deps, we currently use:

pub struct Extern<S: Storage, A: Api, Q: Querier> {
    pub storage: S,
    pub api: A,
    pub querier: Q,
}

and pass in either &mut Extern (handle, init, migrate) or &Extern (query).

This is great for tests and for linking to relatively stateless handlers. Now, I am working to build a integration test framework for contracts to redeploy messages and router querier to the real contracts and their state - held by the same object that is running the query itself.

The current definition means I need to pass in self, when all that is really needed is &self. I start defining impl Querier for &Router (rather than Router) and got into all kinds of lifetime issues. I also could not accept &Q, as I need Q for building Extern.

Proposal

What we really need inside the contracts is:

pub struct ExternMut<S: Storage, A: Api, Q: Querier> {
    pub storage: &mut S,
    pub api: &A,
    pub querier: &Q,
}

pub struct ExternRef<S: Storage, A: Api, Q: Querier> {
    pub storage: &S,
    pub api: &A,
    pub querier: &Q,
}

and can use handle(deps: ExternMut<S, A, Q>, ..) and query(deps: ExternRef<S, A, Q>, ...)

Impact

We could easily implement Extern::as_ref() and Extern::as_mut() to produce these from one owned Extern. This would be used almost identically inside the contract code (probably less typing - deps.storage rather than &mut deps.storage), but would make a bit more typing in the test code (mock_dependencies must return Extern for ownership, then all calls would be like: handle(deps.as_mut(), ...)

I do believe this would not change the wasm<->Rust API, just the unit tests, so pre-compiled 0.11 wasm binaries would be drop-in compatible with a new network with this change. You could just update the code in the next version of each contract, without requiring an upgrade of every contract to use the new network.

[ethanfrey]

Please see CosmWasm/cw-plus#130 for Context. In particular, the Router implementation from line 285 on. (also revert one commit to see me attempt everything with references and the lifetime tunnel).

ContractWrapper and WasmRouter should be more or less complete (untested but functionally possible)

[webmaster128]

This is what a unit test would look like if we gave of the grouping:

diff --git a/contracts/hackatom/src/contract.rs b/contracts/hackatom/src/contract.rs
index b0edd0ad..3b68290d 100644
--- a/contracts/hackatom/src/contract.rs
+++ b/contracts/hackatom/src/contract.rs
@@ -468,7 +468,7 @@ mod tests {
 
     #[test]
     fn handle_release_works() {
-        let mut deps = mock_dependencies(&[]);
+        let (mut storage, api, querier) = mock_dependencies(&[]);
 
         // initialize the store
         let creator = HumanAddr::from("creator");
@@ -481,15 +481,31 @@ mod tests {
         };
         let init_amount = coins(1000, "earth");
         let init_info = mock_info(creator.as_str(), &init_amount);
-        let init_res = init(&mut deps, mock_env(), init_info, init_msg).unwrap();
+        let init_res = init(
+            &mut storage,
+            &api,
+            &querier,
+            mock_env(),
+            init_info,
+            init_msg,
+        )
+        .unwrap();
         assert_eq!(init_res.messages.len(), 0);
 
         // balance changed in init
-        deps.querier.update_balance(MOCK_CONTRACT_ADDR, init_amount);
+        querier.update_balance(MOCK_CONTRACT_ADDR, init_amount);
 
         // beneficiary can release it
         let handle_info = mock_info(verifier.as_str(), &[]);
-        let handle_res = handle(&mut deps, mock_env(), handle_info, HandleMsg::Release {}).unwrap();
+        let handle_res = handle(
+            &mut storage,
+            &api,
+            &querier,
+            mock_env(),
+            handle_info,
+            HandleMsg::Release {},
+        )
+        .unwrap();
         assert_eq!(handle_res.messages.len(), 1);
         let msg = handle_res.messages.get(0).expect("no message");
         assert_eq!(

[ethanfrey]

A bit more ugly. This is the proper types we wish for however, not claiming ownership.

[ethanfrey]

Please look at this for a much cleaner version using the ExternRef and ExternMut types I proposed above: https://github.com/CosmWasm/cosmwasm/pull/586/files

It has the exact same ownership rules as the code you wrote, but is simpler to use. You don't even need to change mock_dependencies, just the usage slightly, like this:

         let info = mock_info(creator.as_str(), &[]);
-        let res = init(&mut deps, mock_env(), info, msg).unwrap();
+        let res = init(deps.as_mut(), mock_env(), info, msg).unwrap();
         assert_eq!(0, res.messages.len());
 
         // check it is 'verifies'
-        let query_response = query(&deps, mock_env(), QueryMsg::Verifier {}).unwrap();
+        let query_response = query(deps.as_ref(), mock_env(), QueryMsg::Verifier {}).unwrap();
         assert_eq!(query_response.as_slice(), b"{\"verifier\":\"verifies\"}");

[webmaster128]

Okay, so those two are basically the same thing. I don't like adding even more symbols, but if this is the price we have to pay, then fine.

But I wonder how this solves even the most basic problem that given an &mut S and a &Q, the querier cannot access the storage because you cannot have a mutable and an immutable reference at the same time.

[ethanfrey]

I solved that with RefCell.

You cannot have a mutable and immutable reference to the same storage at the same time. But if each contract has their own storage, you can have a mutable reference to your storage and imutible reference to all other storage at the same time using RefCell, which allows you to get with &HashMap and each item enforces exclusive &mut or multiple & at runtime. See cell for more info

The querier only has to grant imutible access to other contracts. If it returns an error attempting to self-query while inside a handle function, I think that is fine. Actually good - it was one of the issues that worried me about possible re-entrancy in queries and I proposed blocking that self-recursion explicitly in one early design.

[webmaster128]

Alright, then. Seeing a bit clearer with #589. I'm still not happy we need 3 versions of the Deps struct but also don't see an alternative to it.

[ethanfrey]

I mocked out some ExternMut and ExternRef types in CosmWasm/cw-plus#130 to see if it would compile without errors and Rust approved of all functionality. And it looks good so far.

This code is not tested, but the lifetimes and reference counting makes the compiler happy, which is a good start for the API design.