public-awesome / cw-nfts

Examples and helpers to build NFT contracts on CosmWasm
Apache License 2.0
186 stars 179 forks source link

#151 make minter optional #152

Closed taitruong closed 6 months ago

taitruong commented 6 months ago

PR for #151

Rather prefer having minter: Option<String>: https://github.com/CosmWasm/cw-nfts/blob/177a993dfb5a1a3164be1baf274f43b1ca53da53/contracts/cw721-base/src/msg.rs#L8-L18

In case of none, info.sender should be minter. Makes it easier for other contracts (e.g. minter) re-using this msg.

In most cases creator of contract is also minter. Rn creator (=sender) instantiates contract AND need to provides its own address in minter prop. You can see this in unit tests:

Before: duplicate, since creator is in info.sender and minter.prop

        let info = mock_info(CREATOR, &[]);
        let init_msg = InstantiateMsg {
            name: "SpaceShips".to_string(),
            symbol: "SPACE".to_string(),
            minter: CREATOR.to_string(),
            withdraw_address: None,
        };
        entry::instantiate(deps.as_mut(), mock_env(), info.clone(), init_msg).unwrap();

After: no need to provider minter prop, since creator is sender:

        let info = mock_info(CREATOR, &[]);
        let init_msg = InstantiateMsg {
            name: "SpaceShips".to_string(),
            symbol: "SPACE".to_string(),
            minter: None,
            withdraw_address: None,
        };
        entry::instantiate(deps.as_mut(), mock_env(), info.clone(), init_msg).unwrap();

Instantiation is like this then:

like before, in case caller should NOT be minter/owner (edge case):

{"name": "foo", "symbol": "bar", "minter": "other_than_caller"}

for creator (standard, 99.99% case):

{"name": "foo", "symbol": "bar"}

@shanev @JakeHartnell

taitruong commented 6 months ago

You can also see from adjusted tests - it looks more convenient and natural.

taitruong commented 6 months ago

Reason, why I came to this conclusion is this here: https://github.com/public-awesome/launchpad/blob/main/contracts/collections/sg721-base/src/contract.rs#L53-L58

        // cw721 instantiation
        let info = CW721ContractInfoResponse {
            name: msg.name,
            symbol: msg.symbol,
        };
        self.parent.contract_info.save(deps.storage, &info)?;
        cw_ownable::initialize_owner(deps.storage, deps.api, Some(&msg.minter))?;

All this logic can completely be removed, by calling parent.instantiate. Also these 3 props can be replaced by cw721_base::msg::InstantiateMsg: https://github.com/public-awesome/launchpad/blob/main/packages/sg721/src/lib.rs#L118-L120

pub struct InstantiateMsg {
    pub name: String,
    pub symbol: String,
    pub minter: String,
    pub collection_info: CollectionInfo<RoyaltyInfoResponse>,
}

@jhernandezb

taitruong commented 6 months ago

@shanev need one more approval, then we can merge and release 0.19.0 (#150)