[framework] Display object module #7668

damirka · 2023-01-25T01:16:54Z

Full proposal text is here: https://forums.sui.io/t/nft-object-display-proposal/4872
Adds the sui::display module which allows setting a display for the T by the publisher of the T

vercel · 2023-01-25T01:17:02Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 8:14PM (UTC)
frenemies	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 8:14PM (UTC)
wallet-adapter	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 8:14PM (UTC)

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
explorer-storybook	⬜️ Ignored (Inspect)			Feb 22, 2023 at 8:14PM (UTC)

crates/sui-framework/sources/display.move

sblackshear · 2023-01-25T04:20:02Z

crates/sui-framework/sources/display.move

+    /// we don't need to perform an authorization check.
+    ///
+    /// Also, the only place it can be used is the function where the Display
+    /// object was created; hence values and names are likely to be hardcoded and


I think I understand what this is saying (Move doesn't currently allow string constants and thus wannabe string constants have type vector<u8>), but I think asking for strings is clearer/less error-prone here--a UTF8 decoding error inside this function would be puzzling.

Don't we now allow String on entry functions?

I agree though that we probably want to make these functions all take String instead of vector<u8> to avoid confusion.

I noticed this has been merged with vector<u8>. Are we still planning to change this to String?

crates/sui-framework/sources/display.move

sblackshear · 2023-01-25T04:28:37Z

crates/sui-framework/sources/display.move

+    }
+
+    /// Create an empty Display object. It can either be shared empty of filled
+    /// with data right away via cheaper `set_owned` method.


Is there also a case where we'd want Display to be single-owner or immutable rather than shared?

Intuitively, it seems like a single-owner display owned by the address that has the Publisher cap would be the most common--you want anyone to be able to read the Display info, but no one else needs to update it or read it on-chain.

crates/sui-framework/sources/display.move

sblackshear · 2023-01-25T04:40:22Z

crates/sui-framework/sources/display.move

+
+    /// Create an empty Display object. It can either be shared empty of filled
+    /// with data right away via cheaper `set_owned` method.
+    public fun empty<T: key>(pub: &Publisher, ctx: &mut TxContext): Display<T> {


Should this specify that it should be called at most once for a given type T (and explain what happens if that assumption is violated)?

That's a good one! I have an idea of creating a centralized registry of displays for the sake of discoverability and also uniqueness. We might handle versions there as well.

Another benefit we can get from the centralization is the ability to destroy older versions of the display and get a storage rebate (as opposed to shared objects which are not deletable yet).

Can you update the comment to explain the intention?

crates/sui-framework/sources/display.move

tnowacki · 2023-01-25T19:36:06Z

crates/sui-framework/sources/display.move

+    /// we don't need to perform an authorization check.
+    ///
+    /// Also, the only place it can be used is the function where the Display
+    /// object was created; hence values and names are likely to be hardcoded and


Don't we now allow String on entry functions?

I agree though that we probably want to make these functions all take String instead of vector<u8> to avoid confusion.

crates/sui-framework/sources/display.move

Suficio · 2023-02-07T12:04:43Z

This sounds like a great initiative and is a big step forward for display standards in SUI!

I would like to structure my feedback in two, how this structure affects OriginByte NFTs and perhaps proposing an alternative approach.

Displaying OriginByte NFTs thus far works based on DisplayDomain which contains the bare name and description fields. There are additional domains such as UrlDomain which allow more composability of display properties.

A collection using OB NFTs does not have to use these domains and may opt to instead provide its own methods of storing display information. Since domains are indexed by type, not string, and are dynamic objects, I'm not sure if this proposal will allow for indexing into them like so: {DisplayDomain.name}. This is not a deal-breaker as we can always provide these fields in the core Nft type.

In the same vein, will dynamic fields with string keys be supported for this indexing?

As I mentioned initially, we would like to propose an alternative approach.

Since Display in essence provides a struct that provides a "view" into the object, here at OriginByte we were wondering if there are alternative ways to make that happen.

An initial solution that comes to mind is allowing entry functions (or view functions) to return view structs that contain fields otherwise available in the Display VecMap. For OriginByte, this would allow collection creators to define such a view function that would serialize all of their fields of interest. In a more general view it enables creators more flexibility to implement just in time transformation of their data.

These view functions would run immutably and be limited in complexity, and be run on the same module that would otherwise populate the template fields in Display<T>.

damirka · 2023-02-17T13:16:37Z

Hey @Suficio! Thank you for your feedback. I've replied on the Sui Developers (link to the reply).

Hopefully this answers all of the topics raised in your question, please feel free to disagree or ask if anything is unclear.

Co-authored-by: Sam Blackshear <[email protected]>

sblackshear

Suggesting changes to add a version field + more tests, but otherwise LGTM!

sblackshear

LGTM pending tests!

crates/sui-framework/sources/display.move

666lcz · 2023-02-26T05:33:46Z

crates/sui-framework/sources/display.move

+    /// we don't need to perform an authorization check.
+    ///
+    /// Also, the only place it can be used is the function where the Display
+    /// object was created; hence values and names are likely to be hardcoded and


I noticed this has been merged with vector<u8>. Are we still planning to change this to String?

666lcz · 2023-02-26T05:35:07Z

crates/sui-framework/sources/display.move

+        let i = 0;
+        while (i < 0) {


Uh this should be while (i < len)

Good catch! Thank you!

## Description This PR adds the RPC support for #7668 and added e2e TS tests Todos(in separate PRs) - [ ] merge the display endpoint into the[ new queryObjects API](https://www.notion.so/mystenlabs/RFC-GetObjects-API-Refactoring-27ff6b8d6ffc42c8b492c1efa368b70a?pvs=4) - [ ] Handle struct that has no Display defined `Overall display should be treated as Debug and Display traits in Rust. If we don’t have Display defined, we show default debug (on the FE/Apps), if there’s Display, we show things nicely.` I'll implement this when we implement the new queryObjects API - [ ] Add rust based tests ## Test Plan How did you test the new or updated feature? Added e2e TS tests to cover the following test cases - Escape syntax (e.g., `\{name\}` should return `{name}` instead of `Alice` even if there's a field called Alice) - Nested fields - multiple fields - option::some and option::none - no template value - sui address - UID - `sui::url::Url` type --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes

jovicheng · 2023-11-28T10:01:45Z

@damirka Hi, is there any update regarding template syntax (df or dynamic_field_key)?

damirka · 2023-11-28T10:23:25Z

Hey @jovicheng. We're at full capacity already; more updates will be coming next year. But yes, df and vectors and a lot more will be there!

jovicheng · 2023-11-28T10:36:33Z

Hey @jovicheng. We're at full capacity already; more updates will be coming next year. But yes, df and vectors and a lot more will be there!

Thank you for your reply. I would like to confirm again, currently, this feature is not supported, correct?

damirka · 2023-11-28T10:55:21Z

@jovicheng

I would like to confirm again, currently, this feature is not supported, correct?

Correct, you can't fetch dynamic fields just yet.

jovicheng · 2023-11-28T11:03:48Z

@jovicheng

I would like to confirm again, currently, this feature is not supported, correct?

Correct, you can't fetch dynamic fields just yet.

thanks, looking forward to this new features."

damirka added 10 commits January 6, 2023 21:08

Adds publisher module

a06b03a

display experiment

db7da3c

adds is_package check

ef274a2

uses VecMap instead

893f2e8

fix comment

aa786ae

Merge branch 'main' into ds/publisher-cap

a851549

adds std::address & improves std::type_name

0a20760

Merge branch 'ds/publisher-cap' into ds/display-metadata

8f768aa

Merge branch 'main' into ds/display-metadata

4ac7e2c

adds display module

cf90cb2

damirka added the move label Jan 25, 2023

damirka self-assigned this Jan 25, 2023

fix docs

aeabbce

sblackshear reviewed Jan 25, 2023

View reviewed changes

fix iterators

31f42fe

tnowacki reviewed Jan 25, 2023

View reviewed changes

oxade self-requested a review January 26, 2023 12:50

damirka mentioned this pull request Jan 31, 2023

[JSONRPC] - Use a more structured dynamic field name instead of MoveValue::to_string #7318

Merged

damirka marked this pull request as ready for review February 17, 2023 13:24

damirka requested review from randall-Mysten and ronny-mysten as code owners February 17, 2023 13:24

damirka and others added 3 commits February 17, 2023 16:52

Update crates/sui-framework/sources/display.move

905d236

Co-authored-by: Sam Blackshear <[email protected]>

post review fixes + clarifications

15c925d

Merge branch 'main' into ds/display-metadata

0dae8e1

vercel bot temporarily deployed to Preview – wallet-adapter February 17, 2023 14:08 Inactive

vercel bot deployed to Preview – explorer February 17, 2023 14:08 View deployment

vercel bot temporarily deployed to Preview – frenemies February 17, 2023 14:09 Inactive

damirka changed the title ~~[wip] Display object module~~ [framework] Display object module Feb 22, 2023

sblackshear requested changes Feb 22, 2023

View reviewed changes

post-review + edit / add

861efae

damirka requested a review from sblackshear February 22, 2023 17:25

damirka added 2 commits February 22, 2023 20:31

adds version

3143586

adds update_version hook + emits an event with the update

cfcc2d6

sblackshear approved these changes Feb 22, 2023

View reviewed changes

crates/sui-framework/sources/display.move Outdated Show resolved Hide resolved

change u64 to u16

b9e327a

damirka enabled auto-merge (squash) February 22, 2023 18:12

Merge branch 'main' into ds/display-metadata

4b0a1f3

vercel bot temporarily deployed to Preview – wallet-adapter February 22, 2023 20:13 Inactive

vercel bot deployed to Preview – explorer February 22, 2023 20:13 View deployment

vercel bot temporarily deployed to Preview – frenemies February 22, 2023 20:14 Inactive

damirka merged commit 02fead9 into main Feb 22, 2023

damirka deleted the ds/display-metadata branch February 22, 2023 20:48

666lcz reviewed Feb 26, 2023

View reviewed changes

666lcz mentioned this pull request Feb 26, 2023

[Display] RPC support for fetching and rendering Display object #8663

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[framework] Display object module #7668

[framework] Display object module #7668

damirka commented Jan 25, 2023 •

edited

Loading

vercel bot commented Jan 25, 2023 •

edited

Loading

sblackshear Jan 25, 2023

tnowacki Jan 25, 2023

666lcz Feb 26, 2023

sblackshear Jan 25, 2023

sblackshear Jan 25, 2023

damirka Feb 17, 2023 •

edited

Loading

sblackshear Feb 22, 2023

tnowacki Jan 25, 2023

Suficio commented Feb 7, 2023

damirka commented Feb 17, 2023

sblackshear left a comment

sblackshear left a comment

666lcz Feb 26, 2023

666lcz Feb 26, 2023

damirka Feb 26, 2023

jovicheng commented Nov 28, 2023

damirka commented Nov 28, 2023

jovicheng commented Nov 28, 2023

damirka commented Nov 28, 2023 •

edited

Loading

jovicheng commented Nov 28, 2023

[framework] Display object module #7668

[framework] Display object module #7668

Conversation

damirka commented Jan 25, 2023 • edited Loading

vercel bot commented Jan 25, 2023 • 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

damirka Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suficio commented Feb 7, 2023

damirka commented Feb 17, 2023

sblackshear left a comment

Choose a reason for hiding this comment

sblackshear left a 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

jovicheng commented Nov 28, 2023

damirka commented Nov 28, 2023

jovicheng commented Nov 28, 2023

damirka commented Nov 28, 2023 • edited Loading

jovicheng commented Nov 28, 2023

damirka commented Jan 25, 2023 •

edited

Loading

vercel bot commented Jan 25, 2023 •

edited

Loading

damirka Feb 17, 2023 •

edited

Loading

damirka commented Nov 28, 2023 •

edited

Loading