feat!: remove token voting upgrades #1629

evan-forbes · 2023-04-14T02:03:25Z

Overview

wraps the sdk's standard upgrade module to remove the ability to submit upgrade proposals. The idea being that this maintains compatibility with the IBC module (which requires access to the upgrade module's keeper), while still removing the ability for the upgrade module to work by not registering the upgrade module's msg server or begin block. Also, when we implement #1014 we can progressively flesh this module out with our own logic.

closes #1571

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

evan-forbes · 2023-04-17T23:20:42Z

x/upgrade/module.go

+// RegisterInterfaces overwrites the default upgrade module's method with a
+// noop.
+func (AppModule) RegisterInterfaces(registry codectypes.InterfaceRegistry) {
+}
+
+// RegisterServices overwrites the default upgrade module's method with a
+// noop.
+func (am AppModule) RegisterServices(cfg module.Configurator) {
+}
+
+// ConsensusVersion implements AppModule/ConsensusVersion.
+func (AppModule) ConsensusVersion() uint64 { return consensusVersion }
+
+// BeginBlock overwrites the default upgrade module's BeginBlock method with a
+// noop.
+func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
+}


Here's the most important state machine changes. By wrapping the upgrade module with our own and then overwriting these methods, we're removing any of the external functionality that's in the msg server/begin block, while preserving keeper access to the upgrade module to maintain 100% functionality with the IBC module.

Don't we need to also overwrite endBlock?

I thought the same, but I don't think the upgrade module has an endBlock (its abci file)

my bad, confused it with gov module :D

Could we just keep the keeper and not the module?

Could we just keep the keeper and not the module?

this is a good idea! I think this would be the simplest approach, but I think we still need some way to register some of the upgrade types.

for example when the governance can upgrade an IBC clients

// NewSoftwareUpgradeProposalHandler creates a governance handler to manage new proposal types. // It enables SoftwareUpgradeProposal to propose an Upgrade, and CancelSoftwareUpgradeProposal // to abort a previously voted upgrade. // //nolint:staticcheck // we are intentionally using a deprecated proposal here. func NewSoftwareUpgradeProposalHandler(k *keeper.Keeper) govtypes.Handler { return func(ctx sdk.Context, content govtypes.Content) error { switch c := content.(type) { case *types.SoftwareUpgradeProposal: <-- return handleSoftwareUpgradeProposal(ctx, k, c) case *types.CancelSoftwareUpgradeProposal: <-- return handleCancelSoftwareUpgradeProposal(ctx, k, c) default: return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized software upgrade proposal content type: %T", c) } } }

evan-forbes · 2023-04-17T23:23:22Z

x/upgrade/test/integration_test.go

+func TestUpgrade(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping SDK integration test in short mode.")
+	}
+	suite.Run(t, new(UpgradeTestSuite))
+}


This integration test suite is probably overkill, but at least we have a high degree of confidence that submitting governance proposals to upgrade won't work.

there might be another test we could add where we manually force an upgrade into the state and check that it doesn't get picked up by begin block, but I opted to avoid that for now. If we decide that its worth it, I'm happy to add it!

codecov-commenter · 2023-04-17T23:25:06Z

Codecov Report

Merging #1629 (36ed8b8) into main (8c1fcb5) will decrease coverage by 0.03%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##             main    #1629      +/-   ##
==========================================
- Coverage   50.96%   50.93%   -0.03%     
==========================================
  Files          92       93       +1     
  Lines        5751     5760       +9     
==========================================
+ Hits         2931     2934       +3     
- Misses       2520     2526       +6     
  Partials      300      300

Impacted Files	Coverage Δ
testutil/testnode/query.go	`0.00% <0.00%> (ø)`
app/app.go	`4.65% <62.50%> (+0.74%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

x/upgrade/test/integration_test.go

rach-id · 2023-04-18T00:32:08Z

x/upgrade/module.go

+// RegisterInterfaces overwrites the default upgrade module's method with a
+// noop.
+func (AppModule) RegisterInterfaces(registry codectypes.InterfaceRegistry) {
+}
+
+// RegisterServices overwrites the default upgrade module's method with a
+// noop.
+func (am AppModule) RegisterServices(cfg module.Configurator) {
+}
+
+// ConsensusVersion implements AppModule/ConsensusVersion.
+func (AppModule) ConsensusVersion() uint64 { return consensusVersion }
+
+// BeginBlock overwrites the default upgrade module's BeginBlock method with a
+// noop.
+func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
+}


Don't we need to also overwrite endBlock?

rach-id

LGTM

liamsi · 2023-04-18T08:03:57Z

The idea being that this maintains compatibility with the IBC module (which requires access to the upgrade module's keeper)

Can you link to that code or spec? Just curious what the IBC module does with the upgrade module's keeper. I assume it is to upgrade ibc clients?

evan-forbes · 2023-04-18T11:16:57Z

app/app.go

-	UpgradeKeeper    upgradekeeper.Keeper
+	UpgradeKeeper    sdkupgradekeeper.Keeper


I should also note that we are still selectively using components of the sdk's upgrade module, hence the change in naming to hopefully help differentiate the two. For example, see line 152, where we are using the appupgrade module instead of the sdk's

evan-forbes · 2023-04-18T11:28:44Z

cc @liamsi

Can you link to that code or spec? Just curious what the IBC module does with the upgrade module's keeper. I assume it is to upgrade ibc clients?

most of the usage afaik is here in the beginblock
https://github.com/cosmos/ibc-go/blob/d7da1b7c5e86648d8142c948ba3d6dd9194d3bf9/modules/core/02-client/abci.go#L11-L42

my current thinking is that we can reuse a lot of the upgrade module as we flesh out #1014. Like fulfilling this same api, and portions of things like upgradetypes.Plan/migrations.

cmwaters · 2023-04-18T15:01:21Z

x/upgrade/cli.go

+// GetTxCmd returns the transaction commands for this module
+func GetTxCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   upgradetypes.ModuleName,
+		Short: "Upgrade transaction subcommands",
+	}
+
+	return cmd
+}


Do we need this?

only if we decide that we do need the app module, since the AppModuleBasic interface returns a pointer

cmwaters · 2023-04-18T15:09:30Z

most of the usage afaik is here in the beginblock
https://github.com/cosmos/ibc-go/blob/d7da1b7c5e86648d8142c948ba3d6dd9194d3bf9/modules/core/02-client/abci.go#L11-L42

What about here: https://github.com/cosmos/ibc-go/blob/9e4b0d2f99c18db80db17697ec0875c48d84f328/modules/core/02-client/keeper/proposal.go#L73

Am I correct in thinking that IBC also has a handler that can kick off an upgrade?

evan-forbes · 2023-04-18T15:42:31Z

Am I correct in thinking that IBC also has a handler that can kick off an upgrade?

yeah it appears that that is used when other chains upgrade. For some reason, some of the ibc client state is stored in the upgrade keeper, which is a bit confusing since the ibc module is the only one that writes to that state.

damiannolan · 2023-04-18T16:53:14Z

Am I correct in thinking that IBC also has a handler that can kick off an upgrade?

yeah it appears that that is used when other chains upgrade. For some reason, some of the ibc client state is stored in the upgrade keeper, which is a bit confusing since the ibc module is the only one that writes to that state.

This is currently the only upgrade path for ibc client upgrades. Historically the sdk upgrade plan used to contain a proto Any field which has since been deprecated when x/ibc was migrated to the ibc-go repo.

The proposal handler in ibc-go essentially uses the upgrade keeper to schedule the upgrade plan and store the upgraded client state under an "upgrade path". The path currently being used as the standard/default is under the x/upgrade module at upgrade/UpgradedIBCState/{upgradeHeight}/upgradedClient. Afaik this can be changed but may be a little cumbersome. Essentially its a bit of technical debt from when ibc existed within the sdk.

cc. @AdityaSripal

evan-forbes · 2023-04-21T13:29:26Z

app/app.go

 	// ModuleEncodingRegisters keeps track of all the module methods needed to
 	// register interfaces and specific type to encoding config
-	ModuleEncodingRegisters = moduleMapToSlice(ModuleBasics)
+	ModuleEncodingRegisters = extractRegisters(ModuleBasics, appupgrade.TypeRegister{})


followed through with @cmwaters 's idea to remove the module all together, and now we're just manually registering the upgrade module's types. 81208f9

This is necessary for the IBC module (#1629 (comment))

rootulp

utACK. Do we want a x/upgrade/README.md that describes the diff between this repo's upgrade module and the standard SDK upgrade module?

x/upgrade/test/integration_test.go

Co-authored-by: Rootul P <[email protected]>

evan-forbes · 2023-04-21T22:27:51Z

@rootulp

good point, added a short one, since this module doesn't do much. Perhaps we can add to if we end up doing anything else with it.

cmwaters

LGTM

cmwaters · 2023-04-24T12:56:56Z

testutil/testnode/query.go

+		return nil, err
+	}
+
+	return node.Tx(context.Background(), hash, prove)


Does clientCtx have an underlying go Context?

hmm I thought so too, but myself and my IDE could not find it? maybe some other Contetx does and we should refactor to that?

https://github.com/celestiaorg/cosmos-sdk/blob/50f4c49034ee97424a43b32e77715f9c8413bb5d/client/context.go#L25-L63

Seems like the go context wraps around the sdk Context and not the other way around: https://github.com/celestiaorg/cosmos-sdk/blob/50f4c49034ee97424a43b32e77715f9c8413bb5d/x/bank/keeper/msg_server.go#L27

cmwaters · 2023-04-24T12:58:24Z

x/upgrade/READ_ME.md

@@ -0,0 +1,8 @@
+# `x/upgrade`


I think usually READ_ME.md is README

whoops lol 0f5dc67

good catch fixed

rootulp · 2023-04-25T02:34:52Z

x/upgrade/README.md

+The upgrade module removes the entrypoints to the standard upgrade module by not
+registering a message server. It registers the standard upgrade module types to
+preserve the ability to marshal them. Note that the keeper of the standard
+upgrade module is still added to the application.


feat!: accoy the ugrade module

b9b2a62

evan-forbes added apphash breaking labels Apr 14, 2023

evan-forbes added this to the Mainnet milestone Apr 14, 2023

evan-forbes self-assigned this Apr 14, 2023

evan-forbes added 5 commits April 14, 2023 09:04

chore: add test for removal

221771d

chore: typo

969ab3b

move query from app/test to testutil/testnode

0c0c9df

chore: add integration tests

42c280a

chore: linter

63bd773

evan-forbes commented Apr 17, 2023

View reviewed changes

evan-forbes marked this pull request as ready for review April 17, 2023 23:24

evan-forbes requested review from rootulp, mojtaba-esk, cmwaters and liamsi April 17, 2023 23:24

evan-forbes requested a review from rach-id April 17, 2023 23:34

rach-id reviewed Apr 18, 2023

View reviewed changes

chore: fix typo

62f6a5f

MSevey requested a review from a team April 18, 2023 00:37

rach-id previously approved these changes Apr 18, 2023

View reviewed changes

evan-forbes commented Apr 18, 2023

View reviewed changes

cmwaters reviewed Apr 18, 2023

View reviewed changes

evan-forbes requested a review from cmwaters April 20, 2023 12:10

chore: remove the module and just register types manually

81208f9

evan-forbes dismissed rach-id’s stale review via 81208f9 April 21, 2023 13:27

MSevey requested a review from a team April 21, 2023 13:27

evan-forbes commented Apr 21, 2023

View reviewed changes

evan-forbes added 2 commits April 21, 2023 08:30

chore: linter

2f3d29d

docs: docs

ee156de

evan-forbes requested a review from rach-id April 21, 2023 13:34

rootulp previously approved these changes Apr 21, 2023

View reviewed changes

x/upgrade/test/integration_test.go Outdated Show resolved Hide resolved

evan-forbes dismissed rootulp’s stale review via 593e030 April 21, 2023 21:50

chore: test log adaptation

593e030

Co-authored-by: Rootul P <[email protected]>

MSevey requested a review from a team April 21, 2023 21:50

docs: add short readme

f41e825

Merge branch 'main' into evan/remove-token-voting

36ed8b8

cmwaters previously approved these changes Apr 24, 2023

View reviewed changes

chore: fix readme file name

0f5dc67

evan-forbes dismissed cmwaters’s stale review via 0f5dc67 April 24, 2023 13:34

MSevey requested a review from a team April 24, 2023 13:34

evan-forbes requested review from cmwaters and rootulp April 24, 2023 13:36

evan-forbes added 3 commits April 24, 2023 08:40

Merge branch 'main' into evan/remove-token-voting

ba9e1e6

fix: imports

848bed8

fix:imports

e6a4445

cmwaters approved these changes Apr 24, 2023

View reviewed changes

evan-forbes enabled auto-merge (squash) April 24, 2023 19:39

rootulp approved these changes Apr 25, 2023

View reviewed changes

evan-forbes merged commit a69fe55 into main Apr 25, 2023

evan-forbes deleted the evan/remove-token-voting branch April 25, 2023 02:35

evan-forbes mentioned this pull request Jul 27, 2023

chore: audit x/upgrade module #2072

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: remove token voting upgrades #1629

feat!: remove token voting upgrades #1629

evan-forbes commented Apr 14, 2023 •

edited

Loading

evan-forbes Apr 17, 2023

rach-id Apr 18, 2023

evan-forbes Apr 18, 2023 •

edited

Loading

rach-id Apr 18, 2023

cmwaters Apr 18, 2023

evan-forbes Apr 18, 2023

evan-forbes Apr 17, 2023 •

edited

Loading

codecov-commenter commented Apr 17, 2023 •

edited

Loading

rach-id Apr 18, 2023

rach-id left a comment

liamsi commented Apr 18, 2023

evan-forbes Apr 18, 2023

evan-forbes commented Apr 18, 2023 •

edited

Loading

cmwaters Apr 18, 2023

evan-forbes Apr 18, 2023

cmwaters commented Apr 18, 2023

evan-forbes commented Apr 18, 2023

damiannolan commented Apr 18, 2023

evan-forbes Apr 21, 2023 •

edited

Loading

rootulp left a comment

evan-forbes commented Apr 21, 2023

cmwaters left a comment

cmwaters Apr 24, 2023

evan-forbes Apr 24, 2023

cmwaters Apr 24, 2023

cmwaters Apr 24, 2023

evan-forbes Apr 24, 2023

rootulp Apr 25, 2023

		UpgradeKeeper upgradekeeper.Keeper
		UpgradeKeeper sdkupgradekeeper.Keeper

feat!: remove token voting upgrades #1629

feat!: remove token voting upgrades #1629

Conversation

evan-forbes commented Apr 14, 2023 • edited Loading

Overview

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

liamsi commented Apr 18, 2023

Choose a reason for hiding this comment

evan-forbes commented Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Apr 18, 2023

evan-forbes commented Apr 18, 2023

damiannolan commented Apr 18, 2023

evan-forbes Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes commented Apr 21, 2023

cmwaters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Apr 14, 2023 •

edited

Loading

evan-forbes Apr 18, 2023 •

edited

Loading

evan-forbes Apr 17, 2023 •

edited

Loading

codecov-commenter commented Apr 17, 2023 •

edited

Loading

evan-forbes commented Apr 18, 2023 •

edited

Loading

evan-forbes Apr 21, 2023 •

edited

Loading