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

Add CW-1155 to implement EIP 1155 #27

Closed
wants to merge 9 commits into from
Closed

Conversation

sgoya
Copy link
Contributor

@sgoya sgoya commented Nov 9, 2021

This is a first pass at implementing cw-1155 for cw-nft repo.

@sgoya sgoya changed the title Implement CW-1155 to implement EIP 1155 Add CW-1155 to implement EIP 1155 Nov 9, 2021
@JakeHartnell
Copy link
Collaborator

Wow! This is looking really good! Made a few small comments, will take a deeper pass tomorrow.

Copy link
Collaborator

@the-frey the-frey left a 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)]
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🙃 no worries haha

contracts/cw1155-base/helpers.ts Outdated Show resolved Hide resolved
contracts/cw1155-base/helpers.ts Outdated Show resolved Hide resolved
contracts/cw1155-base/README.md Outdated Show resolved Hide resolved
contracts/cw1155-base/README.md Outdated Show resolved Hide resolved
@giansalex
Copy link
Contributor

@JakeHartnell
Copy link
Collaborator

JakeHartnell commented Nov 13, 2021

cw1155 is not in cw-plus ? https://github.com/CosmWasm/cw-plus/tree/main/contracts/cw1155-base

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 cw721-base.

@ethanfrey
Copy link
Contributor

I would love to see this here, so I can remove it from cw-plus. Both packages/cw1155 as well as contracts/cw1155-base.

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

@JakeHartnell
Copy link
Collaborator

Closing in favor of #78

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.

7 participants