Contract level metadata

[JakeHartnell]

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.

[the-frey]

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 🤷

[the-frey]

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

[JakeHartnell]

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.

[0xekez]

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.

[the-frey]

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).

[shanev]

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.

[JakeHartnell]

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?

[the-frey]

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.

[JakeHartnell]

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]

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

[0xekez]

[larry0x]

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]

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.

[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.

[JakeHartnell]

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:

1. 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?
2. 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
3. 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
4. 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]

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]

Closing in favor of this approach: #90