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

Clone InterfaceRegistry for migrations #9004

Open
4 tasks
amaury1093 opened this issue Mar 26, 2021 · 1 comment
Open
4 tasks

Clone InterfaceRegistry for migrations #9004

amaury1093 opened this issue Mar 26, 2021 · 1 comment

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Mar 26, 2021

Summary

Migrations need to register legacy interfaces. To not pollute the real interfaceRegistry, we should clone a separate interfaceRegistry for migrations only.

Problem Definition

Here's an example:

  • In v042, the supply was stored in state as an interface SupplyI:
    bz, err := k.MarshalSupply(supply)

    To do so, we needed to register SupplyI in the interfaceRegistry
  • In v043, we now store the supply indexed by denom (Index supply by denom #7092). We removed the SupplyI interface, and also removed it from the interfaceRegistry.

However, to perform in-place store migrations from 0.42 to 0.43, we still need to register SupplyI inside an interfaceRegistry, just to be able to perform the migration.

Proposal

Three proposals:

1. Keep one unique interfaceRegistry like now, and register legacy interfaces on it

This is the approach taken in #8998. In the (AppModule) RegisterInterfaces, we also register legacy interfaces.

+ simple
- polluting the current interfaceRegistry with legacy interfaces

2. Clone the interfaceRegistry inside each keeper

Each keeper has a Migrator struct, which could receive a reference to the interfaceRegistry, clone it, and register legacy interfaces on that clones interfaceRegistry.

+ simple (maybe slightly less than 1/, but still okay)
- we are cloning N interfaceRegistries for migrations
- docs for module developers

3. Clone the interfaceRegistry once, and use this same instance for all migrations

An idea could be for the configurator to hold a migrationsInterfaceRegistry, which is a clone of the real interfaceRegistry. Also, when each module calls RegisterMigration, we can allow it to register legacy interfaces:

- RegisterMigration(moduleName string, forVersion uint64, handler MigrationHandler) error
+ RegisterMigration(moduleName string, forVersion uint64, handler MigrationHandler, func (registry types.InterfaceRegistry)) error

// and then, inside modules:

func (am AppModule) RegisterServices(cfg module.Configurator) {
	// -- snip --
-	cfg.RegisterMigration("bank", 1, m.Migrate1to2)
+	cfg.RegisterMigration("bank", 1, m.Migrate1to2, v040bank.RegisterInterfaces)
}

+ no cloning of N interfaceRegistries, no polluting the real interfaceRegistry
- makes code a bit harder to understand
- refactoring (possibly with breaking changes)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093
Copy link
Contributor Author

amaury1093 commented Mar 26, 2021

While 3/ seems like the correct way, i actually prefer 2/ as its a good balance between code cleanliness and simplicity/understandability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants