Questions about NonFungiblePacketData

[yito88]

I'm working on ICS-721 implementation in ibc-rs. I face two issues with decoding NonFungiblePacketData.

First, NonFungiblePacketData has Optional for some members. It causes null value in the packet data. Is it expected?

"{\"classId\":\"wasm14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s0phg4d\",\"classUri\":null,\"classData\":\"eyJvd25lciI6Indhc20xNHpwMzVhdzl3Y256dGxoazUycGpjbHI2enJ2bXJja3pldXJhNWMiLCJjb250cmFjdF9pbmZvIjp7ImNvZGVfaWQiOjEsImNyZWF0b3IiOiJ3YXNtMTR6cDM1YXc5d2NuenRsaGs1MnBqY2xyNnpydm1yY2t6ZXVyYTVjIiwiYWRtaW4iOm51bGwsInBpbm5lZCI6ZmFsc2UsImliY19wb3J0IjpudWxsfSwibmFtZSI6InRlc3RfbmZ0Iiwic3ltYm9sIjoiVEVTVF9ORlQiLCJudW1fdG9rZW5zIjoxfQ==\",\"tokenIds\":[\"test_nft_0\"],\"tokenUris\":[\"http://example.com\"],\"tokenData\":null,\"sender\":\"wasm14zp35aw9wcnztlhk52pjclr6zrvmrckzeura5c\",\"receiver\":\"tnam1qq485cszjczr3379p4tme4u94c8q3la04gne5ft3\",\"memo\":null}"

In go implementation, the packet data doesn't have an Optional field. Because ibc-rs reuses this definition, the decoding packet data failed. When adding #[serde(skip_serializing_if = "Option::is_none")] to the optional members, no null appeared.

Second, shouldn't the data structure compatible with ICS-721 spec? Decoding the above classData in the packet from the contract:

{
  "owner": "wasm1ejs8u53du38wy2ukrpf00nv9r7539w9e6ryfn3",
  "contract_info": {
    "code_id": 1,
    "creator": "wasm1ejs8u53du38wy2ukrpf00nv9r7539w9e6ryfn3",
    "admin": null,
    "pinned": false,
    "ibc_port": null
  },
  "name": "test_nft",
  "symbol": "TEST_NFT",
  "num_tokens": 1
}

ICS-721 spec says:

Both tokenData entries and classData MUST be Base64 encoded strings which SHOULD have the following JSON structure:

{
"key1" : { "value":"...", "mime":"..." },
"key2" : { "value":"...", "mime":"..." },
...
}

Please correct me if I misinterpret it. Thanks!

[humanalgorithm]

@yito88 for the data structure of NonFungiblePacketData, please take a look at the cw-ics721 implementation as an example. You can see which fields have been made optional: https://github.com/public-awesome/cw-ics721/blob/main/packages/ics721-types/src/ibc_types.rs#L11C1-L43C1

[taitruong]

Hi @yito88, we had same discussion with IRISnet team (who does the module implementation for ics721).

In CosmWasm serialialization (using serde) handles Option like this: in case of None, property is left out in JSON. Example:

Example 1 with no memo

NonFungibleTokenPacketData {
    class_id: ClassId::new("id"),
    memo: None,
    ...
};

JSON output:

{
    "class_id": "id",
    ...
}

Example 2 with memo

NonFungibleTokenPacketData {
    class_id: ClassId::new("id"),
    memo: Some("foo"),
    ... //
};

JSON output:

{
    "class_id": "id",
    "memo": "foo",
    ...
}

[taitruong]

Regarding spec, here as well, we pointed this out with IRISnet some time ago. It got updated further below:

classData is an optional field which, if present, MUST be non-empty and contain on-chain class metadata such as royalty related parameters.
...
tokenData array is an optional field which, if present, MUST have the same size as tokenIds and hold non-empty entries each of which contains on-chain application data associated with the token identified by the corresponding tokenIds entry.

[taitruong]

Feel free to re-open if you have further questions.

[yito88]

@taitruong Thank you!

In CosmWasm serialialization (using serde) handles Option like this: in case of None, property is left out in JSON.

I tried to receive the packet from CosmWasm with ics721 contract. The packet had null values as I pasted above. Is that the expected behavior? I had to add #[serde(skip_serializing_if = "Option::is_none")] in the contract code to remove the null.

@@ -14,9 +14,11 @@ pub struct NonFungibleTokenPacketData {
     pub class_id: ClassId,
     /// Optional URL that points to metadata about the
     /// collection. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub class_uri: Option<String>,
     /// Optional base64 encoded field which contains on-chain metadata
     /// about the NFT class. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub class_data: Option<Binary>,
     /// Uniquely identifies the tokens in the NFT collection being
     /// transfered. This MUST be non-empty.
@@ -25,11 +27,13 @@ pub struct NonFungibleTokenPacketData {
     /// transfered. `tokenUris[N]` should hold the metadata for
     /// `tokenIds[N]` and both lists should have the same if
     /// provided. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token_uris: Option<Vec<String>>,
     /// Optional base64 encoded metadata for the tokens being
     /// transfered. `tokenData[N]` should hold metadata for
     /// `tokenIds[N]` and both lists should have the same length if
     /// provided. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token_data: Option<Vec<Binary>>,

     /// The address sending the tokens on the sending chain.
@@ -38,6 +42,7 @@ pub struct NonFungibleTokenPacketData {
     /// chain.
     pub receiver: String,
     /// Memo to add custom string to the msg
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub memo: Option<String>,
 }

[yito88]

Regarding spec, here as well, we pointed this out with IRISnet some time ago. It got updated further below:

I'm wondering why the JSON structure isn't following ICS-721 spec. In my understanding, each key should have a map {"value": ..., "mime": ...} or {"value": ...}.

An example in the spec:

{
  "opensea:name" : { "value":"Crypto Creatures" },
  "opensea:image" : { "value":"...(Base64 encoded media binary)", "mime":"image/png" },
  "opensea:seller_fee_basis_points" : { "value":"100" }
}

[yito88]

Additional question: Have you tried to transfer an NFT between CosmWasm with cw-ics721 and another chain with the go implementation when some optional fields are empty?

go implementation (and ibc-rs) would make a packet with an empty string ("") or vector ([]) for the optional data when the data doesn't exist. ics721 contract couldn't decode this packet because the validation checks if the field is not empty when the field isn't None. (The following processes assume that they're not empty)

To confirm this issue, when padding dummy data for the optional fields, NFT transfer from Namada(w/ ibc-rs) to CosmWasm w/ cw-ics721 worked.

[taitruong]

@taitruong Thank you!

In CosmWasm serialialization (using serde) handles Option like this: in case of None, property is left out in JSON.

I tried to receive the packet from CosmWasm with ics721 contract. The packet had null values as I pasted above. Is that the expected behavior? I had to add #[serde(skip_serializing_if = "Option::is_none")] in the contract code to remove the null.

@@ -14,9 +14,11 @@ pub struct NonFungibleTokenPacketData {
     pub class_id: ClassId,
     /// Optional URL that points to metadata about the
     /// collection. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub class_uri: Option<String>,
     /// Optional base64 encoded field which contains on-chain metadata
     /// about the NFT class. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub class_data: Option<Binary>,
     /// Uniquely identifies the tokens in the NFT collection being
     /// transfered. This MUST be non-empty.
@@ -25,11 +27,13 @@ pub struct NonFungibleTokenPacketData {
     /// transfered. `tokenUris[N]` should hold the metadata for
     /// `tokenIds[N]` and both lists should have the same if
     /// provided. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token_uris: Option<Vec<String>>,
     /// Optional base64 encoded metadata for the tokens being
     /// transfered. `tokenData[N]` should hold metadata for
     /// `tokenIds[N]` and both lists should have the same length if
     /// provided. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token_data: Option<Vec<Binary>>,

     /// The address sending the tokens on the sending chain.
@@ -38,6 +42,7 @@ pub struct NonFungibleTokenPacketData {
     /// chain.
     pub receiver: String,
     /// Memo to add custom string to the msg
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub memo: Option<String>,
 }

serde handles it properly. So if serialization outputs null, on de-serialization it is sett to null. Put we can look into this. Please create another issue for this.

[taitruong]

Regarding spec, here as well, we pointed this out with IRISnet some time ago. It got updated further below:

I'm wondering why the JSON structure isn't following ICS-721 spec. In my understanding, each key should have a map {"value": ..., "mime": ...} or {"value": ...}.

An example in the spec:

{
  "opensea:name" : { "value":"Crypto Creatures" },
  "opensea:image" : { "value":"...(Base64 encoded media binary)", "mime":"image/png" },
  "opensea:seller_fee_basis_points" : { "value":"100" }
}

This is prob a "copy-paste" error taken from ERC721. For clarification pls create an issue for spec in cosmos repo.

[taitruong]

Additional question: Have you tried to transfer an NFT between CosmWasm with cw-ics721 and another chain with the go implementation when some optional fields are empty?

go implementation (and ibc-rs) would make a packet with an empty string ("") or vector ([]) for the optional data when the data doesn't exist. ics721 contract couldn't decode this packet because the validation checks if the field is not empty when the field isn't None. (The following processes assume that they're not empty)

To confirm this issue, when padding dummy data for the optional fields, NFT transfer from Namada(w/ ibc-rs) to CosmWasm w/ cw-ics721 worked.

We have tested it numerous times with nft transfer module together with IRISnet. Also during Game of NFTs.

[taitruong]

off-topic: what is the use case for ibc-rs? can you join our discord and we may chat about this? thx invite: https://discord.gg/fVv6Mf9Wr8 my handle: mr_t__

[humanalgorithm]

Let me add a clarifying point here to clear up confusion potentially:

1) This is the recommended structure for ClassData in the spec

 {
  "key1" : { "value":"...", "mime":"..." },
  "key2" : { "value":"...", "mime":"..." },
}

2) ClassData is designed as a variable field that can have any structure. So the structure you are seeing is technically valid as long as its a JSON

3) We have not fully implemented the per-chain unpacking of these key/values in an implementation yet. There was some discussion about putting royalty data and/or on-chain metadata into these fields, and then adding a module in ics721 that knows how to unpack these key/values when its coming from a specific chain. So for example, on-chain data coming from Terra might have structure A and on-chain data coming from Juno might have structure B. Then we would need modules that know how to unpack these data on the other side. We have not done an instance of this per-chain packing/unpacking.

The flexible ClassData structure however, allows for this to be coded out in the future.

[yito88]

@taitruong @humanalgorithm Thank you for clarifying them. I'll open issues.

I checked IRISnet code. It seems that it sets all optional data.

Could you please tell me how to set all optional data in Stargaze testnet for testing? I tried to mint an NFT on Stargaze testnet according to here. However,ClassUri, ClassData, and TokenData were None.