Call migrate method only when there is a change in the contract's state required

[kulikthebird]

Contract's state version in the wasm binary

Currently each time a new contract binary version is uploaded to Cosmwasm it is required to call a migrate entry point method. The purpose of this entry point is to let the contract to align the state stored on the blockchain to the new version of the contract. Lack of the migrate entry point in the contract results in an error.

However there are significantly many cases when the binary is uploaded but it doesn't change the state in the process, for e.g. bug fixing, new implementation basing on the current version of the state etc. It is a problem, because each call of a migrate method (even the one that does almost nothing) costs gas. In a big scale it might increase the costs quite significantly.

Required steps

There are two sub-issues to be resolved for this issue:

• Macro for setting the contract's state version in the compiled binary #2116
• Conditional migrate call with extra MigrateInfo argument after contract update #2117

Result

CosmWasm will provide the on-chain state version of the contract in the new message:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)]
pub struct MigrateInfo {
    pub sender: Addr,
    pub state_version_on_chain: Option<u64>,
}

Once the current version is provided, developers could implement the migration procedure in a new way. The example of the new approach:

/// This method is called only when the current state version is different
/// than the new contract's one after the contract's binary update procedure.
/// The current version can be found in `info.state_version_on_chain`
///
#[entry_point]
#[set_contract_state_version(3)]  // the new macro call here
pub fn migrate(deps: DepsMut, env: Env, info: MigrateInfo, _msg: MigrateContract) -> StdResult<Response> {
    match info.state_version_on_chain {
        Some(0) => {},
        Some(1) => {},
        Some(2) => {},
        None => {
            // Fallback to the previous method of migrating contract's state.
        }
    }
    Ok(Response::default())
}

[hashedone]

#[set_contract_info(contract_state_version=3)]

At first, I didn't like it being required, as I've seen this argument that we might want to completely not implement migration because we know the state doesn't change. Then it occurred to me it is not a problem - if the migrate is not there, we don't migrate - it is an important part here, and I don't see it being described. Finally, I think migrate is a good place for it, but I don't like how many letters there are. I'd go for something like #[state_version(3)].

What is the contract_state_version? I assume it is a state version "on chain" before migration - so the one fitting the contract code before migration, from which we are trying to migrate, right? If so, it is ok, but it is not very well explained so double checking here.

I don't really see a reason to bring any compatibility to cw2 - those are completely orthogonal. What is important to explain is that when there is no new macro used, the migrate is always called on normal migrations, as the actual "breaking" change is skipping the migrate call in particular circumstances - and we want to emphasize it is not changing for contracts already there.

In general looks good, mi big issue is the macro name. I understand that simple macro names are more likely to collide, but this one is really long. If you don't like #[state_version] then maybe like #[cw(state_version=...)] or something?

[webmaster128]

Very good stuff.

I'm not really sure what the best way is to pass the old contract_state_version. Adding to ContractInfo is breaking and the old version is not really a property of the contract but would better be an argument of the migrate call, which is breaking as well.

a conditional calling the contract's migrate method - only when the new contract's state version is higher than the one for the currently stored contract.

I would say we call migrate whenever the versions are different. This allows the contract to somehow handle downgrades (e.g. old version is 5, new version is 3). I'd probably just error for those cases in the contract but better than just skipping the migrate call.

[hashedone]

Right, I forgot ContractInfo is not non_exhaustive :/ I agree the best would be an additional argument to migrate (like MigrationInfo). Another possibility I think needs to be fixed is adding a function to the Api. It is not idiomatic, as functions should not be stateful, but this is not stateful during execution - for a single pass, it is always constant. We could add the migration_info there. We could panic outside of the migration. I am unsure if while executing the thing in Wasm, we are aware of what entry point we are in, but it might be a way.

I would say we call migrate whenever the versions are different. This allows the contract to somehow handle downgrades (e.g. old version is 5, new version is 3). I'd probably just error for those cases in the contract but better than just skipping the migrate call.

+1

[kulikthebird]

What about such a change in the migrate function signature:

#[set_contract_state_version(3)]
pub fn migrate(deps: DepsMut, env: Env, state_version_on_chain: Option<u64>, msg: MigrateMsg) -> StdResult<Response> {
    // [...]
}

So there are two things added:

1. Optional macro call set_contract_state_version,
2. Optional parameter state_version_on_chain: Option<u64>.

I would say we call migrate whenever the versions are different. This allows the contract to somehow handle downgrades (e.g. old version is 5, new version is 3). I'd probably just error for those cases in the contract but better than just skipping the migrate call.

I agree, it makes sense to call migrate each time there's a difference between on-chain version and the newly uploaded one.

Another possibility I think needs to be fixed is adding a function to the Api. It is not idiomatic, as functions should not be stateful, but this is not stateful during execution - for a single pass, it is always constant. We could add the migration_info there. We could panic outside of the migration. I am unsure if while executing the thing in Wasm, we are aware of what entry point we are in, but it might be a way.

I was also thinking about it, but rejected that idea for the same reason - the Api trait is just a set of helpers methods and it would add unnecessary noise to this trait. Also returning an error when called outside of the migration scope makes it useless in other entry points. There's no reason for the contract devs to get the on-chain version in other entry-points, because it'd be always the current version.

[webmaster128]

It would be nice to create a MigrateInfo analogue to MessageInfo in the 3rd argument. Right now you can't see who does the migration as sender is not available. Usually not needed, but there is no good reason to not have it.

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
pub struct MigrateInfo {
    pub sender: Addr,
    // no funds here
    pub old_state_version: Option<u64>
}

// [...]

#[set_contract_state_version(3)]
pub fn migrate(deps: DepsMut, env: Env, info: MigrateInfo, msg: MigrateMsg) -> StdResult<Response> {
    // [...]
}

But this is breaking and not easy to implement

[chipshort]

But this is breaking and not easy to implement

We could implement it non-breaking by checking for the function arity first before calling into the contract, but it would indeed not be easy because all the abstractions we currently have around calling functions assume that we know the number of arguments beforehand.

Also if we do that, we should make MigrateInfo non_exhaustive this time to future-proof.

[webmaster128]

I like that as it might be a bit of work, but we put the burden on us instead of the contract developer. Developers can then swictch from 3 arg to 4 arg whenever they feel like. In the public wasmvm interface we can have two different calls (like Migrate and Migrate4Arg that at some point call into the same entry point.

[webmaster128]

This is pretty cool. We can extend the low level call_function to use different params depending on the arity of the function we load anyways. See #2115

[kulikthebird]

I've update the issue's description with all those suggestions. Two sub-issues are created to resolve this issue.