Properly handle generic queries in multi-test

[ethanfrey]

Make ContractWrapper::new_with_empty convert Deps to Deps

[ethanfrey]

@maurolacy this is a start.

Maybe you can try this in your tfi branch

[maurolacy]

Will give it a try tomorrow. Ended up avoiding the need for this by removing the custom message from tg4-group entirely. See confio/poe-contracts#109.

[maurolacy]

Trying this, but needs to do an upgrade of dependencies / patches locally to properly assess it works.

That said, I think this is a good idea, and we should merge this, after confirming it works, or making the needed changes.

[ueco-jb]

LGTM
I think this is a way to go.
I only wonder, is this change breaking? From generic type to concrete (Empty) one?

[uint]

Seems good enough!

[ethanfrey]

I only wonder, is this change breaking? From generic type to concrete (Empty) one?

In theory, but everyone using Response<Empty> is also using Deps<Empty>
If there are some using Response<T> with Deps<Empty> or Response<Empty> with Deps<T> that will be breaking.

Maybe we can add two extra functions for those two (less likely cases). I am sure they will pop up eventually

[maurolacy]

Tried this finally, after setting up the local env properly.

For the specific use case of the trusted-token multitests (https://github.com/confio/tfi/tree/mauro/64-replace-cw4-with-tg4) this fix, along with the missing TgradeApp Querier fix (confio/poe-contracts#111) are almost enough.

The only missing piece seems to be that there are some queries to <Empty>-based contracts which require a Deps<Empty>, but with a querier / router with the contracts already set-up:

https://github.com/confio/tfi/blob/6465915e0f14a41a6a406c8b76a664d042ca6322/contracts/trusted-token/src/multitest.rs#L115-L121

That is failing with the expected

   --> contracts/trusted-token/src/multitest.rs:124:32
    |
124 |     verify_sender_on_whitelist(deps, &member).unwrap();
    |                                ^^^^ expected struct `cosmwasm_std::Empty`, found enum `TgradeQuery`

So, it seems that we still need a helper for "downcasting" from TgradeApp to App. I tried to do it (adding an as_app() method to TgradeApp) but ran into access issues, as the TgradeApp members are private, even for itself (because of wrapping?)...

Anyway: I have a strong feeling that the message type (custom or empty) should be orthogonal to the multi-test blockchain / app. And that we are running into all these clashes, in the end, because it isn't.

It would be good to fix this for good, as these "mixed" or heterogeneous setups will become more and more common, the more and more contracts we have around.

[ethanfrey]

Anyway: I have a strong feeling that the message type (custom or empty) should be orthogonal to the multi-test blockchain / app. And that we are running into all these clashes, in the end, because it isn't.

It would be much easier if they were. But I think they cannot be.

Basically, we need to ensure the Response returned is a subset of the possible responses managed by the app.
And the Querier needed is a subset of the Querier provided.

The other option is just use Vec<u8> everywhere, but we couldn't even parse the Response from a contract without knowing the Custom type

[ethanfrey]

Can you make a PR to the case you mention that almost works and I will look into it?

[ethanfrey]

I would like to:

1. Improve multi-test generics enough to make this work well for now.
2. Later, make multi-test more robust / easy to use

[maurolacy]

Can you make a PR to the case you mention that almost works and I will look into it?

There you go: confio/tfi#68

[maurolacy]

Anyway: I have a strong feeling that the message type (custom or empty) should be orthogonal to the multi-test blockchain / app. And that we are running into all these clashes, in the end, because it isn't.

It would be much easier if they were. But I think they cannot be.

Basically, we need to ensure the Response returned is a subset of the possible responses managed by the app. And the Querier needed is a subset of the Querier provided.

The other option is just use Vec<u8> everywhere, but we couldn't even parse the Response from a contract without knowing the Custom type

Can't we define the message type at the calls / store_code, etc. levels? Instead of having everything pre-defined as Empty or some custom type at the App level.

It's a matter of decoupling the message type from the app. I understand it will require some major overhauling, though.

[ethanfrey]

The issue is the App has a custom module (not contract) that can handle custom messages C and queries Q.

The contracts must all return C or Empty so they can be dispatched.

[maurolacy]

I see. Some thoughts or ideas that come to mind:

• Use serialisation / deserialisation tricks to handle / dispatch multiple custom message types internally. Everyone knows what to expect / receive, so it just optimistically tries to deserialise to it.
• Instead of a Custom(T) enum, use the / a CustomMessage trait to abstract away different message types, and dyn our way around it. Not sure we can have something like a Custom(dyn CustomMessage) enum, but you get the idea.

These are just off the top of my head. I'm not sure how relevant / practical / possible / sensible they are.