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

Helpers for state migration #11

Open
hashedone opened this issue Feb 7, 2022 · 2 comments
Open

Helpers for state migration #11

hashedone opened this issue Feb 7, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@hashedone
Copy link
Contributor

hashedone commented Feb 7, 2022

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?

@hashedone hashedone added the enhancement New feature or request label Feb 7, 2022
@ethanfrey
Copy link
Member

ethanfrey commented Feb 7, 2022

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
Copy link
Contributor

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.

@uint uint transferred this issue from CosmWasm/cw-plus Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants