Skip to content
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

Prevent repetitive write of cw2 version info during instantiation #110

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

larry0x
Copy link
Collaborator

@larry0x larry0x commented Feb 15, 2023

Background

To understand what the problem is, let's first talk about how to extend/inherit/customize the base cw721 contract. Using Rust's "composition" pattern, the way to do this is:

#[derive(Default)]
struct<'a> MyCustomCw721Contract<'a> {
    // for simplicity, I use Empty for all extensions in this example
    pub base: Cw721Contract<'a, Empty, Empty, Empty, Empty>,

    // my custom fields here...
}

(this is what we do with stargaze's sg721-base, btw!)

Then, during instantiation, I simply invoke the base contract's instantiate method:

#[entry_point]
fn instantiate(
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    msg: InstantiateMsg,
) -> StdResult<Response> {
    // set my custom contract version
    cw2::set_contract_version("crates.io:my-custom-nft", "1.0.0")?;

    let tract = MyCustomCw721Contract::default();

    // run the base contract's instantiate logic
    // !! WARNING: the base contract OVERWRITES my version info here !!
    tract.base.instantiate(deps, env, info, msg)?;

    // my custom instantiate logics...
}

The problem

cw721-base initializes the cw2 version info inside the Cw721Contract::instantiate method. This means, by running tract.base.instantiate, the base contract overwrites my custom cw2 info!

To avoid this, we have to run cw2::set_contract_version AFTER tract.base.instantiate. This means the cw2 storage slot is written TWICE during instantiation, once by the base contract, once by my custom contract.

This problem is present in cw721-metadata-onchain for example, see the comment on L54:

let res = Cw721MetadataContract::default().instantiate(deps.branch(), env, info, msg)?;
// Explicitly set contract name and version, otherwise set to cw721-base info
set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)
.map_err(ContractError::Std)?;
Ok(res)

The solution

The initialization of cw2 version should NOT be in Cw721Contract::instantiate, but in cw721_base::entry::instantiate. This way, by running Cw721Contract::instantiate the base contract does NOT overwrite the custom cw2 info.

@larry0x
Copy link
Collaborator Author

larry0x commented Feb 18, 2023

@JakeHartnell @0xekez @shanev plz review 🙏

Copy link
Collaborator

@shanev shanev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @larry0x! Thank you.

Copy link
Collaborator

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@JakeHartnell JakeHartnell merged commit a662fac into main Feb 18, 2023
@JakeHartnell JakeHartnell deleted the larry/cw2-fix branch February 18, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants