feat: introduce cli commands for the upgrade module #2910

cmwaters · 2023-12-06T16:11:09Z

Addresses: #2875

This also adds an integration test to the upgrade module

…etwork

coderabbitai · 2023-12-06T16:11:19Z

Walkthrough

The changes across various files indicate an update to the upgrade module in a blockchain application, likely built on the Cosmos SDK. The updates include the addition of new RPC functions and message types for upgrade attempts, modifications to keeper logic and tallying of votes, and enhancements to testing suites to cover the new functionality. The changes suggest a shift towards a more dynamic and possibly governance-driven upgrade process.

Changes

File Path	Change Summary
`app/app.go`, `proto/.../tx.proto`	Modified `UpgradeKeeper` initialization; added `ResetTally` call in `EndBlocker`. Added `TryUpgrade` RPC function and related message types.
`test/util/test_app.go`, `x/upgrade/cli/cli_test.go`	Added import; modified `SetupTestAppWithGenesisValSet` to set `Version`. Added test functions and setup function; updated imports.
`x/upgrade/cli/query.go`, `x/upgrade/cli/tx.go`	Introduced `cli` package; added `CmdQueryTally`, `GetTxCmd`, `CmdSignalVersion`, and `CmdTryUpgrade` functions.
`x/upgrade/integration_test.go`, `x/upgrade/keeper.go`	Added `TestUpgradeIntegration` test function. Changed `Keeper` to pointer receiver; added `TryUpgrade` and `ResetTally` methods; modified `VersionTally` and `TallyVotingPower`.
`x/upgrade/module.go`, `x/upgrade/tally_test.go`	Added import; changed parameter to pointer; removed `EndBlock` function. Added `TestEmptyStore` function; refactored `setup` function; added `newMockStakingKeeper` constructor.
`x/upgrade/types/msgs.go`, `app/test/std_sdk_test.go`	Added `MsgTryUpgrade` entity and related methods; modified `MsgSignalVersion` declaration. Added import; added `cfg` field; added methods; added test cases.
`test/util/genesis/genesis.go`, `test/util/testnode/config.go`	Removed import; changed function call; removed `DefaultConsensusParams` function. Removed import; modified `DefaultConfig` function.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

x/upgrade/tally_test.go

x/upgrade/integration_test.go

x/upgrade/cli/query.go

x/upgrade/cli/tx.go

evan-forbes

dope!

do you think its worth it to run the msgs through the anti handlers in some way? perhaps we could just add them to

celestia-app/app/test/std_sdk_test.go

Line 74 in c5e1786

func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() {

(and maybe rename that test)

it might be overly paranoid, but the reason being that we could remove the output to something like the getsigners method (or other critical piece) atm and all tests would pass.

rootulp

LGTM

x/upgrade/cli/query.go

x/upgrade/cli/tx.go

rootulp · 2023-12-06T19:26:46Z

x/upgrade/integration_test.go

+	})
+	require.NoError(t, err)
+	require.EqualValues(t, 1, res.VotingPower)
+	require.EqualValues(t, 0, res.ThresholdPower)


[optional] this threshold power seems weird b/c it makes me think that no votes are needed to trigger an upgrade.
Is it possible for this test case to have a non-zero threshold power potentially by adding another validator with some voting power?

So currently as it stands, there has to be strictly greater than the threshold power for the upgrade to occur. If we think it is better to present threshold as "greater or equal to" then I can increment this buy 1. Whatever we feel is more intuitive for users

x/upgrade/tally_test.go

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

coderabbitai · 2023-12-07T10:34:14Z

x/upgrade/cli/tx.go

+func CmdTryUpgrade() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "try-upgrade",
+		Short: "Try to perform a software upgrade",
+		Long: `This command will submit a TryUpgrade message to tally all
+the signal votes. If a quorum has been reached, the network will upgrade 
+to the signalled version at the following height.
+`,


The CmdTryUpgrade command's description provides information on the tallying of votes and the upgrade process. It would be beneficial to include additional details or output examples in the help text or documentation to guide users on what to expect after executing this command, especially regarding how they can confirm that an upgrade is in progress or will occur.

… into cal/upgrade-cli

cmwaters · 2023-12-07T11:28:36Z

FYI: I've modified the meaning of threshold here from "strictly greater" to "greater than equal" which I think is more intuitive for users. I've added some test cases to make sure my math is right

cmwaters · 2023-12-07T11:56:22Z

do you think its worth it to run the msgs through the anti handlers in some way? perhaps we could just add them to

celestia-app/app/test/std_sdk_test.go

Line 74 in c5e1786

func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() {

(and maybe rename that test)
it might be overly paranoid, but the reason being that we could remove the output to something like the getsigners method (or other critical piece) atm and all tests would pass.

Yup! done

rootulp

🚢 great work!

cmwaters added 4 commits November 30, 2023 17:30

introduce a TryUpgrade message as a crank message for upgrading the n…

5d1a7d9

…etwork

Use AccAddress instead of ValAddress

d85b606

feat: reset tally after quorum has reached

874aaa0

add integration tests and cli tests

8482daf

cmwaters requested review from evan-forbes and rootulp as code owners December 6, 2023 16:11

celestia-bot requested a review from a team December 6, 2023 16:11

Merge branch 'main' into cal/upgrade-cli

03f5b9b

staheri14 reviewed Dec 6, 2023

View reviewed changes

x/upgrade/tally_test.go Show resolved Hide resolved

x/upgrade/integration_test.go Show resolved Hide resolved

x/upgrade/cli/query.go Outdated Show resolved Hide resolved

x/upgrade/cli/tx.go Show resolved Hide resolved

evan-forbes previously approved these changes Dec 6, 2023

View reviewed changes

rootulp assigned cmwaters Dec 6, 2023

rootulp previously approved these changes Dec 6, 2023

View reviewed changes

cmwaters dismissed stale reviews from rootulp and evan-forbes via 6f6c72a December 7, 2023 10:33

Apply suggestions from code review

6f6c72a

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

celestia-bot requested a review from a team December 7, 2023 10:33

coderabbitai bot reviewed Dec 7, 2023

View reviewed changes

add some comments to the tests

c15ff0b

cmwaters requested review from staheri14, rootulp and evan-forbes December 7, 2023 10:48

cmwaters added 3 commits December 7, 2023 11:48

Merge branch 'main' into cal/upgrade-cli

662d61e

theshold is greater than equal rather than strictly greater

03aa698

Merge branch 'cal/upgrade-cli' of github.com:celestiaorg/celestia-app…

bbdcd15

… into cal/upgrade-cli

add tests to the sdk integration test suite

d3487b3

evan-forbes previously approved these changes Dec 7, 2023

View reviewed changes

rootulp previously approved these changes Dec 7, 2023

View reviewed changes

fix tests

2e29003

cmwaters dismissed stale reviews from rootulp and evan-forbes via 2e29003 December 7, 2023 21:54

celestia-bot requested a review from a team December 7, 2023 21:54

evan-forbes approved these changes Dec 7, 2023

View reviewed changes

rootulp approved these changes Dec 7, 2023

View reviewed changes

cmwaters merged commit 0c09689 into main Dec 8, 2023
30 checks passed

cmwaters deleted the cal/upgrade-cli branch December 8, 2023 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce cli commands for the upgrade module #2910

feat: introduce cli commands for the upgrade module #2910

cmwaters commented Dec 6, 2023

coderabbitai bot commented Dec 6, 2023 •

edited

Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

evan-forbes left a comment •

edited

Loading

rootulp left a comment

rootulp Dec 6, 2023

cmwaters Dec 7, 2023

coderabbitai bot Dec 7, 2023

cmwaters commented Dec 7, 2023

cmwaters commented Dec 7, 2023

rootulp left a comment

feat: introduce cli commands for the upgrade module #2910

feat: introduce cli commands for the upgrade module #2910

Conversation

cmwaters commented Dec 6, 2023

coderabbitai bot commented Dec 6, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp Dec 6, 2023

Choose a reason for hiding this comment

cmwaters Dec 7, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 7, 2023

Choose a reason for hiding this comment

cmwaters commented Dec 7, 2023

cmwaters commented Dec 7, 2023

rootulp left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Dec 6, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

evan-forbes left a comment •

edited

Loading