Skip to content

Commit

Permalink
fix: upgrade handler missing burn permissions for feecollector account (
Browse files Browse the repository at this point in the history
#437)

* passing account keeper to upgrade handlers

* adding the upgrade handler to provide burn permission to feecollector module account

* bumping version to v4.0.1

* Update CHANGELOG.md

* add upgrade test

* Adding interchain test - TestFeeCollectorBurnChainUpgrade

* fixing linting
  • Loading branch information
spoo-bar authored Aug 31, 2023
1 parent 1c06230 commit c989444
Show file tree
Hide file tree
Showing 17 changed files with 272 additions and 55 deletions.
12 changes: 2 additions & 10 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,11 @@ Contains bug fixes.
Contains all the PRs that improved the code without changing the behaviours.
-->

## [Unreleased]

### Added

### Changed

### Deprecated

### Removed
## [v4.0.1]

### Fixed

### Improvements
- [#437](https://github.com/archway-network/archway/pull/437) - adding upgrade handler with missing burn permissions for feecollector account

## [v4.0.0]

Expand Down
7 changes: 3 additions & 4 deletions app/app_upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
upgrade2_0_0 "github.com/archway-network/archway/app/upgrades/2_0_0"
upgrade3_0_0 "github.com/archway-network/archway/app/upgrades/3_0_0"
upgrade4_0_0 "github.com/archway-network/archway/app/upgrades/4_0_0"
upgradelatest "github.com/archway-network/archway/app/upgrades/latest"
upgrade4_0_1 "github.com/archway-network/archway/app/upgrades/4_0_1"
)

// UPGRADES
Expand All @@ -22,8 +22,7 @@ var Upgrades = []upgrades.Upgrade{
upgrade2_0_0.Upgrade, // v2.0.0
upgrade3_0_0.Upgrade, // v3.0.0
upgrade4_0_0.Upgrade, // v4.0.0

upgradelatest.Upgrade, // latest - This upgrade handler is used for all the current changes to the protocol
upgrade4_0_1.Upgrade, // v4.0.1
}

func (app *ArchwayApp) setupUpgrades() {
Expand Down Expand Up @@ -52,7 +51,7 @@ func (app *ArchwayApp) setUpgradeHandlers() {
for _, u := range Upgrades {
app.UpgradeKeeper.SetUpgradeHandler(
u.UpgradeName,
u.CreateUpgradeHandler(app.mm, app.configurator),
u.CreateUpgradeHandler(app.mm, app.configurator, app.AccountKeeper),
)
}
}
3 changes: 2 additions & 1 deletion app/app_upgrades_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
storeTypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -44,7 +45,7 @@ func TestUpgrades(t *testing.T) {
proposalExecuted := false
fauxUpgrade := upgrades.Upgrade{
UpgradeName: "test-upgrade",
CreateUpgradeHandler: func(manager *module.Manager, configurator module.Configurator) upgradetypes.UpgradeHandler {
CreateUpgradeHandler: func(manager *module.Manager, configurator module.Configurator, _ keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
proposalExecuted = true
return manager.RunMigrations(ctx, configurator, fromVM)
Expand Down
2 changes: 1 addition & 1 deletion app/app_upgrades_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ import "github.com/archway-network/archway/app/upgrades"
func (app *ArchwayApp) AddUpgradeHandler(upgrade upgrades.Upgrade) {
app.UpgradeKeeper.SetUpgradeHandler(
upgrade.UpgradeName,
upgrade.CreateUpgradeHandler(app.mm, app.configurator),
upgrade.CreateUpgradeHandler(app.mm, app.configurator, app.AccountKeeper),
)
}
3 changes: 2 additions & 1 deletion app/upgrades/06/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
storetypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
ibcfeetypes "github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types"

Expand All @@ -14,7 +15,7 @@ const Name = "v0.6.0"

var Upgrade = upgrades.Upgrade{
UpgradeName: Name,
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator) upgradetypes.UpgradeHandler {
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator, _ keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
return mm.RunMigrations(ctx, cfg, fromVM)
}
Expand Down
3 changes: 2 additions & 1 deletion app/upgrades/1_0_0_rc_4/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
storetypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

"github.com/archway-network/archway/app/upgrades"
Expand All @@ -13,7 +14,7 @@ const Name = "v1.0.0-rc.4"

var Upgrade = upgrades.Upgrade{
UpgradeName: Name,
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator) upgradetypes.UpgradeHandler {
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator, _ keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
return mm.RunMigrations(ctx, cfg, fromVM)
}
Expand Down
3 changes: 2 additions & 1 deletion app/upgrades/2_0_0/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"

"github.com/cosmos/cosmos-sdk/x/auth/keeper"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
Expand All @@ -23,7 +24,7 @@ const Name = "v2.0.0"

var Upgrade = upgrades.Upgrade{
UpgradeName: Name,
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator) upgradetypes.UpgradeHandler {
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator, _ keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {

// Set Initial Consensus Version
Expand Down
3 changes: 2 additions & 1 deletion app/upgrades/3_0_0/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
storetypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

"github.com/archway-network/archway/app/upgrades"
Expand All @@ -13,7 +14,7 @@ const Name = "v3.0.0"

var Upgrade = upgrades.Upgrade{
UpgradeName: Name,
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator) upgradetypes.UpgradeHandler {
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator, _ keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
return mm.RunMigrations(ctx, cfg, fromVM)
}
Expand Down
3 changes: 2 additions & 1 deletion app/upgrades/4_0_0/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
storetypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

"github.com/archway-network/archway/app/upgrades"
Expand All @@ -13,7 +14,7 @@ const Name = "v4.0.0"

var Upgrade = upgrades.Upgrade{
UpgradeName: Name,
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator) upgradetypes.UpgradeHandler {
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator, _ keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
return mm.RunMigrations(ctx, cfg, fromVM)
}
Expand Down
40 changes: 40 additions & 0 deletions app/upgrades/4_0_1/upgrades.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package upgrade4_0_1

import (
"fmt"

storetypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

"github.com/archway-network/archway/app/upgrades"
)

const Name = "v4.0.1"

var Upgrade = upgrades.Upgrade{
UpgradeName: Name,
CreateUpgradeHandler: func(mm *module.Manager, cfg module.Configurator, accountKeeper keeper.AccountKeeper) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
fcAccount := accountKeeper.GetModuleAccount(ctx, authtypes.FeeCollectorName)
account, ok := fcAccount.(*authtypes.ModuleAccount)
if !ok {
return nil, fmt.Errorf("feeCollector account is not *authtypes.ModuleAccount")
}
if !account.HasPermission(authtypes.Burner) {
account.Permissions = append(account.Permissions, authtypes.Burner)
}
err := accountKeeper.ValidatePermissions(account)
if err != nil {
return nil, fmt.Errorf("Could not validate feeCollectors permissions")
}
accountKeeper.SetModuleAccount(ctx, account)

return mm.RunMigrations(ctx, cfg, fromVM)
}
},
StoreUpgrades: storetypes.StoreUpgrades{},
}
95 changes: 95 additions & 0 deletions app/upgrades/4_0_1/upgrades_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package upgrade4_0_1_test

import (
"fmt"
"testing"

authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
"github.com/stretchr/testify/suite"
abci "github.com/tendermint/tendermint/abci/types"

e2eTesting "github.com/archway-network/archway/e2e/testing"
)

type UpgradeTestSuite struct {
suite.Suite

archway *e2eTesting.TestChain
}

func (s *UpgradeTestSuite) SetupTest() {
s.archway = e2eTesting.NewTestChain(s.T(), 1)
}

func TestUpgradeTestSuite(t *testing.T) {
suite.Run(t, new(UpgradeTestSuite))
}

const (
dummyUpgradeHeight = 5
)

func (suite *UpgradeTestSuite) TestUpgrade() {
testCases := []struct {
name string
pre_upgrade func()
post_upgrade func()
}{
{
"Feecollector does not have burn permissions, we ensure upgrade happens and account gets the burn permissions",
func() {
accountKeeper := suite.archway.GetApp().AccountKeeper
fcAccount := accountKeeper.GetModuleAccount(suite.archway.GetContext(), authtypes.FeeCollectorName)

account, ok := fcAccount.(*authtypes.ModuleAccount)
suite.Require().True(ok)
account.Permissions = []string{}
accountKeeper.SetModuleAccount(suite.archway.GetContext(), account)

fcAccount = accountKeeper.GetModuleAccount(suite.archway.GetContext(), authtypes.FeeCollectorName)
suite.Require().False(fcAccount.HasPermission(authtypes.Burner))
},
func() {
accountKeeper := suite.archway.GetApp().AccountKeeper
fcAccount := accountKeeper.GetModuleAccount(suite.archway.GetContext(), authtypes.FeeCollectorName)
suite.Require().True(fcAccount.HasPermission(authtypes.Burner))
},
},
{
"Feecollector already has burn permissions, we ensure upgrade happens smoothly",
func() {
accountKeeper := suite.archway.GetApp().AccountKeeper
fcAccount := accountKeeper.GetModuleAccount(suite.archway.GetContext(), authtypes.FeeCollectorName)
suite.Require().True(fcAccount.HasPermission(authtypes.Burner))
},
func() {
accountKeeper := suite.archway.GetApp().AccountKeeper
fcAccount := accountKeeper.GetModuleAccount(suite.archway.GetContext(), authtypes.FeeCollectorName)
suite.Require().True(fcAccount.HasPermission(authtypes.Burner))
},
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
suite.SetupTest() // reset

tc.pre_upgrade()

ctx := suite.archway.GetContext().WithBlockHeight(dummyUpgradeHeight - 1)
plan := upgradetypes.Plan{Name: "v4.0.1", Height: dummyUpgradeHeight}
upgradekeeper := suite.archway.GetApp().UpgradeKeeper
err := upgradekeeper.ScheduleUpgrade(ctx, plan)
suite.Require().NoError(err)
_, exists := upgradekeeper.GetUpgradePlan(ctx)
suite.Require().True(exists)
ctx = ctx.WithBlockHeight(dummyUpgradeHeight)
suite.Require().NotPanics(func() {
suite.archway.GetApp().BeginBlocker(ctx, abci.RequestBeginBlock{})
})

tc.post_upgrade()
})
}
}
24 changes: 0 additions & 24 deletions app/upgrades/latest/upgrades.go

This file was deleted.

3 changes: 2 additions & 1 deletion app/upgrades/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package upgrades
import (
"github.com/cosmos/cosmos-sdk/store/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

Expand All @@ -15,7 +16,7 @@ type Upgrade struct {
UpgradeName string

// CreateUpgradeHandler defines the function that creates an upgrade handler
CreateUpgradeHandler func(*module.Manager, module.Configurator) upgradetypes.UpgradeHandler
CreateUpgradeHandler func(*module.Manager, module.Configurator, keeper.AccountKeeper) upgradetypes.UpgradeHandler

// Store upgrades, should be used for any new modules introduced, new modules deleted, or store names renamed.
StoreUpgrades types.StoreUpgrades
Expand Down
12 changes: 6 additions & 6 deletions interchaintest/chain_upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ func TestChainUpgrade(t *testing.T) {
t.Skip("skipping in short mode")
}

archwayChain, client, ctx := startChain(t)
archwayChain, client, ctx := startChain(t, initialVersion)
chainUser := fundChainUser(t, ctx, archwayChain)
haltHeight := submitUpgradeProposalAndVote(t, ctx, archwayChain, chainUser)
haltHeight := submitUpgradeProposalAndVote(t, ctx, upgradeName, archwayChain, chainUser)

height, err := archwayChain.Height(ctx)
require.NoError(t, err, "cound not fetch height before upgrade")
Expand Down Expand Up @@ -63,7 +63,7 @@ func TestChainUpgrade(t *testing.T) {
require.NoError(t, err, "chain did not produce blocks after upgrade")
}

func submitUpgradeProposalAndVote(t *testing.T, ctx context.Context, archwayChain *cosmos.CosmosChain, chainUser ibc.Wallet) uint64 {
func submitUpgradeProposalAndVote(t *testing.T, ctx context.Context, nextUpgradeName string, archwayChain *cosmos.CosmosChain, chainUser ibc.Wallet) uint64 {
height, err := archwayChain.Height(ctx) // The current chain height
require.NoError(t, err, "error fetching height before submit upgrade proposal")

Expand All @@ -72,7 +72,7 @@ func submitUpgradeProposalAndVote(t *testing.T, ctx context.Context, archwayChai
proposal := cosmos.SoftwareUpgradeProposal{
Deposit: "10000000000" + archwayChain.Config().Denom,
Title: "Test upgrade",
Name: upgradeName,
Name: nextUpgradeName,
Description: "Every PR we perform a upgrade check to ensure nothing breaks",
Height: haltHeight,
}
Expand All @@ -94,13 +94,13 @@ func fundChainUser(t *testing.T, ctx context.Context, archwayChain *cosmos.Cosmo
return users[0]
}

func startChain(t *testing.T) (*cosmos.CosmosChain, *client.Client, context.Context) {
func startChain(t *testing.T, startingVersion string) (*cosmos.CosmosChain, *client.Client, context.Context) {
numOfVals := 1
cf := interchaintest.NewBuiltinChainFactory(zaptest.NewLogger(t), []*interchaintest.ChainSpec{
{
Name: chainName,
ChainName: "archway-1",
Version: initialVersion,
Version: startingVersion,
ChainConfig: archwayConfig,
NumValidators: &numOfVals,
},
Expand Down
Loading

0 comments on commit c989444

Please sign in to comment.