Closed 0xekez closed 1 year ago
@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$.
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...
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:
minter
field in instantiate msgQueryMsg::Minter
and use QueryMsg::Ownership
instead@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)
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.
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())
}
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 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
@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:
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.
i'll merge this tomorrow if there are no more comments before then
@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.
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.
This makes a breaking change to cw721-base in that it makes the
minter
field oncw721_base::MinterResponse
optional.Before:
After:
It also makes a breaking change to the
ContractError
enum in that it removes theUnauthorized
variant in favor of cw-ownable's more descriptiveOwnershipError
.It then extends the
cw721_base
, but not thecw721
standard's,ExecuteMsg
withcw-ownable
'sUpdateOwnership
enum variant, and extends theQueryMsg
withcw-ownable
'sOwnership {}
query. Using these messages, the current minter on the contract may nominate a new minter, who may accept or reject this nomination.