CosmWasm / cw-storage-plus

Storage abstractions for CosmWasm smart contracts
Apache License 2.0
36 stars 26 forks source link

Helpers for state migration #11

Open hashedone opened 2 years ago

hashedone commented 2 years ago

Sometimes upgrading contract, the schema of underlying values might change. In such case, the migration function has to:

  1. Load all old schema values from the storage container of a given name
  2. Perform transformation of old schema objects to new schema object
  3. Store all values to new map, but actually the same map.

Example:

Previously members map stored only members weight, but now it stores also some additional required metadata (which we can give some reasonable defaults or deduce from migration msg).

const MEMBERS: Map<&Addr, Member> = Map::new("members");

fn migrate(deps: DepsMut, env: Env, msg: MigrationMsg) -> Result<Response, ContractError> {
  let old_members: Map<&Addr, u64> = Map::new("members");
  let members = old_members.range(deps.storage, None, None, Order::Ascending)
    .map(|member| weight.map(|(addr, w)| (addr, Member::new(w))))
    .collect::<Result<_, _>>()?;
  for (addr, member) in members {
    MEMBERS.save(deps, &addr, &member)?;
  }
}

This is not very nice pattern, and it could be very much simplified by introducing migrate functions on all containers with signature:

fn migrate<Old, Err, F>(&self, storage: &mut Storage, update: F) -> Result<(), Err>
where
  Err: From<StdError>,
  F: Fn(Old) -> Result<Option<T>, Err>;

Such a function just loads data via old schema, updates it with function, if Err is returned - propagates, if None is returned - removes an item from storage, if T is returned - stores it in map under this key. This is the example for Map, but same could be done for probably all containers (in particular - IndexedMap, Item for sure).

This example assumes, that keys structure never changes, and it probably covers most of our needs, but additionally there can be a function which alters key structure:

fn migrate_with_keys<OldK, Old, Err, F>(&self, storage: &mut Storage, update: F) -> Result<(), Err>
where
  Err: From<StdError>,
  F: Fn(OldK, Old) -> Result<Option<(K, T)>, Err>;

The relevant difference is, that this function takes care about removing the previous entry entirely, and stores the item under new key only (so range access won't fail because of old key schema parsing failure).

Additionally such functions could take additional migrate_from: Option<&str> argument, which loads old values from differently named container - useful if at some point there would be need to rename some container. I am not sure if purging old container would make any sense here, but it may make load-update-store loop easier. Possibly additional function clone_from(namespace: &str) for only renaming may also be useful.

@maurolacy - thoughts?

ethanfrey commented 2 years ago

The migrate function on a Map does sound like a nice idea, and doesn't cover all cases (like splitting some item into two keys), but enough of the simpler cases it does make sense to add. I would focus on covering the 80% here and let people code the flow themselves for the more complex logic rather than add more helpers. (Thus not including migrate_with_keys for now)

For Item, I would make it similar but output Result<T, _> not Result<Option<T>, _>

I wouldn't add it on the IndexedMap items at first until we work out the simpler cases. And SnapshotMap may have it's own difficulties (migrate all historical data as well? lazy transform it?)

But for Map and Item, this would be a nice addition and we can add more after we have feedback from usage

maurolacy commented 2 years ago

I agree with @ethanfrey. This will provide support for simple migrations of Maps, and if there's a more complex case, it can always be done manually.