-
Notifications
You must be signed in to change notification settings - Fork 180
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
Add CW-1155 to implement EIP 1155 #27
Conversation
Wow! This is looking really good! Made a few small comments, will take a deeper pass tomorrow. |
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.
Code looks good, thanks for the contribution! However, a couple of fixups before merge please:
- CI is broken
- Tests need uncommenting and adding
@@ -0,0 +1,680 @@ | |||
// #![cfg(test)] |
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.
I haven't seen any other contract tests - so would expect these to be implemented and fixed before merge to be sure of the contract behaviour.
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.
Hi Ethan, sorry this is still super WIP. I wanted to get feedback on the state model before I add all the tests so that if we end up changing the state file, not too much code needs to be changed.
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.
ha, ethan is @ethanfrey - first time i've ever had a name collision with this handle
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.
I am so sorry, I saw frey and assumed incorrectly. Sorry about that.
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.
🙃 no worries haha
cw1155 is not in cw-plus ? |
Probably makes sense for it to move here. Additionally, it would be nice to make it easily extensible similar to what's been done with |
I would love to see this here, so I can remove it from cw-plus. Both I would propose a literal copy (and CI fix, etc) as a first PR. And when that is in a tagged release, some follow ups that modify it like cw721-base was extended. I would note we just bumped cw-plus to v0.11.1. There are some nice helpers, especially about ranges and multi-indexes that should make life easier but will take some conversion. Maybe you can bump this repo to v0.11.1 first, and then push cw1155 as v0.11.2, so you can keep a progress on the same numbers as it was already published on crates.io |
Closing in favor of #78 |
This is a first pass at implementing cw-1155 for cw-nft repo.