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

Add EVMBridgedMetadata & URI to MetadataViews #203

Merged
merged 4 commits into from Apr 26, 2024

Conversation

sisyphusSmiling
Copy link
Collaborator

@sisyphusSmiling sisyphusSmiling commented Mar 12, 2024

Related: #129

Description

With EVM on Flow coming in the near future, we'll need to reconcile differences between EVM & Cadence NFT metadata handling expectations and standards.

Motivated by ongoing work while iterating on the VM bridge, this PR puts forth two metadata views for consideration.

The first, URI, is a generic struct intended to store a string pointer to some metadata. In many cases, this may just be a JSON blob at a URL or an IPFS pointer. In other cases, this may actually be NFT metadata as a JSON blob itself as in URL-encoded URIs. In the case of ERC721s bridged from EVM, we likely won't have enough context to determine the type of URI this is, thus the generic nature of this view.

Here is the URI interface:

pub struct URI: MetadataViews.File {
    /// The base URI prefix, if any. Not needed for all URIs, but helpful
    /// for some use cases For example, updating a whole NFT collection's
    /// image host easily
    pub let baseURI: String?
    /// The URI string value
    /// NOTE: this is set on init as a concatenation of the baseURI and the
    /// value if baseURI != nil
    access(self) let value: String

    pub view fun uri(): String
        
}

The second, EVMBridgedMetadata, contains three fields

pub struct EVMBridgedMetadata {
    pub let name: String
    pub let symbol: String

    pub let uri: {MetadataViews.File}
}

The fields name and symbol are included in several ERC standards, notably ERC721 and ERC20. The last field, uri, allows Cadence projects to define how they would like their token or contract's metadata to be transmitted to an EVM counterpart. In the case of Cadence-native projects, this field could be any File implementing struct, while in the case of bridged assets (e.g. ERC721) the uri value would likely just be a URI struct.

Without getting into the specifics of bridging assets which is out of scope for this PR, it would be very helpful to get feedback on the views as their presented, if we feel they're worth adding in the MetadataViews contract, and if the community feels anything is missing from these very barebones implementations.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels
  • Update the version in package.json when there is a change in the smart contracts

@sisyphusSmiling
Copy link
Collaborator Author

@sisyphusSmiling sisyphusSmiling changed the title add EVMBridgedMetadata & URI to MetadataViews Add EVMBridgedMetadata & URI to MetadataViews Mar 21, 2024
Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

This makes sense to me. Would you want to deploy this upgrade to MetadataViews before the Crescendo upgrade so people can start adding it immediately, or do we want to deploy this as part of the Crescendo upgrade? If the latter is true, we should base this on the standard-v2 branch

@sisyphusSmiling
Copy link
Collaborator Author

Updating to note I added a baseURI: String? field to URI based on feedback implementing the view in bridge contracts.

@joshuahannan I think deploying the update before Crescendo makes the most sense to allow projects to adopt the view as they see fit before bridging is enabled. There was constructive discussion in the linked discord thread, but I'd hoped for more community feedback before merging this view. I'll raise this PR again in discord to ask folks to share feedback.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

We want to do this upgrade before Cadence 1.0, right? After we merge this, can you make a PR updating the V2 standards to include this and also add an implementation for it in the V2 ExampleNFT? I can do the upgrades on testnet and mainnet

@sisyphusSmiling
Copy link
Collaborator Author

Yeah, we'd want to include this before 1.0. I can follow up with a PR including an implementation.

@joshuahannan
Copy link
Member

oh yeah, and make sure you run make test to run the tests and generate the assets

@joshuahannan joshuahannan merged commit 694a05c into master Apr 26, 2024
2 checks passed
@joshuahannan joshuahannan deleted the giovanni/add-cross-vm-views branch April 26, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants