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.

Sign up for GitHub

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

Add in-place store migrations #8485

Merged
merged 33 commits into from
Feb 10, 2021
Merged

Add in-place store migrations #8485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
10d72d7
Add 1st version of migrate
amaury1093 Feb 1, 2021
8036fca
Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-8…
amaury1093 Feb 2, 2021
a9a406e
Put migration logic into Configurator
amaury1093 Feb 2, 2021
9ed3987
add test to bank store migration
amaury1093 Feb 2, 2021
a095d3a
add test for configurator
amaury1093 Feb 2, 2021
d550bff
Merge branch 'master' into am-8345-migration
amaury1093 Feb 2, 2021
b7141f9
Error if no migration found
amaury1093 Feb 3, 2021
2d554ce
Remove RunMigrations from Configurator interface
amaury1093 Feb 3, 2021
3df3f44
Update spec
amaury1093 Feb 3, 2021
ba6bd44
Rename folders
amaury1093 Feb 3, 2021
197b7ce
Merge branch 'master' into am-8345-migration
amaury1093 Feb 3, 2021
424932c
copy-paste from keys.go
amaury1093 Feb 3, 2021
254a71f
Merge branch 'am-8345-migration' of ssh://github.com/cosmos/cosmos-sd…
amaury1093 Feb 3, 2021
b6c0714
Fix nil map
amaury1093 Feb 3, 2021
9ab7f13
rename function
amaury1093 Feb 3, 2021
fc076f5
Update simapp/app.go
amaury1093 Feb 5, 2021
ae7b7dc
Update simapp/app_test.go
amaury1093 Feb 5, 2021
2c26734
Adderss reviews
amaury1093 Feb 8, 2021
eb15047
Merge branch 'am-8345-migration' of ssh://github.com/cosmos/cosmos-sd…
amaury1093 Feb 8, 2021
291a9e6
Fix tests
amaury1093 Feb 8, 2021
41d29ed
Update testutil/context.go
amaury1093 Feb 8, 2021
33494dc
Update docs for ConsensusVersion
amaury1093 Feb 8, 2021
e3a2412
Rename to forVersion
amaury1093 Feb 8, 2021
6db31d4
Merge branch 'am-8345-migration' of ssh://github.com/cosmos/cosmos-sd…
amaury1093 Feb 8, 2021
55f547e
Fix tests
amaury1093 Feb 8, 2021
8f3b9d4
Check error early
amaury1093 Feb 8, 2021
29b85b7
Return 1 for intiial version
amaury1093 Feb 8, 2021
41e0339
Use MigrationKeeper
amaury1093 Feb 10, 2021
d922003
Fix test
amaury1093 Feb 10, 2021
f422b93
Revert adding marshaler to Configurator
amaury1093 Feb 10, 2021
8cf88fe
Godoc updates
amaury1093 Feb 10, 2021
5add13a
Update docs
amaury1093 Feb 10, 2021
2cef291
Merge branch 'master' into am-8345-migration
aaronc Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Rename folders
  • Loading branch information
@amaury1093
amaury1093 committed Feb 3, 2021
commit ba6bd446b8da68cae18d324c07a19f666ac229bd
0 x/bank/legacy/v040/store.go → x/bank/legacy/v040/keys.go
View file
Open in desktop
File renamed without changes.
28 changes: 28 additions & 0 deletions x/bank/legacy/v042/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package v042

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
)

// KVStore keys
var (
// BalancesPrefix is the for the account balances store. We use a byte
// (instead of say `[]]byte("balances")` to save some disk space).
BalancesPrefix = []byte{0x02}
)

// AddressFromBalancesStore returns an account address from a balances prefix
// store. The key must not contain the perfix BalancesPrefix as the prefix store
// iterator discards the actual prefix.
func AddressFromBalancesStore(key []byte) sdk.AccAddress {
addrLen := key[0]
addr := key[1 : addrLen+1]

return sdk.AccAddress(addr)
}

// CreateAccountBalancesPrefix creates the prefix for an account's balances.
func CreateAccountBalancesPrefix(addr []byte) []byte {
return append(BalancesPrefix, address.MustLengthPrefix(addr)...)
}
27 changes: 2 additions & 25 deletions x/bank/legacy/v042/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,39 +3,16 @@ package v042
import (
"github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
v040auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v040"
v040bank "github.com/cosmos/cosmos-sdk/x/bank/legacy/v040"
)

// KVStore keys
var (
// BalancesPrefix is the for the account balances store. We use a byte
// (instead of say `[]]byte("balances")` to save some disk space).
BalancesPrefix = []byte{0x02}
)

// AddressFromBalancesStore returns an account address from a balances prefix
// store. The key must not contain the perfix BalancesPrefix as the prefix store
// iterator discards the actual prefix.
func AddressFromBalancesStore(key []byte) sdk.AccAddress {
addrLen := key[0]
addr := key[1 : addrLen+1]

return sdk.AccAddress(addr)
}

// CreateAccountBalancesPrefix creates the prefix for an account's balances.
func CreateAccountBalancesPrefix(addr []byte) []byte {
return append(BalancesPrefix, address.MustLengthPrefix(addr)...)
}

// StoreMigration performs in-place store migrations from v0.40 to v0.42. The
// MigrateStore performs in-place store migrations from v0.40 to v0.42. The
// migration includes:
//
// - Change addresses to be length-prefixed.
// - Change balances prefix to 1 byte
func StoreMigration(store sdk.KVStore) error {
func MigrateStore(store sdk.KVStore) error {
// old key is of format:
// prefix ("balances") || addrBytes (20 bytes) || denomBytes
// new key is of format
Expand Down
2 changes: 1 addition & 1 deletion x/bank/legacy/v042/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func TestStoreMigration(t *testing.T) {
oldKey := append(append(v040bank.BalancesPrefix, addr...), denom...)
store.Set(oldKey, value)

err := v042bank.StoreMigration(store)
err := v042bank.MigrateStore(store)
require.NoError(t, err)

newKey := append(v042bank.CreateAccountBalancesPrefix(addr), denom...)
Expand Down
2 changes: 1 addition & 1 deletion x/bank/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ type AppModule struct {
func (am AppModule) RegisterServices(cfg module.Configurator) {
types.RegisterMsgServer(cfg.MsgServer(), keeper.NewMsgServerImpl(am.keeper))
types.RegisterQueryServer(cfg.QueryServer(), am.keeper)
cfg.RegisterMigration(types.ModuleName, 0, v042.StoreMigration)
cfg.RegisterMigration(types.ModuleName, 0, v042.MigrateStore)
Copy link
Member

Choose a reason for hiding this comment

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

The store key and codec are in the keeper already so they can be passed from the keeper to the migration handler closure not from the module manager into the handler.

Copy link
Member

Choose a reason for hiding this comment

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

And even if the keeper didn't have this stuff already we can add it to Configurator so that all services have it rather than separately to each migration handler. Does that make sense? I think it will be much cleaner this way. Otherwise every time we need some new bit of configuration stuff then we keep adding more params to migration handler. Instead we can keep the migration handler params minimal and pass around other static configuration higher up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The store key and codec are in the keeper already so they can be passed from the keeper to the migration handler closure not from the module manager into the handler.

Re store key: it is in the keeper yes, but not accessible by the AppModule at this line (cannot do am.keeper.storeKey).

Happy to discuss it today (or feel free to push to this PR if it's faster).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so I think the right way is to make the migration a keeper method.

Basically:

cfg.RegisterMigration(types.ModuleName, 0, func(ctx sdk.Context) error {
  return am.keeper.Migrate0(ctx)
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok, yeah that works, but it also pollutes the keeper. In bank, the Keeper is an interface, so we'd need to put it as a public method there, and it'd also be exposed to other modules.

I also like the fact that migration scripts are in each module's legacy folder.

I prefer the current approach, but if you think the keeper's way is cleaner, I'm okay to change it. (cc @robert-zaremba, maybe you have an opinion, since you reviewed this PR?)

Copy link
Member

@aaronc aaronc Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In ADR 033, we include both StoreKey and Marshaler on Configurator and we could explore that step. An alternative to polluting the keeper is just to put the storeKey and codec in AppModule when it gets created. Or we can just make a MigrationKeeper interface and cast Keeper to that and we still keep the scripts in legacy but import them from the keeper... But I really am pretty strongly opposed to making these parameters on the migration handler, it's just not the right place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can just make a MigrationKeeper interface and cast Keeper to that and we still keep the scripts in legacy but import them from the keeper

I went with this in 41e0339. I feel strongly about having those scripts in legacy, because I am copy/pasting legacy code into those folders.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree they should be in legacy.

What do you think about having StoreKey and Marshaler on Configurator?

One big thing I'm trying to pay attention to with APIs is not trying to pass too many things with function params. What ends up happening is that people say, oh we need just one more parameter and then the function signature changes. It's much less breaking to add fields to structs and methods to interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

This is worth keeping in mind: https://blog.golang.org/module-compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about having StoreKey and Marshaler on Configurator?

Having Marshaler on Configurator is useless imo (for migrations), because most (all?) AppModuleBasics already have a cdc.

Would StoreKey on Configurator look like func StoreKey(moduleName string) sdk.StoreKey? How do you avoid moduleA doing in its AppModule: b:=configurator.StoreKey("moduleB")?

}

// NewAppModule creates a new AppModule object
Expand Down