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

Collin/v8 migration fixes #324

Merged
merged 35 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
9cf139d
Add missing SDK 0.47 upgrade steps
cbrit Oct 29, 2024
442e5b8
Remove msg event boilerplate replace in 0.47
cbrit Oct 29, 2024
f7b844d
Add missing param migrations and additional IBC client type
cbrit Oct 29, 2024
0fbf617
Fixing cellarfees params migration
cbrit Oct 29, 2024
f72ba15
More cellarfees params fixing
cbrit Oct 29, 2024
b8f1b66
Even more cellarfees params fixing
cbrit Oct 29, 2024
84fcf65
Hopefully last cellarfees params fix
cbrit Oct 29, 2024
03e7b7b
Hopefully the last last cellarfees fix
cbrit Oct 29, 2024
dc1e953
Remove write to old cellarfees param subspace
cbrit Oct 29, 2024
d8eb12c
Use correct method to retrieve legacy cellarfees subspace
cbrit Oct 29, 2024
fa58afd
Add missing IBC migration stuff
cbrit Oct 29, 2024
216b255
Test commit for debugging
cbrit Oct 29, 2024
c9d50c5
More debugging
cbrit Oct 29, 2024
c16ff12
Please
cbrit Oct 29, 2024
050b56b
Debugging
cbrit Oct 29, 2024
cdc8446
AHHHH
cbrit Oct 29, 2024
ee76e49
just trying stuff
cbrit Oct 29, 2024
533371b
.
cbrit Oct 29, 2024
033916f
..
cbrit Oct 29, 2024
3178f24
Trying with legacySubspace passed in via app module, and deleting par…
cbrit Oct 30, 2024
a272498
Fix method call
cbrit Oct 30, 2024
509babb
Fix method call
cbrit Oct 30, 2024
ed9ecc1
Move upgrade handler setup after store mount
cbrit Oct 30, 2024
68554f8
Hardcode cellarfees params
cbrit Oct 30, 2024
bc18b56
Incentives params test
cbrit Oct 30, 2024
5da80e6
Cellarfees params migration fix test
cbrit Oct 30, 2024
d8a3941
Clean up auction params migration
cbrit Oct 30, 2024
7fa3a1f
Explicitly set new auction module account permissions
cbrit Oct 31, 2024
0d64fc2
Fix axelarcork export genesis bug that results in null values for Cor…
cbrit Oct 31, 2024
13f302c
Actually add the upgrade fix...
cbrit Oct 31, 2024
6105c40
Move auction permission to end of upgrade handler
cbrit Oct 31, 2024
a680ea4
Wow.
cbrit Oct 31, 2024
a25cea5
Tweak auction test
cbrit Nov 4, 2024
36113b5
Merge branch 'main' into collin/v8-migration-fixes
cbrit Nov 7, 2024
e48b362
Fix params validation in cellarfees
cbrit Nov 7, 2024
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
42 changes: 40 additions & 2 deletions app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import (
govclient "github.com/cosmos/cosmos-sdk/x/gov/client"
govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
govtypesv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
govtypesv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
"github.com/cosmos/cosmos-sdk/x/mint"
mintkeeper "github.com/cosmos/cosmos-sdk/x/mint/keeper"
Expand Down Expand Up @@ -609,7 +610,7 @@ func NewSommelierApp(
authzmodule.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
cork.NewAppModule(app.CorkKeeper, appCodec),
incentives.NewAppModule(app.IncentivesKeeper, app.DistrKeeper, app.BankKeeper, app.MintKeeper, appCodec),
cellarfees.NewAppModule(app.CellarFeesKeeper, appCodec, app.AccountKeeper, app.BankKeeper, app.MintKeeper, app.CorkKeeper, app.AuctionKeeper),
cellarfees.NewAppModule(app.CellarFeesKeeper, appCodec, app.AccountKeeper, app.BankKeeper, app.MintKeeper, app.CorkKeeper, app.AuctionKeeper, app.GetSubspace(cellarfeestypes.ModuleName)),
auction.NewAppModule(app.AuctionKeeper, app.BankKeeper, app.AccountKeeper, appCodec),
pubsub.NewAppModule(appCodec, app.PubsubKeeper, app.StakingKeeper, app.GravityKeeper),
addresses.NewAppModule(appCodec, app.AddressesKeeper),
Expand Down Expand Up @@ -750,7 +751,7 @@ func NewSommelierApp(
authzmodule.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
cork.NewAppModule(app.CorkKeeper, appCodec),
incentives.NewAppModule(app.IncentivesKeeper, app.DistrKeeper, app.BankKeeper, app.MintKeeper, appCodec),
cellarfees.NewAppModule(app.CellarFeesKeeper, appCodec, app.AccountKeeper, app.BankKeeper, app.MintKeeper, app.CorkKeeper, app.AuctionKeeper),
cellarfees.NewAppModule(app.CellarFeesKeeper, appCodec, app.AccountKeeper, app.BankKeeper, app.MintKeeper, app.CorkKeeper, app.AuctionKeeper, app.GetSubspace(cellarfeestypes.ModuleName)),
auction.NewAppModule(app.AuctionKeeper, app.BankKeeper, app.AccountKeeper, appCodec),
pubsub.NewAppModule(appCodec, app.PubsubKeeper, app.StakingKeeper, app.GravityKeeper),
addresses.NewAppModule(appCodec, app.AddressesKeeper),
Expand Down Expand Up @@ -1038,6 +1039,39 @@ func (app *SommelierApp) setupUpgradeStoreLoaders() {
}

func (app *SommelierApp) setupUpgradeHandlers() {
// Set param key table for params module migration
for _, subspace := range app.ParamsKeeper.GetSubspaces() {
subspace := subspace
found := true
var keyTable paramstypes.KeyTable
switch subspace.Name() {
case authtypes.ModuleName:
keyTable = authtypes.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case banktypes.ModuleName:
keyTable = banktypes.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case stakingtypes.ModuleName:
keyTable = stakingtypes.ParamKeyTable()
case minttypes.ModuleName:
keyTable = minttypes.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case distrtypes.ModuleName:
keyTable = distrtypes.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case slashingtypes.ModuleName:
keyTable = slashingtypes.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case govtypes.ModuleName:
keyTable = govtypesv1.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case crisistypes.ModuleName:
keyTable = crisistypes.ParamKeyTable() //nolint: staticcheck // deprecated but required for upgrade
case ibctransfertypes.ModuleName:
keyTable = ibctransfertypes.ParamKeyTable()
default:
// subspace not handled
found = false
}
if found && !subspace.HasKeyTable() {
subspace.WithKeyTable(keyTable)
}
}

baseAppLegacySS := app.ParamsKeeper.Subspace(baseapp.Paramspace).WithKeyTable(paramstypes.ConsensusParamsKeyTable())

// TODO: Add v8 upgrade handle
Expand All @@ -1048,6 +1082,10 @@ func (app *SommelierApp) setupUpgradeHandlers() {
app.configurator,
&baseAppLegacySS,
&app.ConsensusParamsKeeper,
app.IBCKeeper,
app.appCodec,
app.IBCKeeper.ClientKeeper,
&app.AccountKeeper,
),
)
}
59 changes: 58 additions & 1 deletion app/upgrades/v8/upgrades.go
Original file line number Diff line number Diff line change
@@ -1,26 +1,83 @@
package v8

import (
"fmt"

"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
consensusparamkeeper "github.com/cosmos/cosmos-sdk/x/consensus/keeper"
paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
ibckeeper "github.com/cosmos/ibc-go/v7/modules/core/keeper"
ibctmmigrations "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint/migrations"
auctiontypes "github.com/peggyjv/sommelier/v8/x/auction/types"
)

func CreateUpgradeHandler(
mm *module.Manager,
configurator module.Configurator,
baseAppLegacySS *paramstypes.Subspace,
consensusParamsKeeper *consensusparamkeeper.Keeper,
ibcKeeper *ibckeeper.Keeper,
cdc codec.BinaryCodec,
clientKeeper ibctmmigrations.ClientKeeper,
accountKeeper *authkeeper.AccountKeeper,
) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
ctx.Logger().Info("v8 upgrade: entering handler and running migrations")

// Include this when migrating to ibc-go v7 (optional)
// source: https://github.com/cosmos/ibc-go/blob/v7.2.0/docs/migrations/v6-to-v7.md
// prune expired tendermint consensus states to save storage space
if _, err := ibctmmigrations.PruneExpiredConsensusStates(ctx, cdc, clientKeeper); err != nil {
return nil, err
}

// new x/consensus module params migration
baseapp.MigrateParams(ctx, baseAppLegacySS, consensusParamsKeeper)

return mm.RunMigrations(ctx, configurator, vm)
// explicitly update the IBC 02-client params, adding the localhost client type
params := ibcKeeper.ClientKeeper.GetParams(ctx)
params.AllowedClients = append(params.AllowedClients, ibcexported.Localhost)
ibcKeeper.ClientKeeper.SetParams(ctx, params)

vm, err := mm.RunMigrations(ctx, configurator, vm)
if err != nil {
return nil, err
}

// add burner permission to auction account
if err := MigrateAuctionAccountPermissions(ctx, accountKeeper); err != nil {
return nil, err
}

return vm, nil
}
}

func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error {
ctx.Logger().Info("Migrating auction account permissions")
oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName)

if oldAcctI == nil {
return fmt.Errorf("module account not found")
}

newAcct := authtypes.NewEmptyModuleAccount(auctiontypes.ModuleName, authtypes.Burner)
newAcct.AccountNumber = oldAcctI.GetAccountNumber()
newAcct.Address = oldAcctI.GetAddress().String()
newAcct.Sequence = oldAcctI.GetSequence()
newAcct.Name = oldAcctI.GetName()
newAcctI := (accountKeeper.NewAccount(ctx, newAcct)).(authtypes.ModuleAccountI)

accountKeeper.SetModuleAccount(ctx, newAcctI)

ctx.Logger().Info("Auction account permissions migrated")

return nil
}
Comment on lines +63 to +83
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify auction account permissions migration by updating existing account

Instead of creating a new module account and manually copying fields from the old account, you can simplify the migration by directly modifying the permissions of the existing account. This approach reduces complexity and minimizes the risk of introducing errors.

Here's a suggested change:

 func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error {
 	ctx.Logger().Info("Migrating auction account permissions")
 	oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName)

 	if oldAcctI == nil {
 		return fmt.Errorf("module account not found")
 	}

+	// Add the Burner permission to the existing account
+	oldPermissions := oldAcctI.GetPermissions()
+	newPermissions := append(oldPermissions, authtypes.Burner)
+	oldAcctI.SetPermissions(newPermissions)
+
+	// Update the module account with the new permissions
+	accountKeeper.SetModuleAccount(ctx, oldAcctI)

-	newAcct := authtypes.NewEmptyModuleAccount(auctiontypes.ModuleName, authtypes.Burner)
-	newAcct.AccountNumber = oldAcctI.GetAccountNumber()
-	newAcct.Address = oldAcctI.GetAddress().String()
-	newAcct.Sequence = oldAcctI.GetSequence()
-	newAcct.Name = oldAcctI.GetName()
-	newAcctI := (accountKeeper.NewAccount(ctx, newAcct)).(authtypes.ModuleAccountI)
-
-	accountKeeper.SetModuleAccount(ctx, newAcctI)

 	ctx.Logger().Info("Auction account permissions migrated")

 	return nil
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error {
ctx.Logger().Info("Migrating auction account permissions")
oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName)
if oldAcctI == nil {
return fmt.Errorf("module account not found")
}
newAcct := authtypes.NewEmptyModuleAccount(auctiontypes.ModuleName, authtypes.Burner)
newAcct.AccountNumber = oldAcctI.GetAccountNumber()
newAcct.Address = oldAcctI.GetAddress().String()
newAcct.Sequence = oldAcctI.GetSequence()
newAcct.Name = oldAcctI.GetName()
newAcctI := (accountKeeper.NewAccount(ctx, newAcct)).(authtypes.ModuleAccountI)
accountKeeper.SetModuleAccount(ctx, newAcctI)
ctx.Logger().Info("Auction account permissions migrated")
return nil
}
func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error {
ctx.Logger().Info("Migrating auction account permissions")
oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName)
if oldAcctI == nil {
return fmt.Errorf("module account not found")
}
// Add the Burner permission to the existing account
oldPermissions := oldAcctI.GetPermissions()
newPermissions := append(oldPermissions, authtypes.Burner)
oldAcctI.SetPermissions(newPermissions)
// Update the module account with the new permissions
accountKeeper.SetModuleAccount(ctx, oldAcctI)
ctx.Logger().Info("Auction account permissions migrated")
return nil
}

5 changes: 5 additions & 0 deletions integration_tests/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ func (s *IntegrationTestSuite) TestAuction() {

// Calculate the expected burn amount (50% of total SOMM paid in auction)
expectedBurnAmount := totalSommPaid.Quo(sdk.NewInt(2))
s.Require().NotZero(expectedBurnAmount.Int64(), "Expected burn amount should not be zero")

// Calculate the expected new total supply
expectedNewSupply := supplyRes.Amount.Amount.Sub(expectedBurnAmount)
Expand All @@ -326,6 +327,10 @@ func (s *IntegrationTestSuite) TestAuction() {
// Verify that the new supply matches the expected new supply
s.Require().Equal(expectedNewSupply.Int64(), newSupplyRes.Amount.Amount.Int64(), "Total supply of usomm should be reduced by the amount burnt")

// Get auction module account
auctionModuleAccount := authtypes.NewModuleAddress(types.ModuleName)
s.T().Logf("Auction module account: %s", auctionModuleAccount.String())

s.T().Log("Total supply of usomm has been correctly reduced!")

s.T().Log("--Test completed successfully--")
Expand Down
12 changes: 10 additions & 2 deletions x/auction/keeper/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package keeper

import (
sdk "github.com/cosmos/cosmos-sdk/types"
v1 "github.com/peggyjv/sommelier/v8/x/auction/migrations/v1"
"github.com/peggyjv/sommelier/v8/x/auction/types"
)

// Migrator is a struct for handling in-place store migrations.
Expand All @@ -17,5 +17,13 @@ func NewMigrator(keeper Keeper) Migrator {

// Migrate1to2 migrates from consensus version 1 to 2.
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
return v1.MigrateParamStore(ctx, m.keeper.paramSpace)
ctx.Logger().Info("auction v1 to v2: New params")
subspace := m.keeper.paramSpace

if !subspace.Has(ctx, types.KeyAuctionBurnRate) {
subspace.Set(ctx, types.KeyAuctionBurnRate, types.DefaultParams().AuctionBurnRate)
}

ctx.Logger().Info("auction v1 to v2: Params migration complete")
return nil
}
18 changes: 0 additions & 18 deletions x/auction/migrations/v1/store.go

This file was deleted.

Loading
Loading