Migration from sg721 to cw721 `CollectionInfo` #668

taitruong · 2024-03-19T14:29:12Z

Migration from sg721 to cw721 `CollectionInfo`

All changes in this PR is based on new cw721 package. Check for PR in cw-nfts repo here.

There are these naming conventions for metadata in cw721: collection info, nft info, and extensions:

collection info: collection infos (aka contract info) is required for a contract (containing name and symbol)
collection extension: optional extension (containing description, image, etc.)
nft extension: is optional metadata for an nft

Here sg721-base is using new cw721-base with collection info. CollectionInfo (sg721) is migrated to CollectionExtension 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:

#[cw_serde]
#[deprecated = "Please use `CollectionInfo<CollectionExtensionResponse<RoyaltyInfo>>` instead"]
pub struct CollectionInfoResponse {
...
}

pub enum QueryMsg<
     // Return type of NFT metadata defined in `NftInfo` and `AllNftInfo`.
    TNftExtension,
    // Return type of collection extension defined in `GetCollectionInfo`.
    TCollectionExtension,
    // Custom query msg for custom contract logic. Default implementation returns an empty binary.
    TExtensionQueryMsg,
> {
    #[allow(deprecated)]
    #[returns(CollectionInfoResponse)]
     #[deprecated = "Please use GetCollectionInfo instead"]
    CollectionInfo {},
...
}

pub enum ExecuteMsg<pub enum ExecuteMsg<
    // NftInfo extension msg for onchain metadata.
    TNftExtensionMsg,
    // CollectionInfo extension msg for onchain collection attributes.
    TCollectionExtensionMsg,
    // Custom extension msg for custom contract logic. Default implementation is a no-op.
    TExtensionMsg,
> {
    // ---- sg721 specific msgs ----
    /// Update specific collection info fields
    #[deprecated = "Please use Cw721UpdateCollectionInfo instead"]
    UpdateCollectionInfo {
        #[allow(deprecated)]
        collection_info: UpdateCollectionInfoMsg<RoyaltyInfoResponse>,
    },
...
}

#[cw_serde]
#[deprecated = "Please use `UpdateCollectionInfo<DefaultOptionalCollectionExtensionMsg>` instead"]
pub struct UpdateCollectionInfoMsg<T> {
...
}

There are also some helper code like:

#[allow(deprecated)]
impl From<InstantiateMsg> for Cw721InstantiateMsg<DefaultOptionalCollectionExtensionMsg> {
...
}

impl<TNftExtensionMsg, TCollectionExtensionMsg>
    From<ExecuteMsg<TNftExtensionMsg, TCollectionExtensionMsg>> for Sg721ExecuteMsg
...

impl From<QueryMsg>
    for Sg721QueryMsg<DefaultOptionalNftExtension, DefaultOptionalCollectionExtension, Empty>
...

impl From<CollectionInfo<RoyaltyInfoResponse>> for DefaultOptionalCollectionExtensionMsg {
...
}

impl From<UpdateCollectionInfoMsg<RoyaltyInfoResponse>>
    for CollectionExtensionMsg<RoyaltyInfoResponse>
...

impl From<InstantiateMsg> for Cw721InstantiateMsg<DefaultOptionalCollectionExtensionMsg> {
...
}

sg721 derivatives

sg721-metadata-onchain has been removed. new cw721-base supports it by default - hence sg721-base as well.

TBD: sg721-updatable could be removed, since cw721-base also supports this (UpdateNftInfo). sg721-updatable only updates token uri, while cw721-base also allows updating onchain metadata:

pub enum Cw721ExecuteMsg<
    // Message passed for updating metadata.
    TNftMetadataExtensionMsg,
    // Message passed for updating collection metadata extension.
    TCollectionMetadataExtensionMsg,
> {
...
/// The creator is the only one eligible to update NFT's token uri and onchain metadata (`NftInfo.extension`).
    /// NOTE: approvals and owner are not affected by this call, since they belong to the NFT owner.
    UpdateNftInfo {
        token_id: String,
        token_uri: Option<String>,
        extension: TNftMetadataExtensionMsg,
    },
...
}

new `StateFactory`

As recently discussed there was a bug in royalty check:

during instantiation, royalty can be set to max 100%
during execution (update), royalty can be set to max 10%

A reason how this bug wasnt obivious is that royalty validation happend in 2 different places.

In cw721 a new StateFactory has been introduced. Check PR comment here.

humanalgorithm · 2024-03-27T14:11:47Z

contracts/collections/sg721-base/src/contract.rs

            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 {


How come we can delete all this royalty calculation?

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

humanalgorithm · 2024-03-27T14:13:14Z

contracts/collections/sg721-base/src/msg.rs

+    #[returns(AllInfoResponse)]
+    GetAllInfo {},
+
+    /// Returns `CollectionExtensionAttributes`


Can we remove these comments when it just says the same thing as the code/doesn't add any information?

This is for generating docs in JSON files. So we must keep this.

humanalgorithm · 2024-03-27T14:16:50Z

packages/sg721/src/lib.rs

+> {
+    // ---- sg721 specific msgs ----
+    /// Update specific collection info fields
+    #[deprecated = "Please use Cw721UpdateCollectionInfo instead"]


A lot of allow deprecated everywhere. What would be the condition in which we can remove these tags? Will it ever meet that condition?

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.

humanalgorithm · 2024-03-27T14:17:30Z

packages/sg721/src/lib.rs

-    FreezeCollectionInfo,
+}
+
+impl<TNftExtensionMsg, TCollectionExtensionMsg, TExtensionMsg>


Is lib.rs the right place for this?

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.

humanalgorithm · 2024-03-27T14:21:08Z

contracts/collections/sg721-base/src/contract.rs

+        // NftInfo extension (onchain metadata).
+        TNftExtension,
+        // NftInfo extension msg for onchain metadata.
+        TNftExtensionMsg,


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?

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)

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.

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,

humanalgorithm · 2024-03-27T14:22:26Z

contracts/collections/sg721-updatable/src/contract.rs

-pub type Sg721UpdatableContract<'a> = Sg721Contract<'a, Extension>;
+pub type Sg721UpdatableContract<'a> = Sg721Contract<
+    'a,
+    DefaultOptionalNftExtension,


What is meant by "default" ?

It is defined in cw721 package: pub type DefaultOptionalNftExtension = Option<NftExtension>;

So it is optional onchain metadata.

taitruong · 2024-03-27T16:28:59Z

contracts/collections/sg721-base/src/contract.rs

+        self.parent
+            .update_collection_info(deps, Some(&info), Some(&env), msg)?;


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(&current))?;

By calling create() StateFactory comes into account.

taitruong · 2024-09-25T17:36:31Z

contracts/collections/sg721-base/src/contract.rs

-
-        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(


@MightOfOaks check this out for migration!

taitruong changed the title ~~WIP - use collection metadata from cw721~~ Migration from CollectionInfo (sg721) to CollectionMetadata (cw721) Mar 20, 2024

humanalgorithm requested review from jhernandezb and humanalgorithm March 22, 2024 06:44

taitruong changed the title ~~Migration from CollectionInfo (sg721) to CollectionMetadata (cw721)~~ Migration from sg721 to cw721 CollectionInfo Mar 24, 2024

taitruong added 17 commits March 26, 2024 14:53

migrate using royalty info from new cw721-base

17cafde

fix tests

67635de

fix tests

63d2546

fix payout_too_much_royalties: fees exceed payment

ce3448b

#[allow(deprecated)]

9997830

cleanup

cb9eb6a

migrating is always possible, no version check needed

c2ffe15

remove redundant contract name and version migration

3d71f98

rename

abd5cf1

cleanup

ee316f2

migrate collection info (sg721) to collection metadata (cw721)

d073690

cleanup

ecf23f9

update cw721

00e916f

update lib

7cbefc6

update lib

98fd413

final cleanup, docs, renamings, update libs

52bedde

docs

69b65e2

taitruong force-pushed the collection_info branch from e02d2ac to 69b65e2 Compare March 26, 2024 13:58

humanalgorithm requested a review from shanev March 27, 2024 14:06

humanalgorithm reviewed Mar 27, 2024

View reviewed changes

taitruong commented Mar 27, 2024

View reviewed changes

taitruong added 3 commits March 27, 2024 19:01

update lib

5cf7da8

use cw721 for updating token metadata

1db0bb6

update lib

a0cf2f7

taitruong mentioned this pull request Apr 25, 2024

Move to cw721 package public-awesome/cw-nfts#161

Merged

taitruong commented Sep 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration from sg721 to cw721 `CollectionInfo` #668

Migration from sg721 to cw721 `CollectionInfo` #668

taitruong commented Mar 19, 2024 •

edited

Loading

humanalgorithm Mar 27, 2024

taitruong Mar 27, 2024

humanalgorithm Mar 27, 2024

taitruong Mar 27, 2024

humanalgorithm Mar 27, 2024

taitruong Mar 27, 2024

humanalgorithm Mar 27, 2024

taitruong Mar 27, 2024 •

edited

Loading

humanalgorithm Mar 27, 2024

taitruong Mar 27, 2024 •

edited

Loading

taitruong Mar 27, 2024 •

edited

Loading

taitruong Mar 28, 2024 •

edited

Loading

humanalgorithm Mar 27, 2024

taitruong Mar 27, 2024

taitruong Mar 27, 2024

taitruong Sep 25, 2024

		self.parent
		.update_collection_info(deps, Some(&info), Some(&env), msg)?;

Migration from sg721 to cw721 CollectionInfo #668

Are you sure you want to change the base?

Migration from sg721 to cw721 CollectionInfo #668

Conversation

taitruong commented Mar 19, 2024 • edited Loading

Migration from sg721 to cw721 CollectionInfo

Backward Compatibility

sg721 derivatives

new StateFactory

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

taitruong Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

taitruong Mar 28, 2024 • edited Loading

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

Migration from sg721 to cw721 `CollectionInfo` #668

Migration from sg721 to cw721 `CollectionInfo` #668

taitruong commented Mar 19, 2024 •

edited

Loading

Migration from sg721 to cw721 `CollectionInfo`

new `StateFactory`

taitruong Mar 27, 2024 •

edited

Loading

taitruong Mar 27, 2024 •

edited

Loading

taitruong Mar 27, 2024 •

edited

Loading

taitruong Mar 28, 2024 •

edited

Loading