docs: ADR-043 Base NFT Module

[chengwenxi]

Description

replace: #9284
ref: #9174 #9065

This ADR defines a generic NFT module of x/nft which supports NFTs as a proto.Any and contains BaseNFT as the default implementation., which supports storing NFT by id and owner, and contains the data field as proto.Any to extend the usage scenarios of NFTs.

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[chengwenxi]

@alexanderbez @fedekunze @marbar3778 @shahankhatch We updated this ADR and resolved all comments in #9284, could you review it again?

[fedekunze]

Looks good. Some items missing:

• How are NFTs stored?: imo we should remove GetId and GetOwner and use them as primary and secondary index keys, respectively, to store the NFTs and NFT Ids. Otherwise, define how you can query all the NFTs by owner.

id (string) --> NFT (bytes) // primary index for NFTs, stored by ID
owner (address) --> []string  // store the NFT IDs owned by an address

• The decision of leaving minting, burning restrictions to upper-level modules that import the NFT module should be clearly stated.

• The logic for transferring the ownership of an NFT to another address (no coins involved) should also be defined in this module since you are working already with owners.

func (k Keeper) TransferOwnership(nftID string, currentOwner, newOwner sdk.AccAddress) error

• It's not entirely clear how the BaseNFT is based on ERC721. Define how you handle the name and symbol fields (eg: perhaps leave it to the collection module) and how you can perform a conversion from ERC712 --> BaseNFT (i.e the x ERC721 field maps to the y field in the Cosmos NFT).

[chengwenxi]

Looks good. Some items missing:

• How are NFTs stored?: imo we should remove GetId and GetOwner and use them as primary and secondary index keys, respectively, to store the NFTs and NFT Ids. Otherwise, define how you can query all the NFTs by owner.

id (string) --> NFT (bytes) // primary index for NFTs, stored by ID
owner (address) --> []string  // store the NFT IDs owned by an address

My intention to add these two methods is that nftkeeper can expose SaveNFT method as

func (k Keeper) SaveNFT(nft NFT) error{
    if !validate(nft){
        assert("invalid nft")
    }
    saveNFTById(nft.GetId(), nft)
    // saveNFTByOwner(nft.GetOwner(), nft)
    saveNFTByOwner(nft.GetOwner(), nft.GetId())
}

instead of

func (k Keeper) SaveNFT(nftId string, owner sdk.AccAddress, nft NFT) error{
    if !validate(nft){
        assert("invalid nft")
    }
    saveNFTById(nftId, nft)
    // saveNFTByOwner(owner, nft)
    saveNFTByOwner(nft.GetOwner(), nft.GetId())
}

[fedekunze]

My intention to add these two methods is that nftkeeper can expose SaveNFT method as

I guess it's fine as long as you document you are aware of the tradeoff of storing the extra bytes (id and owner) on the struct too

Note, I wouldn't store the NFT twice under different keys, and instead would use:

saveNFTByOwner(nft.GetOwner(), nft.GetId())

[chengwenxi]

I guess it's fine as long as you document you are aware of the tradeoff of storing the extra bytes (id and owner) on the struct too

Actually, I have no particular preference for storing the extra redundant fields. Just follow the conventions in other modules(e.g. staking & gov). Do you have some suggestions?

Note, I wouldn't store the NFT twice under different keys, and instead would use:

saveNFTByOwner(nft.GetOwner(), nft.GetId())

Thanks for your note.

[charleenfei]

@alexanderbez @aaronc would you have a moment to leave any last reviews on this ADR? @aaronc I know you mentioned in particular giving this module its own go.mod --

[aaronc]

I'm concerned that this ADR doesn't actually specify much.

For instance how do any NFTs get minted or burned? It says metadata can be mutated but how? It says that also says that the logic for transferring NFTs is defined but what is the logic? What rules are or are not enforced?

Also, I don't think the protobuf API is very usable. Why do id and owner get packed inside an Any if all NFTs must have them? The Any just wraps polymorphic metadata but how does that even get set? And if there is another module actually managing the NFTs why doesn't that module manage metadata for its class of NFTs?

To move this forward, is some representative of this ADR able to discuss at the next architecture call on June 4th?

[aaronc]

Thanks @jandrieu. So I've been meaning to comment in #9589, but as I understand it there is no known technical solution for IID's. It would be great if @colin-axner could comment, but based on my read of the situation, once IBC is involved we can't disambiguate essentially identical denoms/classes because of how a token was routed from one chain to another. IID's are an idea that I really like in theory, but without a technical solution it is a bit hard to use them in this case.

[aaronc]

This is just a sketch and there are certainly improvements that could be made. It might be more efficient to store the Class and ID combined and use the denomTrace capabilities to allow querying by Class on the destination chain. Those are all important questions but should be part of the ICS. I think the important goal of looking ahead to see if there are any red flags we can already identify is right to bring up and I appreciate @aaronc doing so. But I think it's safe to say there is no obvious reason why the current ADR would be problematic for IBC.

Well, if we are storing class and ID combined then it sounds like we're doing the same thing as ICS20 in which case we wouldn't want class and ID as top-level elements in this design, or am I reading this wrong?

There is one other big issue in my mind about using ics-20. There doesn't seem to be a good way to guarantee that the sender chain is sending a NFT or a FT. We can say that they need to prefix their denom with nft/ or something else before sending it over. But this means that any FT that legitimately uses a denom that begins with nft/ (or whatever other namespacing technique used) will be incorrectly treated as an NFT.

I don't think a denom prefix of nft/ is a good solution here.

An NFT that is forced to be FT once transferred over IBC is a bad user experience.

Can you say a little more about the problem you're seeing here @okwme? It may be a legimate concern and it may also be one that has straightforward solutions. In my mind I'm assuming that a) client software has ways of knowing what the denoms actually are - in the IBC software I've used this has been the case and b) that if a state machine needs to know this there is a light client proof that can provide metadata about the denom. i.e. in neither case we actually need to communicate that the token is in the transfer packet we just need to communicate the ownership information. In my mind, I'm treating this similarly to the URI case that we talked about @okwme. i.e. the URI isn't something that gets "transferred" - it is there on the host chain if any client needs it but it's not IBC transfer's job to transfer metadata. Now, a boolean for FT vs NFT could be an exception to this if I really understood the use case clearly. But, I do think we should justify why for this boolean we do include that in the IBC packet necessitating a completely separate ICS, whereas for metadata/URIs we can just use light client proofs.

Or we need to modify ics-20 to contain a boolean for nft or ft, and that will require just as much work as a new ICS and implementation.

I agree that it sounds like a similar amount of work and if this is the case we should just make a new ICS.

[jandrieu]

@aaronc wrote:

Thanks @jandrieu. So I've been meaning to comment in #9589, but as I understand it there is no known technical solution for IID's.

I'm fascinated by this. Could you explain in more detail? IIDs are DIDs and DIDs are under wide development and use, including deployments that are expected to reach tens of millions of people this year alone. So, it isn't the DID part of IIDs that are the problem. The extensions to DID properties added by IIDs are neither controversial nor complicated. They are a straight-forward engineering task. To say there is "no known technical solution" means either you are seeing something I'm not aware of or I have failed to communicate IIDs effectively.

It would be great if @colin-axner could comment, but based on my read of the situation, once IBC is involved we can't disambiguate essentially identical denoms/classes because of how a token was routed from one chain to another. IID's are an idea that I really like in theory, but without a technical solution it is a bit hard to use them in this case.

I would love to understand how this is a failure. The DID universe not only allows, it requires each namespace to define its own method-specific identifiers. The rest of the ecosystem treats those as opaque. A wallet need not understand anything about particular DID methods, it just needs a resolver that does. Best practice is to use your own resolver (or buy a resolver service from a trusted party), but the wallet just knows the ID and the method, and doesn't need anything else until it parses the DID Document returned from the resolution process. That's where the verification relationships describe how to interact securely with the asset and service definitions present where to interact with the asset.

So how does an opaque identifier--whose purpose is literally to delineate potentially similar assets--how does that become ambiguous just because IBC? A DID is a DID is a DID, regardless of the chain its on.

My expectation is that there are assumptions here, such as constructing identifiers as hashes of denoms and classes, which I believe are the disconnect. This is the disconnect I'd like to figure out.

From your comment, I believe you are actually making my point, namely these two factors are in conflict:

1. We need unique, unambiguous identifiers (which IIDs are)
2. The current class/id schemes are NOT unambiguous

Your conclusion is that IIDs don't work because #1 and #2 are incompatible.

My conclusion is that #2 is the problem and that #1 is a requirement regardless of whether or not IIDs are adopted as a solution. If you're building your identifiers such that they are NOT globally unambiguous, I say you're doing it wrong.

I'm not sure if we're talking past each other, but I think we're getting closer to why I believe IIDs are a critical feature for NFTs: if you can't remove the ambiguity (and IIDs do that) then you can't even know which NFT is involved in a given transaction. Settle on an approach that is compatible with IIDs and then you'll have globally unique identifiers that work. Use a different approach and you still need to figure out global uniqueness or you can't tell NFTs apart. There are other ways to solve that, but you need one as part of the solution.

[aaronc]

@jandrieu this discussion goes into some of the issues: tendermint/cns#16. Again I'd love if @colin-axner or maybe @AdityaSripal could comment.

But as I understand it, with IBC it's a hard problem to prove that two tokens indeed come from the same chain and even if we do know that they're from the same chain, we can't easily make them fungible. I believe this is because IBC tokens are really vouchers for tokens on the origin chain, or in the case of multi-chain hops vouchers for vouchers for origin chain tokens, if that makes sense. Maybe there is a technical solution to all of this but I believe the complexity is that one chain will have different levels of trust for other chains. If tokens from chain X show up directly on my blockchain and tokens from chain Y that are also originally from chain X show up, how does a user know whether to trust chain Y to send the tokens back to X in the general sense? Furthermore, the vouchers on chain Y need to be redeemed when going back to chain X - we can't just go straight back to X. Maybe there will be future solutions for this, but it seems like for now, we're stuck with on-chain identifiers which need to contain the port-channel path and can't be easily disambiguated into universally unique DIDs/IIDs much less made fungible when sent over different port-channel paths. Note that most of this relates to FT fungibility, but it also applies to equality at the class level for NFTs.

Hope that helps, but again I'd love it if someone more knowledgeable than me can comment.

[aaronc]

Also I would say that maybe a resolver at the UI level can help and that would be great, but it would still have to deal with the challenges of multi-chain hops, fungibility, equality, etc. By the way maybe best if we move this to the IID discussion thread.

[jandrieu]

maybe a resolver at the UI level can help

This is at least one of the languaging issues. With DIDs, "resolving" is the process of taking a DID and retrieving or generating its current authoritative DID Document. The way that you find the public key for verifying a particular interaction on behalf of a given DID is by looking up that key in the DID Document that you retrieve by resolving the DID. This has nothing to do with making a human readable string as in this quote from the tendermint thread:

e.g.. 100ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B77878 will be resolved as 100ibc/stake/testchain1

Perhaps more confusing to me is the attempt to assert that an NFT on any given chain is the same as the NFT on another chain. IMO, that's a non-starter. Each chain is its own global statespace. What is on one chain is fundamentally NOT on another chain. Chains are rivalrous just like geography. Your token can't be on two chains at once any more than your physical person can be in two different geographic locations at once.

I know I'm veering into fundamental IBC questions, but I'm trying to understand the semantics of the interaction. This was one of the items we had discussed on the Interchain NFT calls and I thought I understood the answers we got from the Gravity Bridge folks.

It's one thing to freeze an asset on one chain (asset A) and cryptographically tie it to the state of an asset on another chain (asset B). You could consider that the equivalent of moving the NFT from one chain to another... and one COULD construct that freeze to enable its defrosting if--and only if--Asset B is appropriately consolidated and frozen. In this way, you could argue there is a reasonable semantic for "moving" an NFT from one chain to another.

However, in neither case is Asset A ever Asset B. Asset A will have available all of the chain mechanisms of Chain A, and Asset B will not. Full stop. You most likely won't be able to call smart contracts on Chain A from Chain B. You would probably move control back to Chain A to trigger the contract, meaning the smart contract on Chain A only ever acts on Asset A. It probably has no idea about Chain B or Asset B.

For example, if I have a CryptoKitty that allows me to breed on Ethereum, there is no guarantee that that service even exists on Chain B and definitely no guarantee that Chain A would recognize any changes made on Chain B. So, when Chain B "breeds" that CryptoKitty, and sets the siring ID and the breeding lock, that siring ID and breeding lock are ONLY relevant on Chain B. Chain A doesn't even know about it.

In other words, the asset on Chain B is fundamentally NOT the asset on Chain A. Ever. They may have mutual ownership and, in some cases, may recognize mutual encumbrances, but they aren't the same NFT.

So, could you (or someone) walk me through how a CryptoKitty, identified uniquely by an Ethereum IID causes problems when "transferred" in some way to Cosmos, with its Cosmos IID. My understanding is Gravity Bridge would allow something like this, but I don't understand how it works.

Neither the IIDs nor their DID Documents are the same. Something in the Chain A DID Document would likely memorialize the transfer to Chain B and hopefully Chain B has some pointer to Asset A, so these can be cryptographically linked. The details of how this works is TBD and hopefully part of token management enabled by a "Tokens 2.0" module. However, that feels like mostly straightforward engineering; the conceptual foundation is that with the right cryptographic semantics IIDs should absolutely be usable across chain, if only because DIDs as an architecture are chain agnostic.

I'm sure this is a misconnect on the IBC model, because I'm not understanding how very basic statements hold up, like

But as I understand it, with IBC it's a hard problem to prove that two tokens indeed come from the same chain and even if we do know that they're from the same chain, we can't easily make them fungible.

Umm... if two tokens have the same IID method, they are on the same chain (barring multi-chain IID methods, but even this can be addressed because such methods provide a means to find the chain the asset is on). This is why I like IIDs. They can uniquely identify any asset on any chain.

How does IBC undo this uniqueness?

As for making an NFT fungible, the way I would to it is by locking the asset on Chain A with a cryptographic link to a fungible token on Chain B. That link is all you need (as long as its semantics are clear).

What is important is not that the Asset A === Asset B, but that Asset B is a rigorous derivation of value from Asset A.

This semantic makes sense to me, but it seems the IBC way is perhaps much more loose about these definitions. Or maybe its just that words are hard and everyone shaves the edges sometimes and I'm still absorbing the IBC tribal knowledge.

FWIW, I think of this like how corporations work in the US (apologies to non-US folks, I'm not sure how this is handled elsewhere). Corporations only exist if they are registered with the Secretary of State of a specific state. However, to "do business" in many states, they must register as a foreign corporation with each state. So, a startup forms a Delaware corp then registers as a foreign corp in California. In this sense, the corporation has NOT moved its charter to California. It is not a California Corp. It's just filed some paperwork so the machinery of state can track it for taxes and compliance and lawsuits and such. IMO, IBC is like that registration of a foreign corp and does NOT move assets from chain to chain, it simply registers the foreign asset so that on-chain components can make sense of it.

The other option of course would be to dissolve the Delaware corp and incorporate anew in California. However, again, the CA corp would NOT be the same legal entity as the Delaware corp, even if it owns 100% of the same assets and assumes 100% of the Delaware corps obligations.

It's not a big deal if the initial NFT module uses something other than IIDs or DIDs, but it would be a big deal if the NFT architecture precludes the use of DIDs, so I'm trying to understand this argument that there is "no technical solution" for using IIDs with IBC-compatible NFTs.

Could someone walk me through how IIDs for NFTs break when used for IBC interactions? Or even more "standard", how they would break if simple DIDs were used?

[okwme]

Well, if we are storing class and ID combined then it sounds like we're doing the same thing as ICS20 in which case we wouldn't want class and ID as top-level elements in this design, or am I reading this wrong?
I believe we do want class and ID as top level elements. It is useful information to be able to query on the destination chain the total supply for an entire class of NFTs (albeit ones that travelled through the same port channel combo since similar to FT transfers that use different routes are non-fungible to each other). If we lose the ability to distinguish whether an NFT is NFT or FT in the state machine it will be a degraded user experience. It's one thing to resolve the metadata about the NFT from the origin chain, but basic operations and queries around supply are imo important to be part of the state machine.

The light client solution to the problem above is an entire new IBC transaction. I've discussed with @colin-axner and @cwgoes about the problem of IBC queries in the past and they suggested that it would be far easier and more straight forward to make a normal IBC transaction that requests information from another chain and is returned as a result rather than take state from that chain and try to verify it with light client proofs outside of the standard IBC flow.

At the core of this I still stand behind my primary reason for separating the current bank module from the NFT module: fungible and non-fungible tokens serve different purposes and different users to a great enough extent that is worthwhile keeping them separate at the level of module state and IBC.

[okwme]

I'd also like to suggest the IID / DID conversation be taken to the IID discussion thread.

[colin-axner]

Hello! 👋 Sorry for the delay, I was hoping to post a more detailed response today, but it seems the discussion has jumped ahead

I will only touch on IBC integration based on the current proposal

From my understanding, ICS 20 is essentially a generic transfer protocol which makes the assumption we are only transferring fungibile tokens. Specifically, an object with a denom and an amount. I'm working under the impression an ICS 30 protocol would basically be a ICS20, but with the packet data using an NFT object instead of the amount field and a class-id instead of the denom field (based on the current proposal). I wanted to check with @AdityaSripal and @cwgoes if they had another implementation in mind, but this makes the most sense to me. We could reuse a lot of infrastructure being built for ICS 20

With that said, lets look through some of the comments on ICS 20 to ensure we all have the same understanding.

When we move assets over IBC, we can reference the class/ID from the origin chain to the first receiving chain. But, the identifier for an IBC asset then becomes a combination of the source name of the asset and the channel/port. For ICS20, we decided to simply hash the source name/channel/port so that an IBC denom for a fungible token is something like ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2

This is correct with one small correction. ICS 20 does not specify ibc/{hash}. This is specific to the SDK transfer module implementation of ICS 20. It just so happens that IBC UX is also adopting this. The original intention was to devise a scalable internal representation of these tokens. When the ibc/{hash} token is transferred, the packet data does not contain ibc/{hash}, instead it uses the full denomination which refers the entire history of the token, something like channel-97/transfer/channel-11/transfer/atom

Also, on multi-zone hops, the origin denom isn’t preserved - a client would need to trace the denom back to its origin to learn about its origin name and metadata.

The origin denom is preserved as indicated by the full denomination transferred in the packet data. What needs to be traced back and proved is the origin chain

my understanding is that it’s non-trivial to know that two classes are equivalent by comparing channel/port pairs

This is true, but it is still certainly possible. The important thing to note though, in most situations, usage of a different channel/port actually implies that the classes are not equivalent. The only time they would be equivalent is if the NFTs were transferred using the same underlying connection, but why would we have 2 channels doing the exact same thing? Usually each channel performing ICS 20 functions (or ICS 30) will have different security guarantees via usage of different connections/IBC clients. Every time a fungible token or eventually a non fungible token is transferred to another chain, it accrues baggage in the form of security guarantees. The virtual copy is now secured by a different validator set and that should certainly affect its uniqueness. It is not identical to the NFT escrowed on the origin chain because of this

Regardless of the design of the NFT module, associating an object transferred across IBC with a channel/port is an unavoidable. Something good to note, associating an object with the channel/port it was transferred over does give it uniqueness since all channel/port ids are unique on a given chain. So while channel-5 on the hub and regen represent different things, they both represent something very concrete and uniquely identifiable (ie channel-5 is a connection to a chain with id "mychain" with this list of security guarantees and this validator set and this current consensus root at this latest height). Identifying the chain corresponding to channel-5 in a human readable fashion does require extra work, but it isn't a difficult solution, just tedious

All of the class/supply/metadata/etc. management might actually belong in a module on the origin chain which deals with the actual minting, and the basic NFT module should just deal with very simply ownership of unique token identifiers.

Is this the primary question being debated? Once NFTs are transferred over IBC, what functionality is retained?

Assuming ICS30 is the IBC standard for NFT transfer, if ICS30 processes NFT class in the same manner as ICS20 handles denom -- while leaving NFT id intact along the way, then x/nft will perform consistently across all zones on the path in terms of tracking supply of any given NFT class, be it a local class or some alien class on-route to another chain.

I agree with this. My conceptual model is that the class id is the denom. The class id would need to change to account for the channel/ports it has been transferred over (the original class id would not be modified, just the current representation on the chain it exists. Alternatively, a lookup table could be used). This could be done in the same fashion as ICS 20 denomination, but how it is done is outside the scope of the current discussion. I don't see any problem with changing the class id as the nft transferred is not equivalent to the one escrowed on the origin chain as I stated above. Furthermore, 2 identical class id's on different chains are not equivalent as class id's are namespaced by the chain they are on so maintaining the same class id across chains is unnecessary. So long as the entire history of the transfer and the original class id is passed along, then everything should be fine

it would seem to me that reusing ICS20 is still gives us the quickest time to market and there weren't a lot of fundamental blockers to this route in #9065, mainly UX/development speed concerns that I think can be/have been addressed.

I'd favor just doing a copy/paste of ICS 20. Future improvements to the ICS 20 spec could make it a generic transfer protocol, but I think it'd be faster to implement to just copy past the spec and implementation and swap out the packet data to contain NFT information

Without modifications to ICS 20, I don't see it being a good solution to the current discussion because I think we would want to transfer metadata with the NFT in the same way we transfer the amount of a FT.

After going through most of the posts I think I am starting to understand the concerns at hand. ICS 20 offers a very important security guarantee that a counterparty chain cannot change the supply of the origin chain. So a malicious chain connected to the hub cannot increase or decrease the supply of atom. This is an important notion in relation to NFTs because it restricts management of metadata on the receiving chain

I think most posts are working under the implicit assumption that ICS 30 would only transfer the NFT id, it would not transfer permissions to modify the metadata. This makes sense once you consider the issue of modifying metadata on a receiving chain and then trying to unescrow it on the original chain. This does add an extra layer of complexity UX wise since metadata would always need to be obtained from the origin chain. ICS 20 tracks most information of a token so tracing the full history is only needed when you want to know the chain id of the origin chain

However, even if we don't allow metadata to be modified after an IBC transfer, I don't see why we cannot transfer the metadata with the NFT id. This would solve the issue of querying the origin chain. ICS 30 could simply specify that once an NFT is in escrow its metadata cannot be modified. Transferring the class information is probably too much since a single escrow of any nft in that class would need to freeze modifications of that class information. I think this is fine though. I guess the class information is kinda like knowing the original chain id

Some might object here and say "but it'd be really nice to modify NFT metadata while it exists on a different chain", but this seems very complex to me. If we transfer over IBC, how do we know that the receiver is the same identity as the owner? The origin chain cannot do verification of an address corresponding to a counterparty chain. Given the complexity of modifying metadata once it has been transferred via IBC, I think it is an okay requirement to freeze modifications until the NFT is unescrowed. Future improvements could implement a new ICS to tackle this problem, but this seems to be far in the future

Let me know if this clarifies things. I think the current design is clean. It is hard to comment on the design without a understanding of what ICS 30 would look like. Designing ICS 30 in the same approach as ICS 20 makes the most sense to me technically and from a product point of view. As I stated above, without significant complexity in an ICS 30 design, we cannot transfer the permissions to modify an NFT after an IBC transfer nor can we do modifications while it is escrowed. Thus transferring the metadata with the NFT seems like a reasonable approach and handles most UX concerns

[okwme]

Thanks for the thorough rundown @colin-axner ! It's exciting to hear your thoughts about the viability of transferring metadata. I think it points out that we need to have much more discussion about that point, but that it should take place as part of a future ICS conversation and that there is no reason that it would require any changes to the current ADR or block development of the NFT module in its current trajectory.

[aaronc]

Thanks @colin-axner. That helps me better understand the IBC situation and @okwme I am now comfortable with the rationale for ICS30 with this current approach. I'll come through later and add some more comments, but happy to signal my pre-approval.

[aaronc]

I think there are definitely some things that need to be thought through with the proposed design of using ICS30 for metadata. Left a few comments regarding those. They can be dealt with in follow-up PRs as ICS30 evolves if that's preferable.

Also, generally the SDK team is wanting to move forward with the ADR 033 approach. Since that is not ready yet I do want to block this work on that, but it is something that should be added in the near future.

[aaronc]

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.

[aaronc]

Suggested change

UpdateClass(class Class)

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.

[aaronc]

Suggested change

	Update(nft NFT)
	Update(nft NFT)

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.

[jandrieu]

I'd also like to suggest the IID / DID conversation be taken to the IID discussion thread.

Done. #9589 (comment)

[okwme]

@robert-zaremba ok to update your changes requested review?

i believe the issues were addressed in the larger thread and didn't reply to last request to confirm

[robert-zaremba]

It is not clear how the interoperability is achieved between the new NFT modules. I guess you want to make the x/nft to be a new bank module for NFTs. So all new NFT modules will need to hook into the x/nft (similarly to what we have been discussing with x/bank).

Please describe in detail in this ADR how the interoperability is achieved. Maybe in a new section before Consequences?

[robert-zaremba]

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.

[robert-zaremba]

Without IID, the NFT identifier is not different from bank identifier and can be confused.

[robert-zaremba]

wow, get merged while writing a comments. Fine - we can iterate over it. I would still like to have a section in the ADR about interoperability and how it will work.

[okwme]

sorry about that @robert-zaremba you can see it was merged moments before you wrote your comments. Anyway probably better to put interoperability conversation into another thread dedicated to it—maybe the ICS.

[robert-zaremba]

No problem @okwme. The ADR is not finalized, so it's fine to have "checkpoints" and move forward.
I would like to make sure that we clarify the interoperability. We already have a NFT discussion, I will start a thread there - it's related to the x/nft module.