Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: (spec) private property prefix, remove data blob #60

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ozwaldorf
Copy link
Collaborator

@ozwaldorf ozwaldorf commented Apr 14, 2022

Why?

Based on some feedback from the community (cc @jorgenbuilder ), there is a need for specifying if a given key value should be displayed in frontends (eg; plug wallet's trait chips).

This also removes the requirement of the data blob, as storing large amounts of data in the meta is not recommended, due to the IC's 2MB message limit.

How?

Adds the _ prefix for private properties to the spec

Removed reserved property data

Tickets?

n.a

Contribution checklist?

  • The commit messages are detailed
  • It does not break existing features (unless required)
  • I have performed a self-review of my own code
  • Documentation has been updated to reflect the changes
  • Tests have been added or updated to reflect the changes
  • All code formatting pass
  • All lints pass
  • All tests pass

Security checklist?

n.a

Demo?

n.a

Copy link

@jorgenbuilder jorgenbuilder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses the use case I have in mind for "developer properties" perfectly. @ozwaldorf I'd like to get some comments from others on my other suggestions as well: multiple asset locations in metadata beyond location and thumbnail. Should I create a separate PR for that?

@ozwaldorf
Copy link
Collaborator Author

ozwaldorf commented Apr 15, 2022

This addresses the use case I have in mind for "developer properties" perfectly. @ozwaldorf I'd like to get some comments from others on my other suggestions as well: multiple asset locations in metadata beyond location and thumbnail. Should I create a separate PR for that?

A PR would be much appreciated! Along with this though, do you think we should move all the reserved properties to the private _ namespace? So location would be _location, _location1, etc; thumbnail would be _thumbnail, _thumbnail2, etc

@jorgenbuilder
Copy link

do you think we should move all the reserved properties to the private _ namespace? So location would be _location, _location1, etc; thumbnail would be _thumbnail, _thumbnail2, etc

Fair point. Let's discuss in the new PR.

Copy link

@jorgenbuilder jorgenbuilder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me. Before approving, I'd like to run it by the plug and marketplace FE teams: it has implications there and we may get some good input. Also wondering if there's precedent in Ethereum or elsewhere, and who else we might loop in.

@@ -594,18 +594,6 @@ type TokenMetadata = record {

All of the following are reserved by the spec to verify and display assets across all applications.

Noted that `data` and `location` are mutual exclusive, only one of them is required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we delete it?

data is useful when the contract wants to store in-canister and serve it through HTTP request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants