Make cw721-base minter updatable via two-step ownership transfer.

[0xekez]

This makes a breaking change to cw721-base in that it makes the minter field on cw721_base::MinterResponse optional.

Before:

pub struct MinterResponse {
    pub minter: String,
}

After:

pub struct MinterResponse {
    pub minter: Option<String>,
}

It also makes a breaking change to the ContractError enum in that it removes the Unauthorized variant in favor of cw-ownable's more descriptive OwnershipError.

It then extends the cw721_base, but not the cw721 standard's, ExecuteMsg with
cw-ownable's UpdateOwnership enum variant, and extends the QueryMsg with cw-ownable's Ownership {} query. Using these messages, the current minter on the contract may nominate a new minter, who may accept or reject this nomination.

[0xekez]

@shanev , @JakeHartnell , @larry0x - i'd be interested in your feedback about this.

we'd like to use something like this in DAO DAO to allow NFT DAOs to bootstrap themselves. ICS-721 would possibly also benefit from a more flexible ownership scheme as it could allow the owner to control rate-limits and other parameters relevant to their collection moving to other chains (where they could lose royalty protections, for example, when leaving Stargaze).

the change i've made here tries to limit API breakage by not renaming the minter field on the InstantiateMsg. we could further break the API by changing that field to owner. at the moment, $minter \iff owner$.

[JakeHartnell]

we could further break the API by changing that field to owner.

Damn, I really want to do this but feel it would be a pain for a lot of folks...

[larry0x]

Yay cw-ownable adoption!

[larry0x]

we could further break the API by changing that field to owner.

Damn, I really want to do this but feel it would be a pain for a lot of folks...

Opinions from @shanev @jhernandezb should be the most important regarding API breakages... What do you guys think about:

• renaming the minter field in instantiate msg
• delete QueryMsg::Minter and use QueryMsg::Ownership instead

[larry0x]

@0xekez I think you can consider adding rust-version = "1.65" in cw721-base's Cargo.toml to enforce a minimum Rust version

(For context: cw-ownable uses a syntax only available since Rust 1.65)

[shanev]

Fully support this since Stargaze NFT DAOs will also need this to bootstrap themselves. My only concern is that use of the term ownership can be confused with NFT ownership.

[jhernandezb]

I think it would be nice to have an example of how to migrate a contract from previous versions in case someone misses it and leaves a contract uninitialized.

[larry0x]

I think it would be nice to have an example of how to migrate a contract from previous versions in case someone misses it and leaves a contract uninitialized.

The way I suggest to do this is to create an src/migrations folder with this structure:

contracts/
└── cw721-base/
    └── src/
        └── migrations/
            ├── mod.rs
            ├── v0_16.rs
            ├── v0_17.rs
            ├── v1_0.rs
            └── v1_1.rs

Each v*_*.rs exports a single migrate function that contains the logic for migrating from previous version to the current version. For example, v0_17.rs will contain the logic for migrating v0.16 to v0.17.

To migrate a custom cw721 contract from v0.16 to v0.17, simply do:

#[entry_point]
fn migrate(deps: Deps, env: Env, msg: MigrateMsg) -> Result<Response, ContractError> {
    cw721_base::migrations::v0_17::migrate(deps, env, msg)?;
    Ok(Response::new())
}

[larry0x]

I can imagine this would be tricky for Stargaze tho... Since each NFT collection is a separate contract, we have to migrate each one of them. It may be useful to create an sdk module that registers each collection, and provide a governance-gated message to migrate them all together.

[jhernandezb]

I can imagine this would be tricky for Stargaze tho... Since each NFT collection is a separate contract, we have to migrate each one of them. It may be useful to create an sdk module that registers each collection, and provide a governance-gated message to migrate them all together.

I governance proposal would be a nice one to have, currently creators have admin and can do the migration themselves through our studio UI. Your proposed solution sounds good to me

[0xekez]

@jhernandezb: I think it would be nice to have an example of how to migrate a contract from previous versions in case someone misses it and leaves a contract uninitialized.

good idea, i've added migration logic and added this to the README:

Updating the minter

For contract versions $> 0.16$ the minter has been replaced by an
owner which is updatable via the two-step ownership transfer process
of cw_ownable. To retrieve the
owner, QueryMsg::Ownership {} may be executed.

QueryMsg::Minter {} has not been removed, though after version 0.16
the response type has made the minter field optional as it may be
unset. For all intents and purposes, whenever the word minter is used,
it means owner, and owner in turn means minter, $minter \iff owner$.

Before 0.16:

pub struct MinterResponse {
    pub minter: String,
}

After 0.16:

pub struct MinterResponse {
    pub minter: Option<String>,
}

NFTs on version 0.16 may upgrade via MigrateMsg::From016 {}. For an
example of doing so, see
this
integration test.

[0xekez]

i'll merge this tomorrow if there are no more comments before then

[larry0x]

@0xekez The additions of MIGRATING.md and v0_16.rs look good! I left a two comments.

[larry0x]

@0xekez @JakeHartnell I'd say this PR is ready to be merged! However, please don't cut a release for 0.17 yet. I say we include #106, perhaps also #107, and I also have a suggestion to how we do cw2 versioning in cw721-base which I'll do a PR shortly.

[JakeHartnell]

I say we include #106, perhaps also #107, and I also have a suggestion to how we do cw2 versioning in cw721-base which I'll do a PR shortly.

Completely agree.