Support ClassData and TokenData not according to ICS-721 spec

[yito88]

Description

• Custom (de)serialization for ClassUri, ClassData, TokenUris, and TokenData to decode null
• Handle Data as just String if ics721-data feature flag isn't set Allowing to check if the Data is in ICS-721 structure
  • Currently, go implementation doesn't care about the JSON structure in the data, e.g. cw-ics721 implementation uses its own structure
  • (I have some uncertainty about whether using a feature flag is the optimal approach for toggling the data. If you have any alternative suggestions or insights, I would greatly appreciate your input.)
• Change functions and some arguments for ClassData and TokenData to use Option because URI and data (ClassData, TokenData) could not exist
• Check the length of TokenUris and TokenData in send_nft_transfer_validate()

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests.
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).
• Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/ics721-impl@3c0fddd). Click here to learn what that means.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             feat/ics721-impl    #1039   +/-   ##
===================================================
  Coverage                    ?   67.25%           
===================================================
  Files                       ?      196           
  Lines                       ?    20275           
  Branches                    ?        0           
===================================================
  Hits                        ?    13635           
  Misses                      ?     6640           
  Partials                    ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Farhad-Shabani]

Thank you @yito88 for tracking down the Data format issue. 🙏🏻

Handle Data as just String if ics721-data feature flag isn't set

I could come up with another approach for handling any Data format, allowing us to check if the Data is in ICS-721 structure. We shouldn't need to define a feature flag.
Please check out this branch and apply if that makes sense to you. I couldn’t open another PR to your branch.

Change functions and some arguments for ClassData and TokenData to use Option because URI and data (ClassData, TokenData) could not exist

Shouldn't the get_* methods under the NftContext and NftClassContext always return some values? Is it viable for instance we mint a token by only having a pair of class and token ids without any URIs and data?

Similar question applies to the create_or_update_class_execute and mint_nft_execute methods. Do you handle e.g. the TokenUri and TokenData creation locally as the host specifies, or won't you need them at all?

Check the length of TokenUris and TokenData in send_nft_transfer_validate()

Let's define a validate_basic() method for the PacketData so can call that in places we need similar checks. I've showcased that in this branch.

[yito88]

@Farhad-Shabani Thank you! I merged your commits for the data format.
I need to clarify the ICS-721 structure (In a discussion, the structure might be just a recommended one). However, the check function will be helpful.

Shouldn't the get_* methods under the NftContext and NftClassContext always return some values? Is it viable for instance we mint a token by only having a pair of class and token ids without any URIs and data?

Similar question applies to the create_or_update_class_execute and mint_nft_execute methods. Do you handle e.g. the TokenUri and TokenData creation locally as the host specifies, or won't you need them at all?

Technically, we can mint a token only with class and token IDs. In my testing, some optional data were empty. I'm testing actual chains.

[Farhad-Shabani]

I need to clarify the ICS-721 structure (In a discussion, the structure might be just a recommended one). However, the check function will be helpful.

That's appreciated. Thus, I will be waiting for your confirmation before we proceed with this PR.

Technically, we can mint a token only with class and token IDs. In my testing, some optional data were empty. I'm testing actual chains.

Thanks! Good to know this. Let me know plz the actual test results as well.

[Farhad-Shabani]

Just left a comment and please fix the CI catch with the cargo doc, then good to go. 🎉 Thank you @yito88 for implemeting the custom (de)serialization.

[Farhad-Shabani]

Let's keep these TryFrom and From conversions as they should work fine. IIUC, we hit the “null” issue only if someone tries to obtain PacketData proto type from a JSON string. And, this actually stems from the fact that the .proto definitions and hence the Go compiler allow nullable fields, which shouldn't be the case. I noticed that ibc-go handles this by adding (gogoproto.nullable) = false to their .proto files.

Actually, I am thinking of keeping the nft-transfer types' crate complete in terms of providing needed conversions, in case, any other implementors need it.