-
Notifications
You must be signed in to change notification settings - Fork 181
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
Conversation
This is AWESOME idea IMO. It's currently annoying to extend the queries. Others have thoughts on this? |
Very useful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 💯
Will be working to test this hopefully over the weekend. Really excited about this PR. |
Any news ? I would love to see this merged :) |
@ethanfrey what do you think of this design? It looks good to me but best to get your opinion before standardizing this. |
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. |
There was a problem hiding this 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.
Note, I imagine people will have to upgrade their contracts after this... probably should make the next release a major release. |
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? |
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. |
Yeah, no client issues from this, but smart contracts that integrate it will have to be updated.
That would be great! |
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. |
Most definitely. |
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.