-
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
feat: add cw721-non-transferable contract #85
Conversation
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.
Three major issues I found:
- admin address is not validated
- cw2 contract version is overwritten
- there isn't a query method for the admin address
ExecuteMsg::Mint(msg) => { | ||
Cw721NonTransferableContract::default().mint(deps, env, info, msg) | ||
} | ||
_ => Err(ContractError::Unauthorized {}), |
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.
Personally I prefer more descriptive error messages than just "unauthorized", e.g. something like "only admin can transfer this token". However I understand this requires modifying the error type in cw721-base
which is out-of-scope 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.
+1 for more descriptive errors, but as you said, adding a new error type to the cw721-base
is out-of-scope, will take it into account for a future PR
Thanks for the great feedback @larry0x! I've addressed your comments. Lmk if it looks good to you. On another topic, I've mistakenly opened the PR from my fork's main branch instead of the feature one. I can open a new PR and close this one to not have those annoying |
I would just squash-merge |
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.
One final comment. Everything else looks good to me
Co-authored-by: Larry Engineer <[email protected]>
This PR includes a contract for implementing non-transferable (aka soulbound) NFTs.