-
Notifications
You must be signed in to change notification settings - Fork 37
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
Migration from sg721 to cw721 CollectionInfo
#668
base: main
Are you sure you want to change the base?
Conversation
CollectionInfo
(sg721) to CollectionMetadata
(cw721)
CollectionInfo
(sg721) to CollectionMetadata
(cw721)CollectionInfo
e02d2ac
to
69b65e2
Compare
let last_royalty_update = self.royalty_updated_at.load(deps.storage)?; | ||
if last_royalty_update.plus_seconds(24 * 60 * 60) > env.block.time { | ||
return Err(ContractError::InvalidRoyalties( | ||
"Royalties can only be updated once per day".to_string(), | ||
)); | ||
} | ||
|
||
let new_royalty_info = RoyaltyInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we can delete all this royalty calculation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is imo the cool part about it, it is moved to cw721. RoyalInfoMsg implements StateFactory where in create()
it also does validate()
.
Check in other PR my comment here: https://github.com/CosmWasm/cw-nfts/pull/156/files#r1541437296
#[returns(AllInfoResponse)] | ||
GetAllInfo {}, | ||
|
||
/// Returns `CollectionExtensionAttributes` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove these comments when it just says the same thing as the code/doesn't add any information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for generating docs in JSON files. So we must keep this.
> { | ||
// ---- sg721 specific msgs ---- | ||
/// Update specific collection info fields | ||
#[deprecated = "Please use Cw721UpdateCollectionInfo instead"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of allow deprecated everywhere. What would be the condition in which we can remove these tags? Will it ever meet that condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in PR description:
...
# Backward Compatibility
For backward compatibility and not breaking 3rd parties (like frontend) legacy msgs, structs, etc are kept - but marked as deprecated.
...
So this PR can be deployed without breaking frontends. Once frontends (e.g. launchpad, marketplace) are migrated, then all deprecated types can be removed.
FreezeCollectionInfo, | ||
} | ||
|
||
impl<TNftExtensionMsg, TCollectionExtensionMsg, TExtensionMsg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is lib.rs the right place for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%, before PR changes sg721
also wasnt holding much specific data or logic - since everything is in cw721
. so lib.rs in here just binds cw721 and adds only few additional sg specific data.
// NftInfo extension (onchain metadata). | ||
TNftExtension, | ||
// NftInfo extension msg for onchain metadata. | ||
TNftExtensionMsg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE naming conventions. I'm just curious if we need to name variables this way. The comment says NFTInfo extension msg for onchain metadata. Would ie be wrong to name it NFTInfoOnchainMetadataExtensionMsg or OnchainMetadataExtensionMsg something like that? I see this pattern in a lot of variables where the description is different than the name. Or do we need these names for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this. T
is for generic. If it would be e.g. just NftExtension
than it is a specific struct. Look how Sg721Contract
is used:
Sg721Contract::<
DefaultOptionalNftExtension,
DefaultOptionalNftExtensionMsg,
DefaultOptionalCollectionExtension,
DefaultOptionalCollectionExtensionMsg,
Empty,
Empty,
Empty,
>::default()
.execute(deps, env, info, msg)
Now if Stargaze later on wants to provide another custom contract sg721, and provide a different type like MyCustomNftExtension for TNftExtension
. Then it may look like this:
Sg721Contract::<
MyCustomNftExtension,
MyCustomNftExtensionMsg, // optional custom msg, but doesnt has to
DefaultOptionalCollectionExtension,
DefaultOptionalCollectionExtensionMsg,
Empty,
Empty,
Empty,
>::default()
.execute(deps, env, info, msg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So generic TFooBar
is a placeholder for a concrete contract (lib), where lib then provides specific structs. This way you can define a new struct and implement StateFactory with special logic for creation and validation - without(!) changing whole logic or contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE naming conventions. I'm just curious if we need to name variables this way. The comment says NFTInfo extension msg for onchain metadata. Would ie be wrong to name it NFTInfoOnchainMetadataExtensionMsg or OnchainMetadataExtensionMsg something like that? I see this pattern in a lot of variables where the description is different than the name. Or do we need these names for consistency?
I should need change comment, since custom contracts can use this for any logic and type - not only for onchain metadata. This is also the reason why I used extension
(instead of metadata) as naming for all structs, methods, generics, etc.
So comment should be like:
// NftInfo extension, it may use `DefaultOptionalNftExtension` for onchain metadata, but can be of any other type.
TNftExtension,
// NftInfo extension msg, it may use `DefaultOptionalNftExtensionMsg` for onchain metadata msg, but can be of any other type.
TNftExtensionMsg,
pub type Sg721UpdatableContract<'a> = Sg721Contract<'a, Extension>; | ||
pub type Sg721UpdatableContract<'a> = Sg721Contract< | ||
'a, | ||
DefaultOptionalNftExtension, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is meant by "default" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is defined in cw721
package: pub type DefaultOptionalNftExtension = Option<NftExtension>;
So it is optional onchain metadata.
self.parent | ||
.update_collection_info(deps, Some(&info), Some(&env), msg)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here logic is delegated to parent (cw721 package), where collection info and royalty msg is passed and cw721 just do this call:
let collection_info = msg.create(deps.as_ref().into(), env, info, Some(¤t))?;
By calling create()
StateFactory comes into account.
|
||
cw2::set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; | ||
// these upgrades can always be called. migration is only executed in case new stores are empty! It is safe calling these on any version. | ||
response = crate::upgrades::v3_0_0_ownable_and_collection_info::upgrade( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MightOfOaks check this out for migration!
Migration from sg721 to cw721
CollectionInfo
All changes in this PR is based on new
cw721
package. Check for PR incw-nfts
repo here.There are these naming conventions for metadata in cw721: collection info, nft info, and extensions:
Here
sg721-base
is using newcw721-base
with collection info.CollectionInfo
(sg721) is migrated toCollectionExtension
in cw721.sg721-base
does not longer need to hold dedicated collection info store (legacy). As a backup legacy is kept - and may be removed in later versions.As a result code footprint of
sg721-base
is a lot smaller :).Backward Compatibility
For backward compatibility and not breaking 3rd parties (like frontend) legacy msgs, structs, etc are kept - but marked as deprecated. Like this:
There are also some helper code like:
sg721 derivatives
sg721-metadata-onchain
has been removed. newcw721-base
supports it by default - hencesg721-base
as well.TBD:
sg721-updatable
could be removed, sincecw721-base
also supports this (UpdateNftInfo
).sg721-updatable
only updates token uri, whilecw721-base
also allows updating onchain metadata:new
StateFactory
As recently discussed there was a bug in royalty check:
A reason how this bug wasnt obivious is that royalty validation happend in 2 different places.
In
cw721
a newStateFactory
has been introduced. Check PR comment here.