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

packages: support custom queries #652

Merged
merged 8 commits into from
Feb 9, 2022
Merged

Conversation

uint
Copy link
Contributor

@uint uint commented Feb 8, 2022

In order to resolve confio/poe-contracts#36, it looks like we need the tgrade-valset contract to use TgradeQuery as the custom query type for Deps and DepsMut (currently it's Empty).

In order to do that, Admin needs to support custom query types that are not Empty in Deps/DepsMut.

@uint uint requested review from ethanfrey and maurolacy February 8, 2022 14:29
@ethanfrey
Copy link
Member

Ah, yes, now that I made Deps/DepsMut generic over the custom query type, we need to make controllers generic over that as well, not just Response.

I agree. Can you also update other controllers? (Possibly as a follow up)

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

nice find

@ethanfrey
Copy link
Member

ethanfrey commented Feb 8, 2022

Note that main is breaking, so after merge, you should make a new branch off of v0.11.1, back port this fix (and for the other controllers) and release a v0.11.2.

I don't think v0.12.0 is coming out so fast, so let's just patch the last stable version.

That said, those issues seem quite optional investigation work I threw in so they wouldn't be forgotten, so maybe just rename the v0.12.0 milestone to v0.13.0 and we cut a v0.12.0 soon? @maurolacy is managing this release I believe

@uint
Copy link
Contributor Author

uint commented Feb 8, 2022

I agree. Can you also update other controllers? (Possibly as a follow up)

Happy to do that, should be easy enough.

@maurolacy
Copy link
Contributor

OK with me. Only thing is that v0.12 will include type safe bounds, and it will require code migration.

Migration should be easy, though. And, there's already a MIGRATING entry for this.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM.

@maurolacy
Copy link
Contributor

maurolacy commented Feb 8, 2022

As @ethanfrey says, you can either cherry pick this into a 0.11 branch (branched off from the latest 0.11 version tag) and release a new 0.11, or release 0.12 early. I for one would go for the 2nd, even when it'll require some code migration on the client repo(s).

Your call.

@ethanfrey
Copy link
Member

Speaking of.. let me tag the v0.12.0-alpha2 release I prepared.
And then I will make one more PR I would like in a final v0.12.0. I will try to finish tonight, be able to review/merge tomorrow

@maurolacy
Copy link
Contributor

Great. I can help with releasing 0.12 final if needed.

@uint
Copy link
Contributor Author

uint commented Feb 8, 2022

I realized we might also need to update multitest with custom queries and I'm taking a shot at that, but it's not trivial.

@ethanfrey
Copy link
Member

This is the relevant PR: #653

I would like that in before a v0.12.0 is tagged

@uint uint changed the title controllers: support custom queries packages: support custom queries Feb 8, 2022
@uint uint force-pushed the controllers-custom-query-type branch from 28a7ab2 to 3edbdc7 Compare February 8, 2022 21:26
@uint
Copy link
Contributor Author

uint commented Feb 8, 2022

Okay, I tried to cover all of the /packages, multi-test included. Hopefully I didn't miss anything. Can you please take another look?

@uint
Copy link
Contributor Author

uint commented Feb 8, 2022

As @ethanfrey says, you can either cherry pick this into a 0.11 branch (branched off from the latest 0.11 version tag) and release a new 0.11, or release 0.12 early. I for one would go for the 2nd, even when it'll require some code migration on the client repo(s).

I'd rather cut the new release than backport things, too. I guess for the migration there's nothing to do on my side if there's already a MIGRATING.md entry? Just that this is breaking for users?

packages/multi-test/src/lib.rs Outdated Show resolved Hide resolved
packages/controllers/src/admin.rs Show resolved Hide resolved
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM.

packages/multi-test/src/contracts.rs Show resolved Hide resolved
packages/multi-test/src/contracts.rs Show resolved Hide resolved
@uint uint merged commit 49fceee into main Feb 9, 2022
@uint uint deleted the controllers-custom-query-type branch February 9, 2022 13:57
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.

Ensure validator is online before adding to the active set
3 participants