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

Major release v0.19: add CollectionInfo, RoyaltyInfo, updatable NFTs for creator, etc. #156

Open
wants to merge 128 commits into
base: main
Choose a base branch
from

Conversation

taitruong
Copy link
Collaborator

@taitruong taitruong commented Feb 27, 2024

Major refactoring for upcoming v0.19 release.

TL;DR: The cw721 v0.19 release sees a major refactor, introducing distinctions between Creator (manages collection metadata like royalties) and Minter (controls minting), and adds a new UpdateMetadata utility for onchain metadata. We've integrated a new CollectionMedata structure within the cw721 package, moving all core logic for easier access and implementation across contracts, thus streamlining contract architecture. This update simplifies the ecosystem by removing the need for separate cw721-metadata-onchain and cw2981-royalty contracts and clearly defining roles and functionalities.

Distinction between Creator and Minter

In previous release cw721 only knows minter (aka owner). Here we added Creator. Creator controls collection metadata (like royalties) whereas minter controls minting.

NEW utility: UpdateNftInfo and UpdateCollectionMetadata msg

By adding a new CollectionMetadataExtension store as an optional extension for CollectionMetadata (name and symbol), there is also logic introduced here for updating collection metadata.

Same logic is also applied now for updating onchain NFT metadata (after mint).

The creator is now able to do both: updating collection and onchain NFT metadata!

NEW CollectionMedata in cw721 package

#[cw_serde]
pub struct CollectionMetadata<TCollectionMetadataExtension> {
    pub name: String,
    pub symbol: String,
    pub extension: TCollectionMetadataExtension,
    pub updated_at: Timestamp,
}

#[cw_serde]
pub struct CollectionMetadataExtension<TRoyaltyInfo> {
    pub description: String,
    pub image: String,
    pub external_link: Option<String>,
    pub explicit_content: Option<bool>,
    pub start_trading_time: Option<Timestamp>,
    pub royalty_info: Option<TRoyaltyInfo>,
}

#[cw_serde]
pub struct RoyaltyInfo {
    pub payment_address: Addr,
    pub share: Decimal,
}

For making it available to all existing contracts, I had to move all core logic into cw721 package. As a result, outcome is the following:

  • there are now default implementations for the Cw721Execute and Cw721Query traits
  • all structs like CollectionMetadata, NftMetadata, RoyaltyInfo, NftInfo, OwnerOfResponse, ApprovalResponse, ... moved to package

Another upgrade is adding creator along to existing minter address. In previous release due to the introduction of cw-ownable there has been a renaming of minter to ownership - which is confusing. With upcoming v0.19 release it looks like this:

  • minter can mint (formerly aka ownership in v0.17-18)
  • there is no more ownership (deprecated)
  • creator as part of collection metadata, can update collection metadata - including royalty
    • NOTE: all props is owned by collection creator, except start_trading_time which belongs to minter
    • creator and minter ownership is checked in new trait StateFactory
  • both, minter and creator, uses cw-ownable and can be transferred to another owner

all stores are in state.rs (pls note new Cw721Config which will be used for contracts):

/// Creator owns this contract and can update collection metadata!
/// !!! Important note here: !!!
/// - creator is stored using using cw-ownable's OWNERSHIP singleton, so it is not stored here
/// - in release v0.18.0 it was used for minter (which is confusing), but now it is used for creator
pub const CREATOR: OwnershipStore = OwnershipStore::new(OWNERSHIP_KEY);
/// - minter is stored in the contract storage using cw_ownable::OwnershipStore (same as for OWNERSHIP but with different key)
pub const MINTER: OwnershipStore = OwnershipStore::new("collection_minter");

/// Default CollectionMetadataExtension using `Option<CollectionMetadataExtension<RoyaltyInfo>>`
pub type DefaultOptionCollectionMetadataExtension =
    Option<CollectionMetadataExtension<RoyaltyInfo>>;
pub type DefaultOptionCollectionMetadataExtensionMsg =
    Option<CollectionMetadataExtensionMsg<RoyaltyInfoResponse>>;
/// Default NftMetadataExtension using `Option<NftMetadata>`.
pub type DefaultOptionNftMetadataExtension = Option<NftMetadata>;
pub type DefaultOptionNftMetadataExtensionMsg = Option<NftMetadataMsg>;

// explicit type for better distinction.
pub type NftMetadataMsg = NftMetadata;
#[deprecated(since = "0.19.0", note = "Please use `NftMetadata` instead")]
pub type MetaData = NftMetadata;
#[deprecated(
    since = "0.19.0",
    note = "Please use `CollectionMetadata<DefaultOptionCollectionMetadataExtension>` instead"
)]
pub type ContractInfoResponse = CollectionMetadata<DefaultOptionCollectionMetadataExtension>;

pub struct Cw721Config<
    'a,
    // Metadata defined in NftInfo (used for mint).
    TNftMetadataExtension,
    // Message passed for updating metadata.
    TNftMetadataExtensionMsg,
    // Extension defined in CollectionMetadata.
    TCollectionMetadataExtension,
    // Message passed for updating collection metadata extension.
    TCollectionMetadataExtensionMsg,
    // Defines for `CosmosMsg::Custom<T>` in response. Barely used, so `Empty` can be used.
    TCustomResponseMsg,
> where
    TNftMetadataExtension: Cw721State,
    TNftMetadataExtensionMsg: Cw721CustomMsg,
    TCollectionMetadataExtension: Cw721State,
    TCollectionMetadataExtensionMsg: Cw721CustomMsg,
{
    /// Note: replaces deprecated/legacy key "nft_info"!
    pub collection_metadata: Item<'a, CollectionMetadata<TCollectionMetadataExtension>>,
    pub token_count: Item<'a, u64>,
    /// Stored as (granter, operator) giving operator full control over granter's account.
    /// NOTE: granter is the owner, so operator has only control for NFTs owned by granter!
    pub operators: Map<'a, (&'a Addr, &'a Addr), Expiration>,
    pub nft_info: IndexedMap<
        'a,
        &'a str,
        NftInfo<TNftMetadataExtension>,
        TokenIndexes<'a, TNftMetadataExtension>,
    >,
    pub withdraw_address: Item<'a, String>,
...
}

Essentially all boilerplate code (default implementations in Cw721Execute and Cw721Query traits) are now in cw721 package.
As a result contracts are very slim. Please note, implementations by Cw721Config, along with Cw721Execute and
Cw721Query traits are opionated - though contracts may be implement differently by overriding default implementations.

Here is how new cw721-base looks like by using cw721 package:

Define struct for contract interaction:

pub struct Cw721Contract<
    'a,
    // Metadata defined in NftInfo (used for mint).
    TNftMetadataExtension,
    // Message passed for updating metadata.
    TNftMetadataExtensionMsg,
    // Extension defined in CollectionMetadata.
    TCollectionMetadataExtension,
    TCollectionMetadataExtensionMsg,
    // Defines for `CosmosMsg::Custom<T>` in response. Barely used, so `Empty` can be used.
    TCustomResponseMsg,
> where
    TNftMetadataExtension: Cw721State,
    TNftMetadataExtensionMsg: Cw721CustomMsg,
    TCollectionMetadataExtension: Cw721State,
    TCollectionMetadataExtensionMsg: Cw721CustomMsg,
{
    pub config: Cw721Config<
        'a,
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    >,
}

impl<
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    > Default
    for Cw721Contract<
        'static,
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    >
where
    TNftMetadataExtension: Cw721State,
    TNftMetadataExtensionMsg: Cw721CustomMsg,
    TCollectionMetadataExtension: Cw721State,
    TCollectionMetadataExtensionMsg: Cw721CustomMsg,
{
    fn default() -> Self {
        Self {
            config: Cw721Config::default(),
        }
    }
}

Then implement execute and query traits and using default implementations:

// implements Cw721Execute
impl<
        'a,
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    >
    Cw721Execute<
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    >
    for Cw721Contract<
        'a,
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    >
where
    TNftMetadataExtension: Cw721State,
    TNftMetadataExtensionMsg: Cw721CustomMsg + StateFactory<TNftMetadataExtension>,
    TCollectionMetadataExtension: Cw721State,
    TCollectionMetadataExtensionMsg: Cw721CustomMsg + StateFactory<TCollectionMetadataExtension>,
    TCustomResponseMsg: CustomMsg,
{
}

// implements Cw721Query
impl<
        'a,
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    > Cw721Query<TNftMetadataExtension, TCollectionMetadataExtension>
    for Cw721Contract<
        'a,
        TNftMetadataExtension,
        TNftMetadataExtensionMsg,
        TCollectionMetadataExtension,
        TCollectionMetadataExtensionMsg,
        TCustomResponseMsg,
    >
where
    TNftMetadataExtension: Cw721State,
    TNftMetadataExtensionMsg: Cw721CustomMsg,
    TCollectionMetadataExtension: Cw721State,
    TCollectionMetadataExtensionMsg: Cw721CustomMsg,
    TCustomResponseMsg: CustomMsg,
{
}

And finally in lib.rs:

    // This makes a conscious choice on the various generics used by the contract
    #[cfg_attr(not(feature = "library"), entry_point)]
    pub fn instantiate(
        deps: DepsMut,
        env: Env,
        info: MessageInfo,
        msg: Cw721InstantiateMsg<DefaultOptionCollectionMetadataExtensionMsg>,
    ) -> Result<Response, Cw721ContractError> {
        let contract = Cw721Contract::<
            DefaultOptionNftMetadataExtension,
            DefaultOptionNftMetadataExtensionMsg,
            DefaultOptionCollectionMetadataExtension,
            DefaultOptionCollectionMetadataExtensionMsg,
            Empty,
        >::default();
        contract.instantiate(deps, &env, &info, msg, CONTRACT_NAME, CONTRACT_VERSION)
    }

    #[cfg_attr(not(feature = "library"), entry_point)]
    pub fn execute(
        deps: DepsMut,
        env: Env,
        info: MessageInfo,
        msg: Cw721ExecuteMsg<
            DefaultOptionNftMetadataExtensionMsg,
            DefaultOptionCollectionMetadataExtensionMsg,
        >,
    ) -> Result<Response, Cw721ContractError> {
        let contract = Cw721Contract::<
            DefaultOptionNftMetadataExtension,
            DefaultOptionNftMetadataExtensionMsg,
            DefaultOptionCollectionMetadataExtension,
            DefaultOptionCollectionMetadataExtensionMsg,
            Empty,
        >::default();
        contract.execute(deps, &env, &info, msg)
    }

    #[cfg_attr(not(feature = "library"), entry_point)]
    pub fn query(
        deps: Deps,
        env: Env,
        msg: Cw721QueryMsg<
            DefaultOptionNftMetadataExtension,
            DefaultOptionCollectionMetadataExtension,
        >,
    ) -> StdResult<Binary> {
        let contract = Cw721Contract::<
            DefaultOptionNftMetadataExtension,
            DefaultOptionNftMetadataExtensionMsg,
            DefaultOptionCollectionMetadataExtension,
            DefaultOptionCollectionMetadataExtensionMsg,
            Empty,
        >::default();
        contract.query(deps, &env, msg)
    }

    #[cfg_attr(not(feature = "library"), entry_point)]
    pub fn migrate(
        deps: DepsMut,
        env: Env,
        msg: Cw721MigrateMsg,
    ) -> Result<Response, Cw721ContractError> {
        let contract = Cw721Contract::<
            DefaultOptionNftMetadataExtension,
            DefaultOptionNftMetadataExtensionMsg,
            DefaultOptionCollectionMetadataExtension,
            DefaultOptionCollectionMetadataExtensionMsg,
            Empty,
        >::default();
        contract.migrate(deps, env, msg, CONTRACT_NAME, CONTRACT_VERSION)
    }

That's it!

cw721-metadata-onchain and cw2981-royalty contracts removed

Another positive side effect of this refactoring is that there's no need for cw721-metadata-onchain and cw2981-royalty contracts anymore. This is covered in cw721-base. Design hasnt changed and onchain metadata is stored in NftInfo<TNftMetadataExtension>. Before this PR, funny thing is that in cw721-base it was defined as:

TMetadataExtension = Option<Empty>, allowing to NOT store onchain data, but it doesnt make sense having None and Some(Empty).

In cw721-metadata-onchain contract it was defined as:

TMetadataExtension = Option<Metadata>, here for onchain contract it was allowed to either provide onchain data or event not!

In this PR it defines a pub type DefaultOptionNftMetadataExtension = Option<NftMetadata> which is used in cw721-base. onchain contract is obsolete and is removed.

- new query GetMinterOwnership and GetCreatorOwnership, deprecated Ownership
- new execute UpdateMinterOwnership and UpdateCreatorOwnership, deprecate UpdateOwnership
- dedicated stores for minter and creator, where creator usess by default cw_ownable singleton!
- new migrate msg allowing to reset creator and minter
- cleanup migration and split legacy part to dedicated functions
- also make sure using decicated CREATOR and MINTER stores and NOT use cw_ownable::...
- move logic from cw721-base to cw721
- merge cw721-base and cw721-metadata-onchain into one, distinction is: `extension: T` where T for base contract is `type DefaultMetadataExtension = Option<Metadata>`
- all logic now in default implementation for traits Cw721Execute and Cw721Query
Comment on lines 35 to 42
#[allow(deprecated)]
Cw721QueryMsg::ContractInfo {} => {
to_json_binary(&self.query_collection_info(deps, env)?)
}
Cw721QueryMsg::GetCollectionInfo {} => {
to_json_binary(&self.query_collection_info(deps, env)?)
}
Cw721QueryMsg::NftInfo { token_id } => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also ContractInfo is deprecated and is replaced by GetCollectionInfo. In next release all deprecated msgs should be removed. Here it is kept for backward compatibility, so existing frontends wont break!

Comment on lines 171 to 179
#[deprecated(since = "0.19.0", note = "Please use GetMinterOwnership instead")]
#[returns(Ownership<Addr>)]
Ownership {},

#[returns(Ownership<Addr>)]
GetMinterOwnership {},

#[returns(Ownership<Addr>)]
GetCreatorOwnership {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ownership -> GetMinterOwnership
new GetCreatorOwnership

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments on these queries would be helpful. Good to note what they are in plain English and that minter will soon be deprecated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 213 to 215
#[deprecated(since = "0.19.0", note = "Please use GetMinterOwnership instead")]
#[returns(MinterResponse)]
Minter {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also for backward compatibility: Minter -> GetMinterOwnership

Comment on lines +239 to +244
pub enum Cw721MigrateMsg {
WithUpdate {
minter: Option<String>,
creator: Option<String>,
},
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Important: new migrate msg allowing to reset creator and minter!

Comment on lines 18 to 32
pub trait Cw721Execute<T, C>
where
T: Serialize + DeserializeOwned + Clone,
C: CustomMsg,
{
type Err: ToString;

fn transfer_nft(
&self,
deps: DepsMut,
env: Env,
info: MessageInfo,
recipient: String,
token_id: String,
) -> Result<Response<C>, Self::Err>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before it e.g. Cw721Execute was rather an interface with no logic. Also note confusing generics naming like in Cw721Execute<T, C> and ...

Comment on lines +15 to +18
pub const CREATOR: OwnershipStore = OwnershipStore::new(OWNERSHIP_KEY);
/// - minter is stored in the contract storage using cw_ownable::OwnershipStore (same as for OWNERSHIP but with different key)
pub const MINTER: OwnershipStore = OwnershipStore::new("collection_minter");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CREATOR and MINTER. Please note that singleton ownership was used before for minter, but now used for creator! new minter ownership has a new key collection_minter. This is also covered in migration (as shown later below).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so to be clear, we now have an optional second owner which is creator. The max number of owners in the contract would now be 2 and not more than that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay so to be clear, we now have an optional second owner which is creator. The max number of owners in the contract would now be 2 and not more than that right?

Correct. They are both optional: creator and minter. Naming convention before was very confusing - started with the introduction of cw-ownable. There it was minter = owner = ownership.

Imo, using terms creator and minter is much more clear. Ownership appears here only in terms of minter ownership and creator ownership. Both uses for 2-step for transfer as defined by cw-ownable:

  1. Owner transfers to new owner
  2. New owner accepts, on acceptance minter/creator ownership is changed

Comment on lines +702 to +710
/// Migrates only in case ownership is not present
/// !!! Important note here: !!!
/// - creator owns the contract and can update collection info
/// - minter can mint new tokens
///
/// Before v0.19.0 there were confusing naming conventions:
/// - v0.17.0: minter was replaced by cw_ownable, as a result minter is owner
/// - v0.16.0 and below: minter was stored in dedicated `minter` store (so NOT using cw_ownable at all)
pub fn migrate_legacy_minter_and_creator(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

migrate_legacy_minter_and_creator: migrate legacy minter and set new creator...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@humanalgorithm here is the most important part, allowing to migrate any older version to the latest

packages/cw721/src/execute.rs Outdated Show resolved Hide resolved
contracts/cw721-base/src/lib.rs Show resolved Hide resolved
Comment on lines -7 to -19
pub fn migrate<T, C, E, Q>(deps: DepsMut) -> Result<Response<C>, ContractError>
where
T: Serialize + DeserializeOwned + Clone,
Q: CustomMsg,
E: CustomMsg,
{
// remove old minter info
let tract16 = v16::Cw721Contract::<T, C, E, Q>::default();
let minter = tract16.minter.load(deps.storage)?;
tract16.minter.remove(deps.storage);

// save new ownership info
let ownership = cw_ownable::initialize_owner(deps.storage, deps.api, Some(minter.as_str()))?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also this old migrato logic of minter to ownership is error prone. Like it is e.g. possible to migrate from v0.16 to v0.18. Then minter is transferred to new addr. Keep in mind in old minter store there is still old addr. So migrating back to v0.16 and to v0.18 again, would reset minter addy. Better to always check whether new minter ownship is set - if so no migration needed.

This is an edge case, but still not good.

Copy link
Contributor

Choose a reason for hiding this comment

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

We needed this for migration to v.01.7. I think I might have been the one that "wrote" it but I was just copying migration logic from cw base contract.

@taitruong taitruong changed the title WIP - Collection info Add new CollectionInfo, including RoyaltyInfo and creator (along with existing minter). Mar 7, 2024
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.

None yet

4 participants