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

attempt to update cw-nft to add custom query and custom execute. #42

Merged
merged 18 commits into from
Jul 24, 2022

Conversation

SylvestreG
Copy link
Contributor

@SylvestreG SylvestreG commented Jan 4, 2022

Hi,

Here is an attempt to add support for custom queries and custom executes to cw-nft (#31).

I was trying to extend cw-nft but It seems difficult to extend cw721-base without including it in your project.

This is just an attempt, and I am not a rust daily developper. So if my diff is silly or useless feel free to ignore it.

@SylvestreG SylvestreG marked this pull request as draft January 4, 2022 10:37
@JakeHartnell
Copy link
Collaborator

JakeHartnell commented Jan 5, 2022

This is AWESOME idea IMO. It's currently annoying to extend the queries. Others have thoughts on this?

@orkunkl
Copy link
Contributor

orkunkl commented Jan 5, 2022

Very useful

orkunkl
orkunkl previously approved these changes Jan 5, 2022
Copy link
Contributor

@orkunkl orkunkl left a comment

Choose a reason for hiding this comment

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

This is 💯

@JakeHartnell
Copy link
Collaborator

Will be working to test this hopefully over the weekend. Really excited about this PR.

@SylvestreG
Copy link
Contributor Author

SylvestreG commented Feb 15, 2022

Any news ? I would love to see this merged :)

@orkunkl
Copy link
Contributor

orkunkl commented Feb 17, 2022

@ethanfrey what do you think of this design? It looks good to me but best to get your opinion before standardizing this.

@the-frey
Copy link
Collaborator

I think this is a decent solution to the issue at hand. It does remind me though that the spec itself is basically just a contract boundary/validation layer.

IMO that implementation of the functionality might long-term be better served as being entirely a package of functions that you can pick and choose from, rather than traits. Then the contract would just be importing what you want, and not using what you don't.

However, in that case - we would still need to extend the enums arbitrarily in a bunch of scenarios as that is basically the validation part, so I'm very pro simplifying how that all fits together. This seems like a decent way of doing that.

Anyway, possibly too many thoughts for this PR.

JakeHartnell
JakeHartnell previously approved these changes Jul 17, 2022
Copy link
Collaborator

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

This is a really great effort! Pushed some commits to move things along. Would love to get this in the next release as I think it's a big improvement and unlocks a lot of possibilities.

@JakeHartnell
Copy link
Collaborator

JakeHartnell commented Jul 17, 2022

Note, I imagine people will have to upgrade their contracts after this... probably should make the next release a major release.

@the-frey
Copy link
Collaborator

Yep, I think we should merge this when we're ready to coin a breaking major release, because it will need some integrating.

Correct me if I'm wrong though - assuming folks upgrade and integrate, there won't be any client-side issues, because it's only governing custom query extensions, right?

@SylvestreG
Copy link
Contributor Author

IMHO there will be no client issue, and I think the migrate will be quite simple as there is no state modification.

If you want I can try to do some migrate next week on some of my devnet contracts. I will give you some feedback on the result of these migrates.

@JakeHartnell
Copy link
Collaborator

IMHO there will be no client issue, and I think the migrate will be quite simple as there is no state modification.

Yeah, no client issues from this, but smart contracts that integrate it will have to be updated.

If you want I can try to do some migrate next week on some of my devnet contracts. I will give you some feedback on the result of these migrates.

That would be great!

@JakeHartnell JakeHartnell requested review from Callum-A and 0xekez and removed request for orkunkl July 23, 2022 03:34
@shanev
Copy link
Collaborator

shanev commented Jul 24, 2022

Going off what @the-frey said, I think a next-gen version of this is where you choose what features you want to be imported into your custom cw721, similar to how tokio works. But until we have that, this seems like a good solution to me since it removes a bunch of copy-pasta when writing custom cw721 contracts. Oh yeah and we definitely want this to get in during a major version number change.

@JakeHartnell
Copy link
Collaborator

Oh yeah and we definitely want this to get in during a major version number change.

Most definitely.

@JakeHartnell JakeHartnell merged commit d546a61 into CosmWasm:main Jul 24, 2022
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.

None yet

5 participants