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

Update to cw-plus 0.12.1 #103

Merged
merged 40 commits into from
Feb 21, 2022
Merged

Update to cw-plus 0.12.1 #103

merged 40 commits into from
Feb 21, 2022

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Feb 14, 2022

Closes CosmWasm/cw-plus#90.

This ended being more work than anticipated, because of the extensive CustomQuery param changes. This is required by a bound on TgradeApp, part of tgrade multi-test. @hashedone and @uint , please take a look, as I'm not sure there's a simpler approach here.

On the other hand, this is already done. So, we can in the end benefit from this early introduction of CustomQuery everywhere.

Creating as Draft to prevent / delay merge after 0.6.0 is released. (done)

Update:

  • The general strategy followed here is to try to use a generic CustomQuery type parameter. Specifying it as TgradeQuery only when needed (i.e. in entry points). That way, unit tests can still call functions using Empty for simplicity. There exists the possibility to extending this generality to entry points as well. Basically, by extending our entry_point procedural macro to accept a query parameter, and doing the renaming for us. This would allow calling the entry points using Empty as query type. Will create an issue to explore / document this approach.
  • Multitests relying on TgradeApp must now (since cw-plus 0.12.1) use TgradeQuery. I'm a little worried that our (multi-) test environment is putting requirements on our contracts API. It is supposed to be the other way around. But, I don't see an easy way out; at least for for the moment (See Properly handle generic queries in multi-test CosmWasm/cw-plus#660 (comment) and follow-ups).

@maurolacy maurolacy marked this pull request as ready for review February 19, 2022 08:56
Copy link

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
I wonder if those inserted implementations of mock_dependencies() can be handled somehow better. Could cosmwasm_std part be extended with CustomQuery?

@ethanfrey
Copy link
Contributor

Basically, by extending our entry_point procedural macro to accept a query parameter, and doing the renaming for us. This would allow calling the entry points using Empty as query type. Will create an issue to explore / document this approach

Very interesting idea. However, I think we cannot just do it on entry_point in many places, if we actually use that custom query inside.

Multitests relying on TgradeApp must now (since cw-plus 0.12.1) use TgradeQuery. I'm a little worried that our (multi-) test environment is putting requirements on our contracts API. It is supposed to be the other way around. But, I don't see an easy way out; at least for for the moment

Yes, we should make multitest allow arbitrary contracts to easily plug in.

@ethanfrey
Copy link
Contributor

I have no time to review this. But can you make a tracking issue for the multitest changes, and link this and anything else. And mention me in it?

So I have an overview of all that is needed to fix this up

@maurolacy
Copy link
Contributor Author

maurolacy commented Feb 21, 2022

lgtm I wonder if those inserted implementations of mock_dependencies() can be handled somehow better. Could cosmwasm_std part be extended with CustomQuery?

Extending mock_dependencies() in coswasm_std can be done, but it will require us to always pass the CustomQuery type (as functions cannot have default param types). What we can do is to have our own (tgrade) mock_dependencies() in test-utils or so. Will do.

@maurolacy
Copy link
Contributor Author

maurolacy commented Feb 21, 2022

Very interesting idea. However, I think we cannot just do it on entry_point in many places, if we actually use that custom query inside.

Good point. I hadn't thought of that :-)

@maurolacy
Copy link
Contributor Author

maurolacy commented Feb 21, 2022

On second thought, why not?

Instead of having

#[cfg_attr(not(feature = "library"), entry_point)]
pub fn instantiate(
    mut deps: DepsMut<TgradeQuery>,

we have something like

#[cfg_attr(not(feature = "library"), entry_point(TgradeQuery)]
pub fn instantiate<Q: CustomQuery>(
    mut deps: DepsMut<Q>,

instead.

And we modify the entry_point macro to specialise the entry point for us (through raw rewriting / editing). So that, for a wasm32 target, it looks exactly like the first version above. For other targets, we don't do anything.

That means that, in test code we would be able to call both instantiate::<Empty> and instantiate::<TgradeQuery>. Whereas in wasm code, only instantiate<TgradeQuery> will be allowed. The idea here being, just to provide more flexibility during testing.

Am I missing something? Of course, writing a procedural macro like this is relatively non-trivial, as the Q parameter can appear multiple times, and in different parts of the function's body.

@maurolacy
Copy link
Contributor Author

I have no time to review this. But can you make a tracking issue for the multitest changes, and link this and anything else. And mention me in it?

Tracking issue: CosmWasm/cw-multi-test#2

@maurolacy maurolacy merged commit 5a6d3ad into main Feb 21, 2022
@maurolacy maurolacy deleted the update-to-cw-plus-0.12.1 branch February 21, 2022 17:48
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 this pull request may close these issues.

3 participants