Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs: ADR-043 Base NFT Module #9329
docs: ADR-043 Base NFT Module #9329
Changes from 42 commits
8701e97
3876262
6c923d3
bdb6005
af68935
be60788
1759155
96f9e11
09c17a6
13cc5a2
435c219
1571a59
1f3ec2a
b5a36b3
835b581
e39f059
e8e6980
db8bd84
cc289b6
3f7c8bb
1098e27
55ebf53
9b4f4cf
0849698
ccb2807
278507b
8004004
51df217
5e2f4c3
f1e81fa
899e2a2
b641f4c
9d60dfd
0dbfbdb
c8642f4
5152044
0092c53
843f79c
c98ce3a
013e9ca
8891fa2
836580a
afc6680
121dff1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
With the idea of transferring metadata using ICS30, we probably want to think of the minimal on-chain required metadata here.
description
for instance might be overkill.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.
If class metadata is transferred using ICS30, I'm assuming it would effectively become immutable. i.e. once class metadata is on another chain once, it can no longer be mutated on the origin chain because that could cause inconsistencies. Maybe there are other ways to deal with this but immutability seems the simplest.
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.
We need to be very careful about how this
Update
method is used if ICS30 transfers metadata. Basically once an NFT is escrowed, metadata would be locked. If there is a module who's logic allows a minter (not owner) of an NFT to modify the metadata, that could result in an inconsistent state.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.
with the
x/bank
approach, the Mint function should usex/bank
service to create denom and set coins. Example pseudo code:I will write more about it in a general post 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.
So maybe we should change the language and use:
This specification follows the ERC-721 [link needed!] smart contract standard....
Also, an ERC-721 implementer has more choices (he creates a new smart contract for every ERC-721 token), here he can't do 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.
Without IID, the NFT identifier is not different from bank identifier and can be confused.