[Move] Add module for creating example NFT on CLI #1411

666lcz · 2022-04-17T20:51:07Z

Wallet CLI will be the primary interface for users to perform "writes" on Sui at the Devnet launch, therefore we want to provide an easy way for the users to create an NFT-like rich object with image and attributes.

This PR is the first step for #1400, which defines a Move module for a minimalist NFT. The next PR #1412 will implement the CLI interface that will invoke this Move module.

Considerations

ExampleNFT does not have any restrictions or supply cap because we want anybody to be able to easily create a rich object and view it on Sui Explorer through the wallet CLI
This PR defines a TokenMetadata module that can be reused by any Move module that wants to implement an NFT-like object.

666lcz · 2022-04-17T21:47:23Z

sui_programmability/framework/sources/ExampleNFT.move

+        id: VersionedID,
+        /// The metadata associated with this NFT
+        metadata: TokenMetadata,
+        // TODO: allow custom attributes


In general, there are two patterns of defining custom attributes onchain:

as hardcoded struct fields, for example

struct ExampleNFT has key, store { id: VersionedID, metadata: TokenMetadata, level: u64, color: String, }

The primary advantage of this approach is that it's explicit and the properties can be easily read/write by onchain programs. The downside is that it's hard to add new attribute types once the contract is deployed(before Sui support contract update).

as a vector of key value pair

struct ExampleNFT has key, store { id: VersionedID, metadata: TokenMetadata, dynamic_attributes: vector<string, string>, }

where dynamic_attributes can be [('level', '10'), ('color', 'blue')]. Imagine a game company wants to add a new feature to the game by adding a rarity attribute to all existing NFTs, it's possible to do it in approach 2(e.g., the game company can mint a Gem that has trait = "rarity"; value = "legendary" and transfer to the NFT owner, the NFT owner can then invoke the upgrade(gem, nft, ctx) method that will add ("rarity", "legendary") to dynamic_attributes). Adding new attributes in approach 1 is not possible until Sui supports contract update.

Some projects might want to use both pattern so that they can define most attributes in approach 1 but also leverage approach 2 to support future updates. The Sui Explorer should support both patterns.

For ExampleNFT, since it's meant to be a general NFT-like object rather than a specific item like a bored ape, I lean towards supporting custom attributes using Approach 2 where the Command Line Tool users can define there own custom attributes if they want.

Let me know if you have any thoughts cc @sblackshear @lxfind @tnowacki

Based on Friday's discussions, I don't think we would do something exactly like (2) in Move. (though we might have some sort of immutable blob of JSON data).

If you want to make this ExampleNFT have custom attributes you could use generics:

struct ExampleNFT<Attributes: store> has key, store { ... attributes: Attributes }

And then you can do

struct MyRPGItemAttributes has store { level: u64, color: String, rarity: String, ... } ... ExampleNFT<MyRPGItemAttributes>

Though I'm not sure if this exactly what you want for this example

@tnowacki I agree that approach 1 is preferred, but in the case that I mentioned above where creators(e.g., games) want to add new attribute types after a contract is deployed, they have to use approach 2(at lease before Sui supports contract update). And there is nothing that can stop them from doing so.

I see... Yeah I then we might need to define some sort of immutable JSON blob...
If Move had maps that would be very helpful :P

Yes, map would be really helpful here. But if I need to choose between a vector<string, string> and an immutable JSON string, I will prefer the former because it's easier to do on-chain operations such as add/remove/count/read. And we know that a vector is good enough because opensea is using a vector for the definition of attributes.

You will need to do something like vector<StringMapping> and struct StringMapping { key: string, value: string } or something similarly annoying

And we know that a vector is good enough because opensea is using a vector for the definition of attributes.

And it is "good enough", but our hope is to do better :)

My point was that vector<(string, string)>(not sure if you can put tuples in a vec, if not then vector<StringMapping>) is better than an immutable JSON string. Let me know if you have better alternatives in mind.

tnowacki · 2022-04-18T19:52:41Z

sui_programmability/framework/sources/ExampleNFT.move

+        id: VersionedID,
+        /// The metadata associated with this NFT
+        metadata: TokenMetadata,
+        // TODO: allow custom attributes


Based on Friday's discussions, I don't think we would do something exactly like (2) in Move. (though we might have some sort of immutable blob of JSON data).

If you want to make this ExampleNFT have custom attributes you could use generics:

struct ExampleNFT<Attributes: store> has key, store { ... attributes: Attributes }

And then you can do

struct MyRPGItemAttributes has store { level: u64, color: String, rarity: String, ... } ... ExampleNFT<MyRPGItemAttributes>

Though I'm not sure if this exactly what you want for this example

tnowacki · 2022-04-18T19:54:21Z

sui_programmability/framework/sources/TokenMetadata.move

+/// Metadata standard for fungible and non-fungible tokens on Sui
+module Sui::TokenMetadata {
+    use Sui::Url::{Self, Url};
+    use Sui::UTF8;
+    use Std::Vector;
+
+    struct TokenMetadata has store {
+        /// Name for the token


I don't think we want a separate struct like this.

I think we want to allow any sort of composition for these fields. That is, you can add name: String, description: String, and urls: vector<Url> to your struct and it will get rendered properly. You can then include some, none, or all of these combos

I don't think we gain too much from having this separate struct? (other than maybe discoverability of these field combos)

Yeah I thought about this but I do think it's worth having a separate struct with the following advantages:

less code

An application developer can write 3 lines of code instead of 9~12 lines of code(see inline comment). Moreover, imagine a module that has multiple NFT-like structs, They have to repeat name, description, urls in the struct definition and initializations for each struct. Chunking these fields in a standalone struct is much cleaner.

use Sui::TokenMetadata::{Self, TokenMetadata}; metadata: TokenMetadata, metadata: TokenMetadata::new(name, description, url)

use Sui::Url::{Self, Url}; use Sui::UTF8; use Std::Vector; name: UTF8::String, description: UTF8::String, urls: vector<Url>, name: UTF8::string_unsafe(name), description: UTF8::string_unsafe(description), // very likely we will need more when we support multiple urls at initialization urls: Vector::singleton<Url>(Url::new_from_bytes_unsafe(url)),

Less error-prone

What if the developers do not read our documentation carefully, and ended up using url instead of urls or use ASCII::String instead of UTF8::String in the definition? Why should we require them to figure out the difference between ASCII::String and UTF8::String when all they want to do is pass some kind of string in a Hackathon?

Easier for application rendering

With the struct, the applications can just do object.metadata to render an object
Without the struct, the applications have to do if 'name' in object; if 'description' in object; if 'urls' in object.

These three fields are more commonly used together than separately, as evident in all other metadata standard(e.g., metaplex, opensea). Therefore putting them in a struct makes sense instead of allowing a combo. If the application wants to opt-out, they can pass in an empty string/vector.

For ASCII/UTF8 string, we will support both

For url/urls we will support both (but we might also have linters), (we are supporting both for the reason of many vs multiple)

Easier for application rendering
An application developer can write 3 lines of code instead of 9~12 lines of code(see inline comment). Moreover, imagine a module that has multiple NFT-like structs, They have to repeat name, description, urls in the struct definition and initializations for each struct

My worry for both of these is we are going to have more meta data fields, other than just these 3. We are going to have a whole bag of optional "meta data" fields, and we want some generic way of annotating meta data fields vs attribute fields. I'm not quite sure this struct approach is flexible for an extensible API.

Got it, I was thinking having all the essential fields(most likely just these 3) in this TokenMetada struct, and put all the optional ones inline with the NFT definition(instead of in the TokenMetadata). That way we can have the best of both worlds and don't need to force people to write 9~12 lines of code or more instead of 3 lines of code.

For ASCII/UTF8 string, we will support both

For url/urls we will support both (but we might also have linters), (we are supporting both for the reason of many vs multiple)

It's not just if we decide to support or not. This will also put a burden on applications where they now need to do if 'urls' in object; else if 'url' in object.

I was thinking having all the essential fields(most likely just these 3) in this TokenMetada struct, and put all the optional ones inline with the NFT definition(instead of in the TokenMetadata).

We could do that sure. I just worry about the complexity it will add for tooling. I don't think having the struct is that much more ergonomic for the programmer than the 3 fields, as it adds a layer of indirection when packing(creating), unpacking(pattern matching), reading, writing (mabye that last one won't happen though).

It also makes the story around the field+type name thing a lot more complicated.

My hope would be something simple like:

Known field+type combos are specially

For fields not known by the explorer

Any field with a leading _, e.g. _foo: u64, is a meta data field

All other fields are "attribute" or "property" fields

It's not just if we decide to support or not. This will also put a burden on applications where they now need to do if 'urls' in object; else if 'url' in object.

This is a very good point. But I don't think it will just be solved by the TokenMetadata case.
Specifically if end up with cases such as url: Url vs url: VerifiedUrl where VerifiedUrl is a Url + hash (@kchalkias was talking about this maybe being necessary)

At any rate, it might be helpful to have a tool that takes the JSON for a Sui Move object and canonicalizes it based on the rules/format the Sui explorer expects (bucketing these major fields like name/description vs attributes vs misc. metadata)

666lcz · 2022-04-19T00:52:37Z

@tnowacki , thanks for the discussion. I've removed the TokenMetadata module, let me know if you have any other concern.

codecov · 2022-04-19T01:29:18Z

Codecov Report

Merging #1411 (f77775b) into main (c1e46f0) will increase coverage by 0.22%.
The diff coverage is n/a.

❗ Current head f77775b differs from pull request most recent head 800dcd9. Consider uploading reports for the commit 800dcd9 to get more accurate results

@@            Coverage Diff             @@
##             main    #1411      +/-   ##
==========================================
+ Coverage   82.19%   82.41%   +0.22%     
==========================================
  Files         104      103       -1     
  Lines       21145    21069      -76     
==========================================
- Hits        17380    17364      -16     
+ Misses       3765     3705      -60

Impacted Files	Coverage Δ
sui/src/keystore.rs	`54.68% <0.00%> (-6.13%)`	⬇️
sui/src/unit_tests/cli_tests.rs	`89.52% <0.00%> (ø)`
sui/src/unit_tests/sui_network.rs	`100.00% <0.00%> (ø)`
sui_core/src/authority/temporary_store.rs	`83.52% <0.00%> (ø)`
sui_core/src/unit_tests/move_integration_tests.rs	`93.33% <0.00%> (ø)`
sui/src/validator.rs
sui/src/sui_commands.rs	`76.10% <0.00%> (+0.18%)`	⬆️
sui_types/src/object.rs	`81.74% <0.00%> (+0.20%)`	⬆️
sui/src/config.rs	`87.23% <0.00%> (+4.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6762c9...800dcd9. Read the comment docs.

tnowacki · 2022-04-20T17:33:23Z

sui_programmability/framework/sources/Url.move

@@ -36,6 +36,13 @@ module Sui::Url {
        Url { url }
    }

+    /// Create a `Url` with no validation from bytes
+    /// Note: this will abort if `bytes` is not valid ASCII
+    public fun new_from_bytes_unsafe(bytes: vector<u8>): Url {


I think this should be new_unsafe_from_bytes

damirka

IMO we don't want to ship it as a part of our framework. Better place would be examples.

666lcz · 2022-04-20T17:37:39Z

@damirka , we want this to be part of the framework at least for DevNet because we intend to provide an easy way for anyone to create a rich object. Otherwise we have to deal with publishing the module manually every time the network is redeployed

damirka

Sorry, you are right. The best way would be to add to the framework. LGTM!

666lcz · 2022-04-21T19:22:13Z

Will handle #1527 in a separate PR

666lcz · 2022-04-21T19:22:13Z

Will handle #1527 in a separate PR

666lcz marked this pull request as ready for review April 17, 2022 21:07

666lcz requested review from oxade, sblackshear, awelc, damirka and lxfind April 17, 2022 21:07

666lcz commented Apr 17, 2022

View reviewed changes

666lcz requested a review from tnowacki April 17, 2022 21:47

tnowacki reviewed Apr 18, 2022

View reviewed changes

tnowacki reviewed Apr 20, 2022

View reviewed changes

damirka requested changes Apr 20, 2022

View reviewed changes

666lcz requested a review from damirka April 20, 2022 17:37

tnowacki approved these changes Apr 20, 2022

View reviewed changes

damirka approved these changes Apr 20, 2022

View reviewed changes

666lcz and others added 4 commits April 21, 2022 12:37

[Move] Add module for creating example NFT on CLI

2d4fea9

Address comment to remove the TokenMetadata struct

ef6e258

ExampleNFT -> DevNetNFT

ea04977

[Wallet CLI] Add command to create example NFT (#1412)

800dcd9

666lcz force-pushed the chris/example-nft branch from 05002bc to 800dcd9 Compare April 21, 2022 19:45

666lcz enabled auto-merge (squash) April 21, 2022 19:46

666lcz merged commit 1030d1e into main Apr 21, 2022

666lcz deleted the chris/example-nft branch April 21, 2022 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Move] Add module for creating example NFT on CLI #1411

[Move] Add module for creating example NFT on CLI #1411

666lcz commented Apr 17, 2022 •

edited

Loading

666lcz Apr 17, 2022 •

edited

Loading

tnowacki Apr 18, 2022

666lcz Apr 18, 2022 •

edited

Loading

tnowacki Apr 18, 2022

666lcz Apr 18, 2022 •

edited

Loading

tnowacki Apr 18, 2022

666lcz Apr 18, 2022 •

edited

Loading

tnowacki Apr 18, 2022

tnowacki Apr 18, 2022

666lcz Apr 18, 2022

tnowacki Apr 18, 2022

666lcz Apr 18, 2022 •

edited

Loading

tnowacki Apr 18, 2022

tnowacki Apr 18, 2022

666lcz commented Apr 19, 2022

codecov bot commented Apr 19, 2022 •

edited

Loading

tnowacki Apr 20, 2022

damirka left a comment

666lcz commented Apr 20, 2022

damirka left a comment

666lcz commented Apr 21, 2022

666lcz commented Apr 21, 2022

[Move] Add module for creating example NFT on CLI #1411

[Move] Add module for creating example NFT on CLI #1411

Conversation

666lcz commented Apr 17, 2022 • edited Loading

Considerations

666lcz Apr 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz commented Apr 19, 2022

codecov bot commented Apr 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

damirka left a comment

Choose a reason for hiding this comment

666lcz commented Apr 20, 2022

damirka left a comment

Choose a reason for hiding this comment

666lcz commented Apr 21, 2022

666lcz commented Apr 21, 2022

666lcz commented Apr 17, 2022 •

edited

Loading

666lcz Apr 17, 2022 •

edited

Loading

666lcz Apr 18, 2022 •

edited

Loading

666lcz Apr 18, 2022 •

edited

Loading

666lcz Apr 18, 2022 •

edited

Loading

666lcz Apr 18, 2022 •

edited

Loading

codecov bot commented Apr 19, 2022 •

edited

Loading