Major release v0.19: add CollectionInfo, RoyaltyInfo, updatable NFTs for creator, etc.#156
Conversation
taitruong
force-pushed
the
collection-info
branch
2 times, most recently
from
5fe10c0 to
89da1ab
Compare
taitruong
force-pushed
the
collection-info
branch
from
0494810 to
895f462
Compare
packages/cw721/src/query.rs
Outdated
| #[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 } => { |
There was a problem hiding this comment.
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!
| #[deprecated(since = "0.19.0", note = "Please use GetMinterOwnership instead")] | ||
| #[returns(Ownership<Addr>)] | ||
| Ownership {}, | ||
|
|
||
| #[returns(Ownership<Addr>)] | ||
| GetMinterOwnership {}, | ||
|
|
||
| #[returns(Ownership<Addr>)] | ||
| GetCreatorOwnership {}, |
There was a problem hiding this comment.
Ownership -> GetMinterOwnership
new GetCreatorOwnership
There was a problem hiding this comment.
Comments on these queries would be helpful. Good to note what they are in plain English and that minter will soon be deprecated.
There was a problem hiding this comment.
packages/cw721/src/msg.rs
Outdated
| #[deprecated(since = "0.19.0", note = "Please use GetMinterOwnership instead")] | ||
| #[returns(MinterResponse)] | ||
| Minter {}, |
There was a problem hiding this comment.
Also for backward compatibility: Minter -> GetMinterOwnership
| pub enum Cw721MigrateMsg { | ||
| WithUpdate { | ||
| minter: Option<String>, | ||
| creator: Option<String>, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Important: new migrate msg allowing to reset creator and minter!
packages/cw721/src/traits.rs
Outdated
| 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>; |
There was a problem hiding this comment.
Before it e.g. Cw721Execute was rather an interface with no logic. Also note confusing generics naming like in Cw721Execute<T, C> and ...
| 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"); | ||
|
|
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Owner transfers to new owner
- New owner accepts, on acceptance minter/creator ownership is changed
| /// 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( |
There was a problem hiding this comment.
migrate_legacy_minter_and_creator: migrate legacy minter and set new creator...
There was a problem hiding this comment.
@humanalgorithm here is the most important part, allowing to migrate any older version to the latest
| 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()))?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
changed the title
CollectionInfo, including RoyaltyInfo and creator (along with existing minter).
contracts/cw721-base/src/error.rs
Outdated
| pub enum ContractError { | ||
| #[error(transparent)] | ||
| Std(#[from] StdError), | ||
|
|
||
| #[error(transparent)] | ||
| Ownership(#[from] OwnershipError), | ||
|
|
||
| #[error(transparent)] | ||
| Version(#[from] cw2::VersionError), | ||
|
|
||
| #[error("token_id already claimed")] | ||
| Claimed {}, | ||
|
|
||
| #[error("Cannot set approval that is already expired")] | ||
| Expired {}, | ||
|
|
||
| #[error("Approval not found for: {spender}")] | ||
| ApprovalNotFound { spender: String }, | ||
|
|
||
| #[error("No withdraw address set")] | ||
| NoWithdrawAddress {}, | ||
| } |
There was a problem hiding this comment.
Moved to cw721 package for better re-use
contracts/cw721-base/src/msg.rs
Outdated
| use schemars::JsonSchema; | ||
|
|
||
| #[cw_serde] | ||
| pub struct InstantiateMsg { |
There was a problem hiding this comment.
All msgs and response structs moved from cw721-base to cw721 for better re-use
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)] | ||
| pub struct Metadata { |
There was a problem hiding this comment.
Should we have some kind of flexible field here if we are putting all these attributes? I wonder if we would ever need more than what's listed here. Or is attributes flexible enough?
There was a problem hiding this comment.
It is flexible enough. There is Metadata.attributes which is a list of Traits:
#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
pub struct Trait {
pub display_type: Option<String>,
pub trait_type: String,
pub value: String,
}There was a problem hiding this comment.
So it follows 100% ERC721 spec, using metadata and traits.
|
What is "T" in this struct naming? pub struct Cw721Config< |
It is a general convention for generics starting generally with "T". Has nothing to do with my name :P Cheers, Mr T |
contracts/cw721-base/src/query.rs
Outdated
| TMetadataExtension, | ||
| TCustomResponseMessage, | ||
| TExtensionExecuteMsg, | ||
| TCollectionInfoExtension, |
There was a problem hiding this comment.
Much nicer with real trait names, been wanting this for a while. ❤️
| /// Deprecated: use GetCollectionInfo instead! Will be removed in next release! | ||
| ContractInfo {}, |
There was a problem hiding this comment.
@JakeHartnell, added docs for all deprecated query msgs and noting they'll be removed in next release.
| /// The creator is the only one eligible to update `CollectionInfo`. | ||
| UpdateCollectionInfo { | ||
| collection_info: CollectionInfoMsg<TCollectionInfoExtensionMsg>, | ||
| }, |
| fn proper_minter_and_creator_initialization() { | ||
| // case 1: sender is used in case minter and creator is not set | ||
| { | ||
| let mut deps = mock_dependencies(); | ||
|
|
||
| let info_minter_and_creator = mock_info("minter_and_creator", &[]); | ||
| Cw721Contract::< | ||
| DefaultOptionMetadataExtension, | ||
| MetadataMsg, | ||
| DefaultOptionCollectionInfoExtension, | ||
| CollectionInfoExtensionMsg<RoyaltyInfo>, | ||
| Empty, | ||
| >::default() | ||
| .instantiate( | ||
| deps.as_mut(), | ||
| mock_env(), | ||
| info_minter_and_creator.clone(), | ||
| Cw721InstantiateMsg { | ||
| name: "collection_name".into(), | ||
| symbol: "collection_symbol".into(), | ||
| collection_info_extension: None, | ||
| creator: None, | ||
| minter: None, | ||
| withdraw_address: None, | ||
| }, | ||
| "contract_name", | ||
| "contract_version", | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let minter = MINTER.item.load(deps.as_ref().storage).unwrap().owner; | ||
| assert_eq!(minter, Some(info_minter_and_creator.sender.clone())); | ||
| let creator = CREATOR.item.load(deps.as_ref().storage).unwrap().owner; | ||
| assert_eq!(creator, Some(info_minter_and_creator.sender)); | ||
| } | ||
| // case 2: minter and creator are set | ||
| { |
There was a problem hiding this comment.
unit test for optional creator and minter on init
packages/cw721/src/execute.rs
Outdated
| let collection_info = CollectionInfo { | ||
| name: msg.name, | ||
| symbol: msg.symbol, | ||
| extension: msg.collection_info_extension, | ||
| updated_at: env.block.time, | ||
| }; | ||
| collection_info.extension.validate()?; |
There was a problem hiding this comment.
... collection info validation during instantiation and ...
packages/cw721/src/execute.rs
Outdated
| if let Some(symbol) = msg.symbol { | ||
| collection_info.symbol = symbol; | ||
| } | ||
| collection_info.extension = collection_info.extension.update(&msg.extension)?; |
There was a problem hiding this comment.
... update collection info during execution ...
packages/cw721/src/state.rs
Outdated
| impl Update<CollectionInfoExtensionMsg<RoyaltyInfo>> for CollectionInfoExtension<RoyaltyInfo> { | ||
| fn update( | ||
| &self, | ||
| msg: &CollectionInfoExtensionMsg<RoyaltyInfo>, | ||
| ) -> Result<Self, crate::error::Cw721ContractError> { | ||
| let mut extension = self.clone(); | ||
| // validate royalty before updating | ||
| if let Some(royalty_info) = &extension.royalty_info { | ||
| royalty_info.validate(msg.royalty_info.clone())?; | ||
| } | ||
| extension.description = msg.description.clone().unwrap_or(self.description.clone()); | ||
| extension.image = msg.image.clone().unwrap_or(self.image.clone()); | ||
| extension.external_link = msg.external_link.clone().or(self.external_link.clone()); | ||
| extension.explicit_content = msg.explicit_content.or(self.explicit_content); | ||
| extension.start_trading_time = msg.start_trading_time.or(self.start_trading_time); | ||
| extension.royalty_info = msg.royalty_info.clone().or(self.royalty_info.clone()); | ||
| extension.validate()?; | ||
|
|
||
| Ok(extension) | ||
| } | ||
| } |
There was a problem hiding this comment.
... and in implementation for Update trait it does a validation. This way it is always guaranteed there is no invalid data. Custom contracts dont need to do this extra check anymore.
packages/cw721/src/state.rs
Outdated
| impl Update<NftInfo<Metadata>> for NftInfo<Metadata> { | ||
| fn update(&self, msg: &NftInfo<Metadata>) -> Result<Self, crate::error::Cw721ContractError> { | ||
| msg.validate()?; | ||
| Ok(msg.clone()) | ||
| } | ||
| } | ||
|
|
||
| impl<TMetadataExtension> Validate for NftInfo<TMetadataExtension> | ||
| where | ||
| TMetadataExtension: Validate, | ||
| { |
There was a problem hiding this comment.
Same applies for NftInfo and ...
packages/cw721/src/state.rs
Outdated
| impl Validate for Metadata { | ||
| fn validate(&self) -> Result<(), Cw721ContractError> { | ||
| // check URLs | ||
| if let Some(image) = &self.image { | ||
| Url::parse(image)?; | ||
| } | ||
| if let Some(url) = &self.external_url { | ||
| Url::parse(url)?; | ||
| } | ||
| if let Some(animation_url) = &self.animation_url { | ||
| Url::parse(animation_url)?; | ||
| } | ||
| if let Some(youtube_url) = &self.youtube_url { | ||
| Url::parse(youtube_url)?; | ||
| } | ||
| // Strings must not be empty | ||
| if let Some(image_data) = &self.image_data { | ||
| if image_data.is_empty() { | ||
| return Err(Cw721ContractError::MetadataImageDataEmpty {}); | ||
| } | ||
| } | ||
| if let Some(desc) = &self.description { | ||
| if desc.is_empty() { | ||
| return Err(Cw721ContractError::MetadataDescriptionEmpty {}); | ||
| } | ||
| } | ||
| if let Some(name) = &self.name { | ||
| if name.is_empty() { | ||
| return Err(Cw721ContractError::MetadataNameEmpty {}); | ||
| } | ||
| } | ||
| if let Some(background_color) = &self.background_color { | ||
| if background_color.is_empty() { | ||
| return Err(Cw721ContractError::MetadataBackgroundColorEmpty {}); | ||
| } | ||
| } | ||
| // check traits | ||
| if let Some(attributes) = &self.attributes { | ||
| for attribute in attributes { | ||
| if attribute.trait_type.is_empty() { | ||
| return Err(Cw721ContractError::TraitTypeEmpty {}); | ||
| } | ||
| if attribute.value.is_empty() { | ||
| return Err(Cw721ContractError::TraitValueEmpty {}); | ||
| } | ||
| if let Some(display_type) = &attribute.display_type { | ||
| if display_type.is_empty() { | ||
| return Err(Cw721ContractError::TraitDisplayTypeEmpty {}); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl Update<MetadataMsg> for Metadata { |
There was a problem hiding this comment.
... and Metadata.
| fn proper_collection_info_initialization() { | ||
| // case 1: extension set with proper data | ||
| { | ||
| let mut deps = mock_dependencies(); |
There was a problem hiding this comment.
testing update and validate for collection info during instantiation
| #[test] | ||
| fn proper_collection_info_update() { | ||
| // case 1: update with proper data | ||
| { | ||
| // initialize contract | ||
| let mut deps = mock_dependencies(); | ||
| let env = mock_env(); | ||
| let info = mock_info(CREATOR_ADDR, &[]); | ||
| let instantiated_extension = Some(CollectionInfoExtension { |
There was a problem hiding this comment.
testing update and validate for collection info during execution
| #[test] | ||
| fn test_migrate() { | ||
| let mut deps = mock_dependencies(); | ||
|
|
||
| let env = mock_env(); | ||
| use cw721_base_016 as v16; | ||
| v16::entry::instantiate( |
There was a problem hiding this comment.
migration test and especially how minter, creator, collection info looks like BEFORE and AFTER migration
| #[test] | ||
| fn mint_with_metadata() { | ||
| // case 1: mint with valid metadata | ||
| { | ||
| let mut deps = mock_dependencies(); | ||
| let contract = setup_contract(deps.as_mut()); | ||
|
|
||
| let token_id = "1".to_string(); | ||
| let token_uri = "ipfs://foo.bar".to_string(); | ||
| let mint_msg = Cw721ExecuteMsg::Mint { | ||
| token_id: token_id.clone(), | ||
| owner: String::from("medusa"), | ||
| token_uri: Some(token_uri.clone()), | ||
| extension: Some(Metadata { | ||
| image: Some("ipfs://foo.bar/image.png".to_string()), | ||
| image_data: Some("image data".to_string()), | ||
| external_url: Some("https://github.com".to_string()), | ||
| description: Some("description".to_string()), | ||
| name: Some("name".to_string()), | ||
| attributes: Some(vec![Trait { | ||
| trait_type: "trait_type".to_string(), | ||
| value: "value".to_string(), | ||
| display_type: Some("display_type".to_string()), | ||
| }]), | ||
| background_color: Some("background_color".to_string()), | ||
| animation_url: Some("ssl://animation_url".to_string()), | ||
| youtube_url: Some("file://youtube_url".to_string()), | ||
| }), | ||
| }; | ||
|
|
||
| let info_minter = mock_info(MINTER_ADDR, &[]); | ||
| let env = mock_env(); | ||
| contract | ||
| .execute(deps.as_mut(), env.clone(), info_minter, mint_msg) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
happy path testing on contract level, where new traits Update and Validate are taken into account. So minting now checks new features and make sure e.g. URLs are correct, Strings are not empty - in case Some(string) is provided.
| // case 2: mint with invalid metadata | ||
| { | ||
| let mut deps = mock_dependencies(); | ||
| let contract = setup_contract(deps.as_mut()); | ||
|
|
||
| let token_id = "1".to_string(); | ||
| let token_uri = "ipfs://foo.bar".to_string(); | ||
| let info_minter = mock_info(MINTER_ADDR, &[]); | ||
| let env = mock_env(); | ||
|
|
||
| let valid_metadata = Metadata { | ||
| image: Some("ipfs://foo.bar/image.png".to_string()), | ||
| image_data: Some("image data".to_string()), | ||
| external_url: Some("https://github.com".to_string()), | ||
| description: Some("description".to_string()), | ||
| name: Some("name".to_string()), | ||
| attributes: Some(vec![Trait { | ||
| trait_type: "trait_type".to_string(), | ||
| value: "value".to_string(), | ||
| display_type: Some("display_type".to_string()), | ||
| }]), | ||
| background_color: Some("background_color".to_string()), | ||
| animation_url: Some("ssl://animation_url".to_string()), | ||
| youtube_url: Some("file://youtube_url".to_string()), | ||
| }; | ||
|
|
||
| // invalid image | ||
| let mut metadata = valid_metadata.clone(); | ||
| metadata.image = Some("invalid".to_string()); |
There was a problem hiding this comment.
deep happy path tests down to onchain metadata level! Like it checks whether metadata.image has an invalid URL!
taitruong
changed the title
CollectionInfo, including RoyaltyInfo and creator (along with existing minter).CollectionMetadata, including RoyaltyInfo and creator (along with existing minter).
packages/cw721/src/state.rs
Outdated
| pub struct CollectionMetadata<TCollectionMetadataExtension> { | ||
| pub name: String, | ||
| pub symbol: String, | ||
| pub extension: TCollectionMetadataExtension, | ||
| pub updated_at: Timestamp, | ||
| } |
There was a problem hiding this comment.
This is the new CollectionInfo which replaces...
packages/cw721/src/query.rs
Outdated
| pub struct ContractInfoResponse { | ||
| pub name: String, | ||
| pub symbol: String, |
There was a problem hiding this comment.
... ContractInfoResponse.
packages/cw721/src/query.rs
Outdated
| pub trait Cw721Query< | ||
| // Metadata defined in NftInfo. | ||
| TNftMetadataExtension, | ||
| // Extension defined in CollectionMetadata. | ||
| TCollectionMetadataExtension, | ||
| > where | ||
| TNftMetadataExtension: Cw721State, | ||
| TCollectionMetadataExtension: Cw721State, | ||
| { | ||
| fn query( | ||
| &self, | ||
| deps: Deps, | ||
| env: &Env, | ||
| msg: Cw721QueryMsg<TNftMetadataExtension, TCollectionMetadataExtension>, | ||
| ) -> StdResult<Binary> { | ||
| match msg { | ||
| #[allow(deprecated)] | ||
| Cw721QueryMsg::Minter {} => to_json_binary(&self.query_minter(deps.storage)?), | ||
| #[allow(deprecated)] | ||
| Cw721QueryMsg::ContractInfo {} => { | ||
| to_json_binary(&self.query_collection_metadata(deps, env)?) | ||
| } | ||
| Cw721QueryMsg::GetCollectionMetadata {} => { | ||
| to_json_binary(&self.query_collection_metadata(deps, env)?) | ||
| } |
There was a problem hiding this comment.
All logic from cw721-base move to trait in default implementations. Here with query entry point, used by contracts e.g. in lib.rs
packages/cw721/src/execute.rs
Outdated
| pub trait Cw721Execute< | ||
| // 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 + StateFactory<TNftMetadataExtension>, | ||
| TCollectionMetadataExtension: Cw721State, | ||
| TCollectionMetadataExtensionMsg: Cw721CustomMsg + StateFactory<TCollectionMetadataExtension>, | ||
| TCustomResponseMsg: CustomMsg, | ||
| { | ||
| fn instantiate( | ||
| &self, | ||
| deps: DepsMut, | ||
| env: &Env, | ||
| info: &MessageInfo, | ||
| msg: Cw721InstantiateMsg<TCollectionMetadataExtensionMsg>, | ||
| contract_name: &str, | ||
| contract_version: &str, | ||
| ) -> Result<Response<TCustomResponseMsg>, Cw721ContractError> { | ||
| cw2::set_contract_version(deps.storage, contract_name, contract_version)?; | ||
| let config = Cw721Config::< | ||
| TNftMetadataExtension, | ||
| TNftMetadataExtensionMsg, | ||
| TCollectionMetadataExtension, | ||
| TCollectionMetadataExtensionMsg, | ||
| TCustomResponseMsg, | ||
| >::default(); |
There was a problem hiding this comment.
Cw721Execute trait with entry points for instantiate, execute, query, and migrate. Also in both both traits specific calls define, like in...
packages/cw721/src/execute.rs
Outdated
| fn transfer_nft( | ||
| &self, | ||
| deps: DepsMut, | ||
| env: &Env, | ||
| info: &MessageInfo, | ||
| recipient: String, | ||
| token_id: String, | ||
| ) -> Result<Response<TCustomResponseMsg>, Cw721ContractError> { | ||
| _transfer_nft::<TNftMetadataExtension>(deps, env, info, &recipient, &token_id)?; | ||
|
|
||
| Ok(Response::new() | ||
| .add_attribute("action", "transfer_nft") | ||
| .add_attribute("sender", info.sender.to_string()) | ||
| .add_attribute("recipient", recipient) | ||
| .add_attribute("token_id", token_id)) | ||
| } |
There was a problem hiding this comment.
... transfer_nft default implementation.
packages/cw721/src/execute.rs
Outdated
| let collection_metadata = msg.create(deps.as_ref(), env, info, Some(¤t))?; | ||
| config | ||
| .collection_metadata | ||
| .save(deps.storage, &collection_metadata)?; |
There was a problem hiding this comment.
... where in update_collection_metadata where on msg, a single line creates a new, updated state:
let collection_metadata = msg.create(deps.as_ref(), env, info, Some(¤t))?;
And create() in StateFactory does a deep creation and validation: ...
packages/cw721/src/state.rs
Outdated
| if self.name.is_some() && self.name.clone().unwrap().is_empty() { | ||
| return Err(Cw721ContractError::CollectionNameEmpty {}); | ||
| } | ||
| if self.symbol.is_some() && self.symbol.clone().unwrap().is_empty() { | ||
| return Err(Cw721ContractError::CollectionSymbolEmpty {}); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
... here in validate() in makes sure that name and symbol in CollectionMetadata is not empty.
packages/cw721/src/state.rs
Outdated
| fn create( | ||
| &self, | ||
| deps: Deps, | ||
| env: &cosmwasm_std::Env, | ||
| info: &cosmwasm_std::MessageInfo, | ||
| current: Option<&CollectionMetadata<TCollectionMetadataExtension>>, | ||
| ) -> Result<CollectionMetadata<TCollectionMetadataExtension>, Cw721ContractError> { | ||
| self.validate(deps, env, info, current)?; | ||
| match current { | ||
| // Some: update existing metadata | ||
| Some(current) => { | ||
| let mut updated = current.clone(); | ||
| if let Some(name) = &self.name { | ||
| updated.name = name.clone(); | ||
| } |
There was a problem hiding this comment.
... then in create of StateFactory, it:
- validates msg first,
- uses current state and
- update it with msg data
... then it goes one level deeper, ...
packages/cw721/src/state.rs
Outdated
| let current_extension = current.extension.clone(); | ||
| let updated_extension = | ||
| self.extension | ||
| .create(deps, env, info, Some(¤t_extension))?; | ||
| updated.extension = updated_extension; | ||
| Ok(updated) |
There was a problem hiding this comment.
... and creates CollectionMedataExtension, including validation of optional props like description, royalties, start trading time, etc.
| fn validate( | ||
| &self, | ||
| deps: Deps, | ||
| _env: &Env, | ||
| info: &MessageInfo, | ||
| _current: Option<&CollectionMetadataExtension<RoyaltyInfo>>, | ||
| ) -> Result<(), Cw721ContractError> { | ||
| // start trading time can only be updated by minter | ||
| let minter_initialized = MINTER.item.may_load(deps.storage)?; | ||
| if self.start_trading_time.is_some() | ||
| && minter_initialized.is_some() | ||
| && MINTER.assert_owner(deps.storage, &info.sender).is_err() | ||
| { | ||
| return Err(Cw721ContractError::NotMinter {}); | ||
| } | ||
| // all other props collection metadata extension can only be updated by the creator | ||
| let creator_initialized = CREATOR.item.may_load(deps.storage)?; | ||
| if (self.description.is_some() | ||
| || self.image.is_some() | ||
| || self.external_link.is_some() | ||
| || self.explicit_content.is_some() | ||
| || self.royalty_info.is_some()) | ||
| && creator_initialized.is_some() | ||
| && CREATOR.assert_owner(deps.storage, &info.sender).is_err() | ||
| { | ||
| return Err(Cw721ContractError::NotCollectionCreator {}); | ||
| } | ||
| // check description length, must not be empty and max 512 chars | ||
| if let Some(description) = &self.description { | ||
| if description.is_empty() { | ||
| return Err(Cw721ContractError::CollectionDescriptionEmpty {}); | ||
| } | ||
| if description.len() > MAX_COLLECTION_DESCRIPTION_LENGTH as usize { | ||
| return Err(Cw721ContractError::CollectionDescriptionTooLong { | ||
| max_length: MAX_COLLECTION_DESCRIPTION_LENGTH, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pls also note that StateFactory can also do complex validation. Like here in CollectionMetadataExtension:
start_trading_timecan only be updated by Minter- all other probs belongs to and can only be updated by Creator 😎
In short: all state-based logic can be handled in StateFactory.
…rationContract for ease-of-use
…a-onchein, cleanup
taitruong
force-pushed
the
collection-info
branch
from
df8ebc8 to
a996184
Compare
4 participants
Major refactoring for upcoming v0.19 release.
TL;DR: v0.19 release is a major refactoring, introducing distinctions between Creator (manages collection metadata like royalties) and Minter (controls minting), and extends
UpdateNftInfowith extension for onchain metadata. We've also added a newCollectionInfostructure 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
CreatorandMinterIn previous release
cw721is only aware of minter (aka owner/minter ownership). Here we addedCreator. Creator controls collection info (like royalties) whereas minter controls minting.NEW utility:
UpdateNftInfoandUpdateCollectionInfomsgBy adding a new
CollectionInfoExtensionstore as an optional extension forCollectionInfo(name and symbol), there is also logic introduced here for updating collection metadata.The creator is now able to do both: updating collection and NFT Info!
NEW
CollectionMedataincw721packageFor making it available to all existing contracts, I had to move all core logic into
cw721package. As a result, outcome is the following:Cw721ExecuteandCw721QuerytraitsAnother upgrade is adding
creatoralong to existingminteraddress. In previous release due to the introduction ofcw-ownablethere has been a renaming ofmintertoownership- which is confusing. With upcoming v0.19 release it looks like this:ownershipin v0.17-18)ownership(deprecated)start_trading_timewhich belongs to minterStateFactoryall stores are in
state.rs(pls note newCw721Configwhich will be used for contracts):Essentially all boilerplate code (default implementations in
Cw721ExecuteandCw721Querytraits) are now incw721package.As a result contracts are very slim. Please note, implementations by
Cw721Config, along withCw721ExecuteandCw721Querytraits are opionated - though contracts may be implement differently by overriding default implementations.Here is how new
cw721-baselooks like by usingcw721package:Define struct for contract interaction:
Then implement execute and query traits and using default implementations:
And finally in lib.rs:
That's it!
cw721-metadata-onchainandcw2981-royaltycontracts removedAnother positive side effect of this refactoring is that there's no need for
cw721-metadata-onchainandcw2981-royaltycontracts anymore. This is covered incw721-base. Design hasnt changed and onchain metadata is stored inNftInfo<TNftMetadataExtension>. Before this PR, funny thing is that incw721-baseit was defined as:TMetadataExtension = Option<Empty>, allowing to NOT store onchain data, but it doesnt make sense havingNoneandSome(Empty).In
cw721-metadata-onchaincontract 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 incw721-base. onchain contract is obsolete and is removed.