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.

Sign up for GitHub

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

Jump to bottom

Implement cw721-base nft contract #97

Merged
merged 33 commits into from
Sep 30, 2020
Merged

Implement cw721-base nft contract #97

merged 33 commits into from
Sep 30, 2020

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Sep 25, 2020 •
edited
Loading

Closes #44

Most logic writen.

TODO:

  • Send proper message on SendNftToken
  • Add unit tests
  • Write a better README

@ethanfrey ethanfrey mentioned this pull request Sep 26, 2020
Merged
3 tasks
@ethanfrey ethanfrey requested a review from maurolacy September 26, 2020 21:34
@ethanfrey
Copy link
Member Author

@maurolacy This is roughly done for the main code path, only basic unit tests.
It also could use a good review to point out any issues in the design.

@maurolacy
Copy link
Contributor

OK, I'll review it tomorrow morning.

maurolacy
maurolacy reviewed Sep 28, 2020
View reviewed changes
Copy link
Contributor

@maurolacy maurolacy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks pretty good to me. Besides what I think is a serious error (token_id overwrite in minter), logic looks good.

Will continue reviewing the tests later.

contracts/cw721-base/schema/all_nft_info_response.json Show resolved Hide resolved
contracts/cw721-base/schema/tokens_response.json Outdated Show resolved Hide resolved
contracts/cw721-base/src/contract.rs Show resolved Hide resolved
contracts/cw721-base/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw721-base/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw721-base/src/contract.rs Show resolved Hide resolved
contracts/cw721-base/src/contract.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

Addressed the major issues raised - good points, thank you.

@maurolacy
Copy link
Contributor

I've made some changes directly on your branch. Please review them.

@ethanfrey ethanfrey marked this pull request as ready for review September 30, 2020 19:45
@ethanfrey ethanfrey merged commit 98ec1ab into master Sep 30, 2020
@ethanfrey ethanfrey deleted the cw721-nft-base branch September 30, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maurolacy maurolacy maurolacy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement base NFT contract
2 participants
@ethanfrey @maurolacy