[trusted-token] Fix multi-tests with `TgradeApp`

[maurolacy]

This is WIP for making trusted-token multitests work with TgradeApp.

Requires some local setup, as we moved the code away from this variant already:

• Required poe-contracts branch: https://github.com/confio/poe-contracts/tree/tgradeapp-querier-0.6.1
• Required cw-plus branch: https://github.com/CosmWasm/cw-plus/tree/multi-test-generic-deps-0.11.1

These basically have the fixes applied over the versions used by the original tfi code in this branch.

So, checkout all three, and then put this into the tfi workspace's Cargo.toml:

[patch.crates-io]
tg4 = { path = "../poe-contracts/packages/tg4" }
tg4-group = { path = "../poe-contracts/contracts/tg4-group" }
cw2 = { path = "../cw-plus/packages/cw2" }
cw20 = { path = "../cw-plus/packages/cw20" }
cw-multi-test = { path = "../cw-plus/packages/multi-test" }
cw-storage-plus = { path = "../cw-plus/packages/storage-plus" }
cw20-base = { path = "../cw-plus/contracts/cw20-base" }
tg-bindings = { path = "../poe-contracts/packages/bindings" }
tg-bindings-test = { path = "../poe-contracts/packages/bindings-test" }

And you should be able to reproduce the current error:

$ cd contracts/trusted-token/
$ cargo build
$ cargo test
   Compiling trusted-token v0.2.2 (/home/mauro/work/tfi/contracts/trusted-token)
error[E0308]: mismatched types
   --> contracts/trusted-token/src/multitest.rs:124:32
    |
124 |     verify_sender_on_whitelist(deps, &member).unwrap();
    |                                ^^^^ expected struct `cosmwasm_std::Empty`, found enum `TgradeQuery`
...

[maurolacy]

Thinking about this a little more, I think I now understand the issue better: These are mixed / hybrid multi-/unit-tests. Probably for historical reasons.

Proper way to fix this (in the multitest context) is to turn those whitelist checks into tests routed through their proper contract wrapper.

[maurolacy]

Yes, the commit logs confirm this:

commit 80d69d8a3370c14c0311a8ed14d6dedcbe215cbf
Author: Ethan Frey <ethanfrey@users.noreply.github.com>
Date:   Wed Aug 18 09:33:06 2021 +0200

    Port it to unit test using custom mock querier

commit c2059ef419591b95f9f26a8895b4ad27b0f95255
Author: Ethan Frey <ethanfrey@users.noreply.github.com>
Date:   Wed Aug 18 09:00:31 2021 +0200

    Add first semi-unit test

So, the offending whitelist_works() hybrid or semi-unit test is in fact redundant(!). The easiest (and boring) way around this is to simply remove the whitelist_works() test from multitest.rs.

[maurolacy]

OK, moved forward to make this work. @ethanfrey, here's a new one for you:

error[E0277]: the trait bound `ContractWrapper<tfi::factory::ExecuteMsg, tfi::factory::InstantiateMsg, tfi::factory::QueryMsg, error::ContractError, error::ContractError, cosmwasm_std::StdError, cosmwasm_std::Empty, cosmwasm_std::Empty, anyhow::Error, cosmwasm_std::StdError>: Contract<TgradeMsg>` is not satisfied
  --> contracts/tfi-factory/src/multitest/suite.rs:36:5
   |
36 | /     Box::new(                                                                                                                         
37 | |         ContractWrapper::new_with_empty(                                                                                              
38 | |             crate::contract::execute,
39 | |             crate::contract::instantiate,
...  |
42 | |         .with_reply(crate::contract::reply),
43 | |     )
   | |_____^ the trait `Contract<TgradeMsg>` is not implemented for `ContractWrapper<tfi::factory::ExecuteMsg, tfi::factory::InstantiateMsg, t
fi::factory::QueryMsg, error::ContractError, error::ContractError, cosmwasm_std::StdError, cosmwasm_std::Empty, cosmwasm_std::Empty, anyhow::Error, cosmwasm_std::StdError>`

This is now in tfi-factory tests, where using the new_with_empty() constructor forbids us to use the with_reply() builder-like helper. I don't see an easy way around this, except for implementing some extra constructors. Or a new_with_empty constructor with optional sudo, reply, and migrate params.

Here's where Rust's lack of default arguments comes home to roost.

[ethanfrey]

Thanks for digging into this.

Is this blocking a release of patchnet? I totally support and quick changes to make tests pass and get that shipped.

That said, this is a gret investigation and points out the limits of multitest with custom messages/queries. I am very happy for you to keep this branch open and pointing out issues so we can improve multitest and others don't get hit in the future.

My first step is getting this branch to pass.

Second is capturing this behavior in simple tests in multitest.

Third would see how to refactor multitest to make this all simpler

[maurolacy]

Is this blocking a release of patchnet? I totally support and quick changes to make tests pass and get that shipped.

We already did that. This is not blocking anything anymore.

That said, this is a gret investigation and points out the limits of multitest with custom messages/queries. I am very happy for you to keep this branch open and pointing out issues so we can improve multitest and others don't get hit in the future.

My first step is getting this branch to pass.

I'm working on a proposal for that.

Second is capturing this behavior in simple tests in multitest.

Good point. Will do / create a follow-up issue for that.

Third would see how to refactor multitest to make this all simpler

Yes. I think we need to do this at some point. And the sooner the better.

[maurolacy]

This is now in tfi-factory tests, where using the new_with_empty() constructor forbids us to use the with_reply() builder-like helper. I don't see an easy way around this, except for implementing some extra constructors. Or a new_with_empty constructor with optional sudo, reply, and migrate params.

Solution was straightforward: Use the already existing and aptly named with_reply_empty builder-like helper.

So, after all this, we can see that multi-test can be used in a mixed (custom + empty) message setup. Not without some struggle, I must say; particularly for someone who is not yet an expert in it.

[ethanfrey]

Solution was straightforward: Use the already existing and aptly named with_reply_empty builder-like helper.

So, after all this, we can see that multi-test can be used in a mixed (custom + empty) message setup. Not without some struggle, I must say; particularly for someone who is not yet an expert in it.

I was just about to add this, then saw it was there, then saw your comment.

We definitely need to document multitest. If it is confusing for internal devs, how do we expect anyone else to use it?

[ethanfrey]

Is this resolved? The remaining error was | ---- ^^^^^^^^^ the traitcosmwasm_std::Querieris not implemented fortg_bindings_test::TgradeApp

But I made that change already. I guess we need to tag poe-contracts and update the tfi deps and this should pass? I'd love to see this minimally pass CI to have more confidence we fixed the major rough edges

[maurolacy]

This is resolved already, in the environment defined above.

For the main version, we just got rid of the TgradeApp entirely for the tg4-group contract, which is the dependency that was dragging it in here.

[maurolacy]

Closing this as resolved, but won't merge.