addressed review comments

cosmos · Aug 7, 2024 · 37125ba · 37125ba
1 parent 35978c7
commit 37125ba
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 136 deletions.
diff --git a/app/provider/app.go b/app/provider/app.go
@@ -114,7 +114,6 @@ import (
 	no_valupdates_genutil "github.com/cosmos/interchain-security/v5/x/ccv/no_valupdates_genutil"
 	no_valupdates_staking "github.com/cosmos/interchain-security/v5/x/ccv/no_valupdates_staking"
 	ibcprovider "github.com/cosmos/interchain-security/v5/x/ccv/provider"
-	ibcproviderclient "github.com/cosmos/interchain-security/v5/x/ccv/provider/client"
 	ibcproviderkeeper "github.com/cosmos/interchain-security/v5/x/ccv/provider/keeper"
 	providertypes "github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
 )
@@ -144,7 +143,6 @@ var (
 		gov.NewAppModuleBasic(
 			[]govclient.ProposalHandler{
 				paramsclient.ProposalHandler,
-				ibcproviderclient.ChangeRewardDenomsProposalHandler,
 			},
 		),
 		mint.AppModuleBasic{},
@@ -576,7 +574,6 @@ func New(
 			govtypes.ModuleName: gov.NewAppModuleBasic(
 				[]govclient.ProposalHandler{
 					paramsclient.ProposalHandler,
-					ibcproviderclient.ChangeRewardDenomsProposalHandler,
 				},
 			),
 		})

diff --git a/docs/docs/upgrading/migrate_v4_v5.md b/docs/docs/upgrading/migrate_v4_v5.md
@@ -159,36 +159,28 @@ interchain-security-pd q provider params -o json
 
 ### Governance proposals
 
+Submitting the following legacy proposals is still supported:
+
 # Consumer addition proposal
 
 ```shell
-interchain-security-pd tx gov submit-proposal <proposal_file.json>
+interchain-security-pd tx gov submit-legacy-proposal consumer-addition <proposal_file.json>
 ```
 
 # Consumer removal proposal
 
 ```shell
-interchain-security-pd tx gov submit-proposal <proposal_file.json>
+interchain-security-pd tx gov submit-legacy-proposal consumer-removal <proposal_file.json>
 ```
 
-# Consumer modification proposal
+# Consumer addition proposal
 ```shell
-interchain-security-pd tx gov submit-proposal <proposal_file.json>
+interchain-security-pd tx gov submit-legacy-proposal change-reward-denoms <proposal_file.json>
 ```
 
-Run `interchain-security-pd tx gov draft-proposal` command and select in `other` one of the following
-message types to generate a draft example:
-- `/interchain_security.ccv.provider.v1.MsgConsumerAddition`
-- `/interchain_security.ccv.provider.v1.MsgConsumerModification`
-- `/interchain_security.ccv.provider.v1.MsgConsumerRemoval`
+You may also submit proposal messages above using `submit-proposal`.
 
 
-# Change reward denoms
-
-```shell
-interchain-security-pd tx gov submit-legacy-proposal change-reward-denoms <proposal_file.json>
-```
-
 ## Consumer
 
 ### Keeper initialization
@@ -234,22 +226,22 @@ app.ConsumerKeeper = ibcconsumerkeeper.NewKeeper(
     // for x/ccv/democracy using the x/gov module address is correct
     // if you don't have a way of updating consumer params you may still use the line below as it will have no affect
 +   authtypes.NewModuleAddress(govtypes.ModuleName).String(),
-
-    // add address codecs
+    
+    // add address codecs  
 +   authcodec.NewBech32Codec(sdk.GetConfig().GetBech32ValidatorAddrPrefix()),
 +   authcodec.NewBech32Codec(sdk.GetConfig().GetBech32ConsensusAddrPrefix()),
 )
 ```
 
 
 * `authority` was added - requirement for executing `MsgUpdateParams`
-    * make sure the authority address makes sense for your chain
+    * make sure the authority address makes sense for your chain   
     * the exact module account may differ depending on your setup (`x/gov`, `x/admin` or custom module)
     * for `x/ccv/democracy` using the `x/gov` module address is correct
     * if you don't have a way of updating consumer params you may use `authtypes.NewModuleAddress(govtypes.ModuleName).String()` (has no effect on functionality)
 
 * `validatorAddressCodec` & `consensusAddressCodec` were added - they must match the bech32 address codec used by `x/auth`, `x/bank`, `x/staking`
-
+ 
 
 ### Additions
 
@@ -258,7 +250,7 @@ app.ConsumerKeeper = ibcconsumerkeeper.NewKeeper(
 **This functionality is not supported on `x/ccv/consumer` without additional configuration.**
 * if you are using `x/ccv/democracy` the feature is supported out of the box
 * if you are using custom logic for changing consumer params, please update your code by providing the appropriate `authority` module account during `ConsumerKeeper` initialization in `app.go`.
-
+    
 **You must add `"/interchain_security.ccv.consumer.v1.MsgUpdateParams"` to your parameters whitelist to be able to change `ccvconsumer` parameters via governance.**
 
 It is available when using `gov` CLI commands:
@@ -384,7 +376,7 @@ The consumer implements the `StakingKeeper` interface shown above.
 
 ## Democracy
 
-Changes in `Consumer` also apply to `Democracy`.
+Changes in `Consumer` also apply to `Democracy`. 
 
 Democracy `x/staking`, `x/distribution` and `x/gov` were updated to reflect changes in `cosmos-sdk v0.50.x`.
 

diff --git a/docs/docs/upgrading/migrate_v5_v6.md b/docs/docs/upgrading/migrate_v5_v6.md
@@ -0,0 +1,42 @@
+---
+sidebar_position: 1
+---
+
+# Upgrading to ICS v6.x from v5.x
+
+ICS specific changes are outlined below.
+
+Pre-requisite version for this upgrade: any from the `v5.x` release line.
+
+## Note
+
+## Provider
+
+### Migration (v5 -> v6)
+
+ConensusVersion was bumped to `6`.
+
+### Governance proposals
+
+#### Consumer addition proposal
+
+```shell
+interchain-security-pd tx gov submit-proposal <proposal_file.json>
+```
+
+####  Consumer removal proposal
+
+```shell
+interchain-security-pd tx gov submit-proposal <proposal_file.json>
+```
+
+####  Consumer modification proposal
+```shell
+interchain-security-pd tx gov submit-proposal <proposal_file.json>
+```
+
+Run `interchain-security-pd tx gov draft-proposal` command and select in `other` one of the following
+message types to generate a draft example:
+- `/interchain_security.ccv.provider.v1.MsgConsumerAddition`
+- `/interchain_security.ccv.provider.v1.MsgConsumerModification`
+- `/interchain_security.ccv.provider.v1.MsgConsumerRemoval`
diff --git a/tests/e2e/action_rapid_test.go b/tests/e2e/action_rapid_test.go
@@ -99,6 +99,7 @@ func GetActionGen() *rapid.Generator[any] {
 func CreateSubmitChangeRewardDenomsProposalActionGen() *rapid.Generator[SubmitChangeRewardDenomsProposalAction] {
 	return rapid.Custom(func(t *rapid.T) SubmitChangeRewardDenomsProposalAction {
 		return SubmitChangeRewardDenomsProposalAction{
+			Chain:   GetChainIDGen().Draw(t, "Chain"),
 			From:    GetValidatorIDGen().Draw(t, "From"),
 			Deposit: rapid.Uint().Draw(t, "Deposit"),
 			Denom:   rapid.String().Draw(t, "Denom"),

diff --git a/tests/e2e/actions.go b/tests/e2e/actions.go
@@ -21,8 +21,6 @@ import (
 	"golang.org/x/mod/semver"
 
 	e2e "github.com/cosmos/interchain-security/v5/tests/e2e/testlib"
-	"github.com/cosmos/interchain-security/v5/x/ccv/provider/client"
-	"github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
 	ccvtypes "github.com/cosmos/interchain-security/v5/x/ccv/types"
 )
 
@@ -2019,54 +2017,70 @@ func (tr Chain) registerRepresentative(
 }
 
 type SubmitChangeRewardDenomsProposalAction struct {
+	Chain   ChainID
 	Denom   string
 	Deposit uint
 	From    ValidatorID
 }
 
 func (tr Chain) submitChangeRewardDenomsProposal(action SubmitChangeRewardDenomsProposalAction, verbose bool) {
-	providerChain := tr.testConfig.chainConfigs[ChainID("provi")]
-
-	prop := client.ChangeRewardDenomsProposalJSON{
-		Summary: "Change reward denoms",
-		ChangeRewardDenomsProposal: types.ChangeRewardDenomsProposal{
-			Title:          "Change reward denoms",
-			Description:    "Change reward denoms",
-			DenomsToAdd:    []string{action.Denom},
-			DenomsToRemove: []string{"stake"},
-		},
-		Deposit: fmt.Sprint(action.Deposit) + `stake`,
-	}
-
-	bz, err := json.Marshal(prop)
-	if err != nil {
-		log.Fatal(err)
-	}
+	template := `
+{
+ "messages": [
+  {
+   "@type": "/interchain_security.ccv.provider.v1.MsgChangeRewardDenoms",
+   "denoms_to_add": %s,
+   "denoms_to_remove": %s,
+   "authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn"
+  }
+ ],
+ "metadata": "ipfs://CID",
+ "deposit": "%dstake",
+ "title": "change reward denoms",
+ "summary": "Proposal to change reward denoms",
+ "expedited": false
+}`
 
-	jsonStr := string(bz)
-	if strings.Contains(jsonStr, "'") {
-		log.Fatal("prop json contains single quote")
-	}
+	denomsToAdd := []string{action.Denom}
+	denomsToRemove := []string{"stake"}
+	jsonStr := fmt.Sprintf(template,
+		denomsToAdd,
+		denomsToRemove,
+		action.Deposit)
 
-	bz, err = tr.target.ExecCommand(
-		"/bin/bash", "-c", fmt.Sprintf(`echo '%s' > %s`, jsonStr, "/change-reward-denoms-proposal.json")).CombinedOutput()
+	//#nosec G204 -- bypass unsafe quoting warning (no production code)
+	proposalFile := "/consumer-addition.proposal"
+	bz, err := tr.target.ExecCommand(
+		"/bin/bash", "-c", fmt.Sprintf(`echo '%s' > %s`, jsonStr, proposalFile),
+	).CombinedOutput()
 	if err != nil {
 		log.Fatal(err, "\n", string(bz))
 	}
 
 	// CHANGE REWARDS DENOM PROPOSAL
-	bz, err = tr.target.ExecCommand(providerChain.BinaryName,
-		"tx", "gov", "submit-legacy-proposal", "change-reward-denoms", "/change-reward-denoms-proposal.json",
+	cmd := tr.target.ExecCommand(
+		tr.testConfig.chainConfigs[action.Chain].BinaryName,
+		"tx", "gov", "submit-proposal", proposalFile,
 		`--from`, `validator`+fmt.Sprint(action.From),
-		`--chain-id`, string(providerChain.ChainId),
-		`--home`, tr.getValidatorHome(providerChain.ChainId, action.From),
-		`--node`, tr.getValidatorNode(providerChain.ChainId, action.From),
-		`--gas`, "9000000",
+		`--chain-id`, string(tr.testConfig.chainConfigs[action.Chain].ChainId),
+		`--home`, tr.getValidatorHome(action.Chain, action.From),
+		`--gas`, `900000`,
+		`--node`, tr.getValidatorNode(action.Chain, action.From),
 		`--keyring-backend`, `test`,
 		`-y`,
-	).CombinedOutput()
+	)
+
+	if verbose {
+		fmt.Println("change rewards denom props cmd:", cmd.String())
+		fmt.Println("change rewards denom props json:", jsonStr)
+	}
+	bz, err = cmd.CombinedOutput()
 	if err != nil {
-		log.Fatal(err, "\n", string(bz))
+		log.Fatal("submit-proposal failed:", err, "\n", string(bz))
+	}
+
+	if verbose {
+		fmt.Println("change rewards denom props output:", string(bz))
 	}
 
 	// wait for inclusion in a block -> '--broadcast-mode block' is deprecated

diff --git a/tests/e2e/steps_democracy.go b/tests/e2e/steps_democracy.go
@@ -152,6 +152,7 @@ func stepsDemocracy(consumerName string, expectRegisteredRewardDistribution bool
 		},
 		{
 			Action: SubmitChangeRewardDenomsProposalAction{
+				Chain:   ChainID("provi"),
 				Denom:   consumerRewardDenom,
 				Deposit: 10000001,
 				From:    ValidatorID("bob"),

diff --git a/tests/integration/provider_gov_hooks.go b/tests/integration/provider_gov_hooks.go
@@ -61,7 +61,7 @@ func (s *CCVTestSuite) TestGetConsumerAdditionFromProp() {
 	s.Require().NoError(err)
 
 	// create a valid consumer addition proposal
-	addConsumerProp := testkeeper.GetTestMsgConsumerAddition()
+	msgConsumerAddition := testkeeper.GetTestMsgConsumerAddition()
 
 	// create a legacy consumer addition proposal content
 	// (not supported anymore)
@@ -107,8 +107,8 @@ func (s *CCVTestSuite) TestGetConsumerAdditionFromProp() {
 			expPanic:                false,
 		},
 		{
-			name:                    "msg contains a prop of legacy ConsumerAdditionProposal type - hook should create a new proposed chain",
-			propMsg:                 &addConsumerProp,
+			name:                    "msg contains a prop of MsgConsumerAddition type - hook should create a new proposed chain",
+			propMsg:                 &msgConsumerAddition,
 			expectConsumerPropFound: true,
 			expPanic:                false,
 		},

diff --git a/x/ccv/provider/client/legacy_proposal_handler.go b/x/ccv/provider/client/legacy_proposal_handler.go