Contract level metadata #75

JakeHartnell · 2022-07-25T02:38:42Z

Adds support for an optional collection_uri for use in ics721, as well as extensible on chain collection level metadata.

Also added a migrate entrypoint, messages, and tests.

Closes #68.

contracts/cw721-base/src/state.rs

contracts/cw721-base/src/contract_tests.rs

contracts/cw721-base/src/execute.rs

contracts/cw721-base/src/state.rs

packages/cw721/src/traits.rs

contracts/cw721-base/src/state.rs

the-frey · 2022-07-26T16:27:33Z

contracts/cw721-base/src/msg.rs

+    /// Universal resource identifier for this NFT Collection
+    /// Should point to a JSON file that conforms to contract level metadata
+    /// schema: https://docs.opensea.io/docs/contract-level-metadata
+    pub collection_uri: Option<String>,


Isn't this just something that an implementer should put in the metadata? It assumes quite heavily that you're using NFTs in a particular way, which as noted by the sarcastic (fair) comment at line 11 is a thing that has occurred elsewhere and resulted in fields that aren't really used by all implementers 🤷

Especially as it also has an implication on the default config response vs it just being cleanly encapsulated in the optional meta.

Isn't this just something that an implementer should put in the metadata?

Well collection_uri is fairly important. It's used by ics721 to transfer collection level metadata. Without it, a Juno NFT transferred to Stargaze would have no way to display information about the collection. For example, at the top of the Bad Kids marketplace page, we show the collection image, name, and description. We use this info in other views as well, all of which wouldn't work with cross chain NFTs if they didn't have a collection_uri. Of course other apps using Interchain NFTs may also want to use this data. ics721 as written currently does not support other on-chain metadata.

@shanev initially suggested we put it in the metadata extension, and it's an easy change to make, but given the spec I thought maybe it would be best to higher level than the optional metadata and always included in the default config response. Will go with what we decide as a group, but wanted to articulate my concerns and the rational for not putting it in the metadata extension.

Another reason to have this is that the cosmos SDK NFT implementation does: https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-043-nft-module.md#class

I think it is reasonable that we have the same info they do. OpenSea also has support for collection level metadata: https://docs.opensea.io/docs/contract-level-metadata

Am all for not including it if we don't think it provides value, but IMO most NFTs will want to be able to have some top-level metadata? Am having trouble thinking of an example where they wouldn't.

I think the opensea argument isn't particularly compelling, but take the point that we probably want the same implementation as the SDK - to me, that's probably enough to swing it.

From a design point of view, I still believe that it should simply be in the metadata, and that metadata is therefore simply not optional, since it has a key that is always required.

It strikes me as bad data structure design to have something that is metadata... not in the metadata.

This is already an issue with the opensea style URIs in the existing CW NFTs, because you end up duplicating most metadata fields in the main metadata, whether you're using external JSON or on-chain metadata (which both the projects I work on use).

I am in agreement with @the-frey here in theory. However, although collection_uri and metadata both represent top-level metadata, one is a URI and the other is an arbitrary data type that lives on-chain. The problem is that it's possible to have both. You can have partial metadata off-chain, and partial and/or conflicting metadata on-chain. Especially considering the SDK NFT implementation, I see no harm in having both, especially since they are optional.

That said, there is a way to represent "either" in Rust. Can make this an enum like:

pub enum Metadata<T> { CollectionUri(String), Data(T), }

This is how I would prefer to do it. But the current approach is also fine imho.

Personally, I would rather just have metadata and not have collection_uri, but unfortunately the ics721 spec doesn't support on chain metadata. 🥲

So I am also in agreement with @the-frey in theory, but I think the spec is forcing our hand here. Maybe @ezekiiel has some context here since he's been working on an implementation of ics721?

Feel like @shanev has a point that we could encapsulate part of the dilemma in an ADT and thus make it more concretely a single thing rather than two separate fields...

Kind of annoying that the ICS721 spec doesn't support on-chain metadata, as an aside.

Kind of annoying that the ICS721 spec doesn't support on-chain metadata, as an aside.

This is the problem with having a single thing. If we use an enum like above, any NFTs with on-chain metadata will not have collection metadata when using ics721.

Is this ideal? No.

Is it the end of the world to have two optional fields? Also no. 🤷‍♂️

JakeHartnell · 2022-08-21T22:57:21Z

@shanev / @yubrew, how do you feel about these changes? Is there anything you would like to see different?

0xekez

contracts/cw721-base/README.md

larry0x · 2022-09-15T01:33:38Z

SG-721 also has collection-level metadata but it’s implemented differently. I think @shanev should take a look at this and see if we can avoid the conflicts

0xekez

new migration changes look good to me!

JakeHartnell · 2022-09-15T18:38:01Z

SG-721 also has collection-level metadata but it’s implemented differently. I think @shanev should take a look at this and see if we can avoid the conflicts

Yes, would be good to get his thoughts. @shanev care to weigh in?

sg721 uses collection_info, so could make a custom metadata extension with all the same info. https://github.com/public-awesome/launchpad/blob/main/packages/sg721/src/lib.rs#L81

Happy to help on the migration side there. Frontend changes would be minor as well, just would need to continue to check the collection_info field for legacy sg721 contracts that aren't migrated. Queries and everything else would be the same.

Add an exentsion to thde InstantiateMessage so that custom contract-level metadata can be added. Querying CollectionInfo returns this extenstion.

shanev

Thanks for this update!

I'm a bit concerned about the additional complexity this is adding with execute and query extensions. Can't those be added be added simply by adding new execute and query functions in a new contract with a wrapper struct? Check out Extending the refactored sg721-base. So instead of extending this contract to support all uses cases, have new contracts compose this and add to it. Thoughts?

A noob getting into CW NFTs might be a bit overwhelmed with all the options. Ideally cw721-base should be a simple starting point.

contracts/cw721-base/src/execute.rs

contracts/cw721-base/README.md

Co-authored-by: shane.stars <shanev@gmail.com>

JakeHartnell · 2022-09-18T02:07:27Z

Thanks for this update!

I'm a bit concerned about the additional complexity this is adding with execute and query extensions. Can't those be added be added simply by adding new execute and query functions in a new contract with a wrapper struct? Check out Extending the refactored sg721-base. So instead of extending this contract to support all uses cases, have new contracts compose this and add to it. Thoughts?

A noob getting into CW NFTs might be a bit overwhelmed with all the options. Ideally cw721-base should be a simple starting point.

Valid concern! A couple of thoughts here:

Consumers of cw721-base like sg721-base can certainly make a nicer development experience on top of this. I think that's a great opportunity for sg721. Really love the work @larry0x did there. Perhaps he has some thoughts on this PR?
We already had Execute extensions, but lacked query and collection metadata extensions. Query extensions were merged in with this PR: attempt to update cw-nft to add custom query and custom execute. #42
Query extensions can remove a lot of repetative code and complexity around extending the base query types, so no need to impl From<QueryMsg> for Cw721QueryMsg like here: https://github.com/public-awesome/launchpad/blob/main/contracts/sg721-base/src/msg.rs#L50
Traits are a little confusing for new rust programmers, but something folks will have to learn. To address this, we can and should make better dev-ex wrappers around this contract like sg721-base. Completely open to better patterns (maybe @larry0x has some ideas?), but I do think this PR makes things a little better by making everything extensible in the same way and giving human readable names to all the traits.

The thing I like about Query and Metadata extensions is that they allow for a single file NFT contract that customizes all aspects.

Here's an example that has custom execute, collection metadata, token metadata, and query extensions... all in 118 lines of code.

use cosmwasm_schema::cw_serde;
use cosmwasm_std::{CustomMsg, Empty};
pub use cw721_base::{
    ContractError, Cw721Contract, ExecuteMsg, InstantiateMsg, MintMsg, MinterResponse, QueryMsg,
};

// Version info for migration
const CONTRACT_NAME: &str = "crates.io:cw721-example";
const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION");

// MintExt allows for adding custom on-chain metadata to your NFTs
#[cw_serde]
pub struct MintExt {
    creator: String,
}
impl CustomMsg for MintExt {}

// Define custom contract metadata, the ContractInfo query will return with the info set here
#[cw_serde]
pub struct CollectionMetadataExt {
    pub creator: String,
}
impl CustomMsg for CollectionMetadataExt {}

// Define a custom query ext
#[cw_serde]
pub enum QueryExt {
    AdditionalQuery {},
}
impl CustomMsg for QueryExt {}

// Define a custom query response
#[cw_serde]
pub struct AdditionalQueryResponse {
    message: String,
}

// Define a custom execute extension. Allows for creating new contract methods
#[cw_serde]
pub enum ExecuteExt {
    AdditionalExecute {},
}
impl CustomMsg for ExecuteExt {}

// Put it all together!
pub type CustomCw721<'a> =
    Cw721Contract<'a, MintExt, Empty, CollectionMetadataExt, ExecuteExt, QueryExt>;

#[cfg(not(feature = "library"))]
pub mod entry {
    use super::*;

    use cosmwasm_std::{entry_point, to_binary};
    use cosmwasm_std::{Binary, Deps, DepsMut, Env, MessageInfo, Response, StdResult};
    use cw2::set_contract_version;

    #[entry_point]
    pub fn instantiate(
        mut deps: DepsMut,
        env: Env,
        info: MessageInfo,
        msg: InstantiateMsg<CollectionMetadataExt>,
    ) -> Result<Response, ContractError> {
        // Call the instantiate on our base cw721 with our custom extensions
        let res = CustomCw721::default().instantiate(deps.branch(), env, info, msg)?;
        // Explicitly set contract name and version, otherwise set to cw721-base info
        set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)
            .map_err(ContractError::Std)?;
        Ok(res)
    }

    #[entry_point]
    pub fn execute(
        deps: DepsMut,
        env: Env,
        info: MessageInfo,
        msg: ExecuteMsg<MintExt, ExecuteExt>,
    ) -> Result<Response, ContractError> {
        match msg {
            // Match extension messages
            ExecuteMsg::Extension { msg } => match msg {
                // Map them to their message handlers
                ExecuteExt::AdditionalExecute {} => execute_custom(deps, env, info),
            },
            // Handle other messages with the cw721-base default
            _ => CustomCw721::default().execute(deps, env, info, msg),
        }
    }

    #[entry_point]
    pub fn query(deps: Deps, env: Env, msg: QueryMsg<QueryExt>) -> StdResult<Binary> {
        match msg {
            // Match extension messages
            QueryMsg::Extension { msg } => match msg {
                // Map them to their message handlers
                QueryExt::AdditionalQuery {} => to_binary(&custom_query(deps, env)?),
            },
            // Handle other queries with the cw721-base default
            _ => CustomCw721::default().query(deps, env, msg),
        }
    }

    // Custom execute handler
    pub fn execute_custom(
        deps: DepsMut,
        env: Env,
        info: MessageInfo,
    ) -> Result<Response, ContractError> {
        Ok(Response::default())
    }

    // Custom query handler
    pub fn custom_query(deps: Deps, env: Env) -> StdResult<AdditionalQueryResponse> {
        Ok(AdditionalQueryResponse {
            message: String::from("meow"),
        })
    }
}

yubrew

different creators have varying motivations, and i think cw-nfts should start very simply and allow advanced entry points for customization. imagine a new dev is looking at this repo with fresh eyes. can they go from nothing to launching a working smart contract in a few hours? can they use the same repo and modify with advanced functionality over a longer time period?

first: cw721-base, does the base mean basic like the 101 version to get up and running quickly? or does it mean base like the foundation of a great empire with heavy infrastructure? i think it should mean basic and err on the side of too simplistic so non-devs can get productive quickly. ex: some nft collections never intend to ibc anywhere else.

second: i'm in favor of optional collection uri. ics721, sdk nft standard, etc are all new and evolving. we haven't figured out the final solutions. on chain metadata is one way, collection uri (also an opensea standard https://docs.opensea.io/docs/contract-level-metadata) is another

maybe on chain collection info is required by the current ics721. some creators will want to use a collection uri anyway and then relay it to other chains even with a broken collection uri. if there is no optional standard for collection uri, then the number of user generated deviations can easily create downstream problems.

also it'd be nice to have some interchain standards and use sane defaults. maybe start with whatever opensea does and go from there.

regarding traits / type inference: these types of questions come up a lot with rust, along with borrow checker and lifetimes. it's definitely a stumbling block for new rust devs and i'm not confident everyone using cw-nfts should gain experience with types and traits to do the basic stuff.

JakeHartnell · 2022-09-18T23:46:50Z

maybe on chain collection info is required by the current ics721. some creators will want to use a collection uri anyway and then relay it to other chains even with a broken collection uri. if there is no optional standard for collection uri, then the number of user generated deviations can easily create downstream problems.

+1

first: cw721-base, does the base mean basic like the 101 version to get up and running quickly? or does it mean base like the foundation of a great empire with heavy infrastructure?

cw721-base (as it is currently written) is meant to a base for other NFT smart contracts like sg721.

To make things easier, we could make a cw721-basic contract as a 101. We could also make a video on writing custom NFT contracts. Personally, I think extension does make things easier and not harder.

Is there room to improve? Of course, but we shouldn't let our quest for perfection be the enemy of the good.

i think it should mean basic and err on the side of too simplistic so non-devs can get productive quickly.

Non-devs can use things like Stargaze Studio or other tools. If people want to work with smart contracts, they have to learn things... I don't think there is a way to get around that.

JakeHartnell · 2022-10-22T00:02:54Z

Closing in favor of this approach: #90

JakeHartnell commented Jul 25, 2022

View reviewed changes

contracts/cw721-base/src/state.rs Outdated Show resolved Hide resolved

JakeHartnell requested review from orkunkl, Callum-A, 0xekez, shanev, the-frey and yubrew July 25, 2022 03:51

0xekez reviewed Jul 25, 2022

View reviewed changes

the-frey reviewed Jul 26, 2022

View reviewed changes

JakeHartnell marked this pull request as ready for review July 28, 2022 06:25

0xekez previously approved these changes Aug 23, 2022

View reviewed changes

contracts/cw721-base/README.md Show resolved Hide resolved

0xekez mentioned this pull request Aug 23, 2022

Fix trait to be compatible for custom modules #77

Closed

JakeHartnell dismissed 0xekez’s stale review via 0993e21 September 14, 2022 19:58

JakeHartnell force-pushed the contract-level-metadata branch from 3a3fdd7 to 0993e21 Compare September 14, 2022 19:58

JakeHartnell requested review from 0xekez and the-frey September 14, 2022 19:59

JakeHartnell force-pushed the contract-level-metadata branch 2 times, most recently from 9cb9b34 to b8b4094 Compare September 14, 2022 23:55

JakeHartnell requested a review from larry0x September 14, 2022 23:56

0xekez previously approved these changes Sep 15, 2022

View reviewed changes

JakeHartnell added 7 commits September 15, 2022 11:52

Support extensible contract-level metadata

f8553fd

Add an exentsion to thde InstantiateMessage so that custom contract-level metadata can be added. Querying CollectionInfo returns this extenstion.

Update other contracts to support the new cw721-base

6f1adeb

Make contract_uri optional

c62af80

Re-add name and symbol for better backwards compatability

ee06fc6

Remove unneeded impl block

a06def2

Give traits human readable names

e30fc58

Remove unused trait on CwExecute, human readable names for cw721 package

b8c44f4

JakeHartnell added 7 commits September 15, 2022 11:53

Tests for mint, instantiate, execute, and query extensions

914997d

Remove outdated helpers file

b7d03c2

Better test variable names for Instantiate extension

77af7d3

Update READMEs to be up to date, add extension example

d925783

Bump versions to v0.14.0

4d099a7

Update schema, add schema generation script from DAO DAO

9cf591f

Add migrate entrypoint, message, and basic tests

66e93e3

JakeHartnell dismissed 0xekez’s stale review via 66e93e3 September 15, 2022 18:56

JakeHartnell force-pushed the contract-level-metadata branch from b8b4094 to 66e93e3 Compare September 15, 2022 18:56

0xekez previously approved these changes Sep 15, 2022

View reviewed changes

shanev requested changes Sep 17, 2022

View reviewed changes

contracts/cw721-base/src/execute.rs Outdated Show resolved Hide resolved

contracts/cw721-base/README.md Outdated Show resolved Hide resolved

JakeHartnell dismissed 0xekez’s stale review via 2b11b6a September 18, 2022 01:25

JakeHartnell and others added 3 commits September 17, 2022 18:25

Update contracts/cw721-base/README.md

2b11b6a

Co-authored-by: shane.stars <shanev@gmail.com>

InstantiateExt -> CollectionMetadataExt

0b9013c

Update the example in the README

4590e79

yubrew reviewed Sep 18, 2022

View reviewed changes

JakeHartnell closed this Oct 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contract level metadata #75

Contract level metadata #75

JakeHartnell commented Jul 25, 2022 •

edited

the-frey Jul 26, 2022

the-frey Jul 26, 2022

JakeHartnell Jul 26, 2022

0xekez Jul 26, 2022

the-frey Jul 31, 2022

shanev Sep 17, 2022 •

edited

JakeHartnell Sep 18, 2022

the-frey Sep 18, 2022

JakeHartnell Sep 18, 2022

JakeHartnell commented Aug 21, 2022

0xekez left a comment

larry0x commented Sep 15, 2022

0xekez left a comment

JakeHartnell commented Sep 15, 2022 •

edited

shanev left a comment

JakeHartnell commented Sep 18, 2022

yubrew left a comment •

edited

JakeHartnell commented Sep 18, 2022 •

edited

JakeHartnell commented Oct 22, 2022

Contract level metadata #75

Contract level metadata #75

Conversation

JakeHartnell commented Jul 25, 2022 • edited

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

shanev Sep 17, 2022 • edited

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

JakeHartnell commented Aug 21, 2022

0xekez left a comment

Choose a reason for hiding this comment

larry0x commented Sep 15, 2022

0xekez left a comment

Choose a reason for hiding this comment

JakeHartnell commented Sep 15, 2022 • edited

shanev left a comment

Choose a reason for hiding this comment

JakeHartnell commented Sep 18, 2022

yubrew left a comment • edited

Choose a reason for hiding this comment

JakeHartnell commented Sep 18, 2022 • edited

JakeHartnell commented Oct 22, 2022

JakeHartnell commented Jul 25, 2022 •

edited

shanev Sep 17, 2022 •

edited

JakeHartnell commented Sep 15, 2022 •

edited

yubrew left a comment •

edited

JakeHartnell commented Sep 18, 2022 •

edited