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

nep-393: add class metadata interface #528

Merged
merged 8 commits into from
Feb 23, 2024

Conversation

robert-zaremba
Copy link
Contributor

In #393 we defined Issuer (an entity authorized to mint SBTs in the registry) and SBT Class. We also defined Issuer Metadata and Token Metadata, but we didn't provide interface for class metadata. This was implemented in the reference implementation (in one of the subsequent revisions), but was not backported to the NEP. In this PR

  • we fix the name of the issuer interface from SBTContract to SBTIssuer. The original name is wrong and we oversight it in reviews. We talk everywhere about the issuer entity and issuer contract (even the header is SBT Issuer interface).
  • Renames ContractMetadata to IssuerMetadata.
  • added ClassMetadata struct and sbt_class_metadata function to the SBTIssuer.

@robert-zaremba robert-zaremba requested a review from a team as a code owner January 25, 2024 17:50
Copy link

render bot commented Jan 25, 2024

@frol frol added WG-contract-standards Contract Standards Work Group should be accountable S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. A-NEP-Extension A new functionality proposal to existing NEP. Once original author merges changes, we close this. labels Jan 26, 2024
@frol
Copy link
Collaborator

frol commented Jan 26, 2024

Thank you @robert-zaremba for submitting this NEP extension.

As a moderator, I reviewed this NEP and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-contract-standards working group members to assign 2 Technical Reviewers to complete a technical review (see expectations below).

Just for clarity, Technical Reviewers play a crucial role in scaling NEAR ecosystem as they provide their in-depth expertise in the niche topic while work group members can stay on guard of the NEAR ecosystem. The discussions may get too deep and it would be inefficient for each WG member to dive into every single comment, so NEAR Developer Governance designed this process that includes subject matter experts helping us to scale by writing a summary with the raised concerns and how they were addressed.

Technical Review Guidelines
  • First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.

  • Second, once all the suggestions are addressed, produce a Technical Summary, which helps the working group members make a weighted decision faster. Without the summary, the working group will have to read the whole discussion and potentially miss some details.

Technical Summary guidelines:

  • A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation). Please note that this is the reviewer's personal recommendation.

  • A summary of benefits that surfaced in previous discussions. This should include a concise list of all the benefits that others raised, not just the ones that the reviewer personally agrees with.

  • A summary of concerns or blockers, along with their current status and resolution. Again, this should reflect the collective view of all commenters, not just the reviewer's perspective.

Here is a nice example and a template for your convenience:


### Recommendation

Add recommendation

### Benefits

* Benefit

* Benefit

### Concerns

| # | Concern | Resolution | Status |

| - | - | - | - |

| 1 | Concern | Resolution | Status |

| 2 | Concern | Resolution | Status |

Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.

@frol frol requested review from a team and fadeevab January 26, 2024 12:46
@frol frol added S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. and removed S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. labels Jan 26, 2024
@robert-zaremba
Copy link
Contributor Author

I would like to ask @fadeevab and @KazanderDad to review this PR. Both provided valuable comments and reviewed the #393 .

@frol frol added S-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. and removed S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. labels Jan 27, 2024
Copy link
Contributor

@fadeevab fadeevab left a comment

Choose a reason for hiding this comment

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

Recommendation

Fix the broken references in the documentation, typos, and wording.

Benefits

Overall, the patch is not complex, everything looks fine.

Concerns

# Concern Resolution Status
1 Broken references after renaming the struct Fixes suggested Done
2 Wording and typos Fixes suggested Done

neps/nep-0393.md Outdated Show resolved Hide resolved
neps/nep-0393.md Outdated Show resolved Hide resolved
neps/nep-0393.md Outdated Show resolved Hide resolved
neps/nep-0393.md Outdated Show resolved Hide resolved
neps/nep-0393.md Outdated Show resolved Hide resolved
neps/nep-0393.md Outdated Show resolved Hide resolved
Co-authored-by: Alexander Fadeev <[email protected]>
@robert-zaremba
Copy link
Contributor Author

Thanks @fadeevab . I've applied all suggestions. I'm not sure if we should use IssuerMetadata::icon or IssuerMetadata.icon (:: vs .). Personally, I think we should use . (dot), because we refer to a struct field.

Copy link
Contributor

@fadeevab fadeevab left a comment

Choose a reason for hiding this comment

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

All suggestions are implemented, table updated: #528 (review)

@KazanderDad
Copy link

As a SME I've reviewed this NEP and find that it fills a useful purpose. It clarifies some of the potential points of confusion for anyone wishing to implement SBTs, including some that I ran into myself when deploying a new issuer recently.

I might suggest rethinking the hashing requirement of Base64 encoded SHA256. While we can recognize that this - if done right - is a little more compact with 44 characters; it is not a commonly available method and the risk for implementing wrong is high. Casual users who try to find online SHA256 hashing tools will almost exclusively find tools that instead output hexadecimal characters, resulting in a 64-character string. A very likely risk is that they will attempt to Base64-encode this string (instead of the binary), resulting in a much longer output than intended, and resulting in a lot of confusion. A more usable requirement might be "a SHA256 hash expressed as a hexadecimal string"

@robert-zaremba robert-zaremba enabled auto-merge (squash) January 30, 2024 20:40
@robert-zaremba
Copy link
Contributor Author

Base64 is used in Near more often than hex. I think we are fine with base64. Moreover it is also recommend for nft metadata

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

As a Contract Standards Work Group member, I reviewed and lean to approving this NEP. That said, I believe we reached the quorum here to approve it, and since it is just an extension to the existing NEP, I suggest we go ahead and merge it.

@robert-zaremba Please, update the CHANGELOG section with 1.1.0 version summarizing the change and the decision.

@robert-zaremba
Copy link
Contributor Author

Changelog added.
The link issues reported by the CI are not related to this PR, and should be handled separately.

@robert-zaremba robert-zaremba merged commit 747a55f into master Feb 23, 2024
3 of 4 checks passed
@robert-zaremba robert-zaremba deleted the robert/sbt-class-metadata branch February 23, 2024 14:59
@victorchimakanu
Copy link

victorchimakanu commented Mar 21, 2024

NEP Status (Updated by NEP Moderators)

Status: Approved

SME reviews:

Contract Standards Work Group voting indications (❔ | 👍 | 👎 ):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP-Extension A new functionality proposal to existing NEP. Once original author merges changes, we close this. S-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. WG-contract-standards Contract Standards Work Group should be accountable
Projects
Status: APPROVED NEPs
Development

Successfully merging this pull request may close these issues.

5 participants