feat!: provider proposal for changing reward denoms (#1280)

* new provider prop type

* add methods and tests for new prop, update docs

* remove old tx, fix tests

* e2e handling

* fix command type

* boilerplate

* fix e2e tests

* Update CHANGELOG.md

* lint

* validate denoms

* Update proposal.go

* rm msg string

* fix tests

* rm chain in change denom action

* lint

* test for invalid denom

* events for both add and remove

* Update proposal_test.go

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 Add an entry to the unreleased provider section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a provider release.
 
+* (feature!) [#1280](https://github.com/cosmos/interchain-security/pull/1280) provider proposal for changing reward denoms
 * (feature!) [#1244](https://github.com/cosmos/interchain-security/pull/1244) Update the default consumer unbonding period to 2 weeks.
 * (deps) [#1259](https://github.com/cosmos/interchain-security/pull/1259) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.5).
 * (deps!) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [ibc-go](https://github.com/cosmos/ibc-go) to [v7.3.0](https://github.com/cosmos/ibc-go/releases/tag/v7.3.0).

app/provider/app.go

@@ -141,6 +141,7 @@ var (
 				ibcproviderclient.ConsumerAdditionProposalHandler,
 				ibcproviderclient.ConsumerRemovalProposalHandler,
 				ibcproviderclient.EquivocationProposalHandler,
+				ibcproviderclient.ChangeRewardDenomsProposalHandler,
 			},
 		),
 		params.AppModuleBasic{},

docs/docs/features/proposals.md

@@ -105,6 +105,23 @@ Minimal example:
 }
 ```
 
+## ChangeRewardDenomProposal
+:::tip
+`ChangeRewardDenomProposal` will only be accepted on the provider chain if at least one of the denomsToAdd or denomsToRemove fields is populated with at least one denom. Also, a denom cannot be repeated in both sets.
+:::
+
+Proposal type used to mutate the set of denoms accepted by the provider as rewards.
+
+Minimal example:
+```js
+{
+  "title": "Add untrn as a reward denom",
+  "description": "Here is more information about the proposal",
+  "denomsToAdd": ["untrn"],
+  "denomsToRemove": []
+}
+```
+
 ### Notes
 When submitting equivocation evidence through an `EquivocationProposal` please take note that you need to use the consensus address (`valcons`) of the offending validator on the **provider chain**.
 Besides that, the `height` and the `time` fields should be mapped to the **provider chain** to avoid your evidence being rejected.

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -116,6 +116,19 @@ message EquivocationProposal {
   repeated cosmos.evidence.v1beta1.Equivocation equivocations = 3;
 }
 
+// ChangeRewardDenomsProposal is a governance proposal on the provider chain to
+// mutate the set of denoms accepted by the provider as rewards.
+message ChangeRewardDenomsProposal {
+  // the title of the proposal
+  string title = 1;
+  // the description of the proposal
+  string description = 2;
+  // the list of consumer reward denoms to add
+  repeated string denoms_to_add = 3;
+  // the list of consumer reward denoms to remove
+  repeated string denoms_to_remove = 4;
+}
+
 // A persisted queue entry indicating that a slash packet data instance needs to
 // be handled. This type belongs in the "global" queue, to coordinate slash
 // packet handling times between consumers.

proto/interchain_security/ccv/provider/v1/tx.proto

@@ -12,8 +12,6 @@ import "google/protobuf/any.proto";
 service Msg {
   rpc AssignConsumerKey(MsgAssignConsumerKey)
       returns (MsgAssignConsumerKeyResponse);
-  rpc RegisterConsumerRewardDenom(MsgRegisterConsumerRewardDenom)
-      returns (MsgRegisterConsumerRewardDenomResponse);
 }
 
 message MsgAssignConsumerKey {
@@ -30,18 +28,3 @@ message MsgAssignConsumerKey {
 }
 
 message MsgAssignConsumerKeyResponse {}
-
-// MsgRegisterConsumerRewardDenom allows an account to register
-// a consumer reward denom, i.e., add it to the list of denoms
-// accepted by the provider as rewards.
-message MsgRegisterConsumerRewardDenom {
-  option (gogoproto.equal) = false;
-  option (gogoproto.goproto_getters) = false;
-
-  string denom = 1;
-  string depositor = 2;
-}
-
-// MsgRegisterConsumerRewardDenomResponse defines the
-// Msg/RegisterConsumerRewardDenom response type.
-message MsgRegisterConsumerRewardDenomResponse {}

tests/e2e/actions.go

@@ -1751,35 +1751,63 @@ func (tr TestRun) registerRepresentative(
 	wg.Wait()
 }
 
-type registerConsumerRewardDenomAction struct {
-	chain chainID
-	from  validatorID
-	denom string
+type submitChangeRewardDenomsProposalAction struct {
+	denom   string
+	deposit uint
+	from    validatorID
 }
 
-func (tr TestRun) registerConsumerRewardDenom(action registerConsumerRewardDenomAction, verbose bool) {
+func (tr TestRun) submitChangeRewardDenomsProposal(action submitChangeRewardDenomsProposalAction, verbose bool) {
+	providerChain := tr.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)
+	}
+
+	jsonStr := string(bz)
+	if strings.Contains(jsonStr, "'") {
+		log.Fatal("prop json contains single quote")
+	}
+
 	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
-	bz, err := exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[action.chain].binaryName,
-		"tx", "provider", "register-consumer-reward-denom", action.denom,
+	bz, err = exec.Command("docker", "exec", tr.containerConfig.instanceName,
+		"/bin/bash", "-c", fmt.Sprintf(`echo '%s' > %s`, jsonStr, "/change-reward-denoms-proposal.json")).CombinedOutput()
 
+	if err != nil {
+		log.Fatal(err, "\n", string(bz))
+	}
+
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	// CHANGE REWARDS DENOM PROPOSAL
+	bz, err = exec.Command("docker", "exec", tr.containerConfig.instanceName, providerChain.binaryName,
+		"tx", "gov", "submit-legacy-proposal", "change-reward-denoms", "/change-reward-denoms-proposal.json",
 		`--from`, `validator`+fmt.Sprint(action.from),
-		`--chain-id`, string(action.chain),
-		`--home`, tr.getValidatorHome(action.chain, action.from),
-		`--node`, tr.getValidatorNode(action.chain, action.from),
+		`--chain-id`, string(providerChain.chainId),
+		`--home`, tr.getValidatorHome(providerChain.chainId, action.from),
+		`--node`, tr.getValidatorNode(providerChain.chainId, action.from),
 		`--gas`, "9000000",
 		`--keyring-backend`, `test`,
 		`-y`,
 	).CombinedOutput()
 
-	if verbose {
-		fmt.Println("redelegate cmd:", string(bz))
-	}
-
 	if err != nil {
 		log.Fatal(err, "\n", string(bz))
 	}
 
-	tr.waitBlocks(action.chain, 2, 10*time.Second)
+	// wait for inclusion in a block -> '--broadcast-mode block' is deprecated
+	tr.waitBlocks(chainID("provi"), 2, 30*time.Second)
 }
 
 // Creates an additional node on selected chain

tests/e2e/main.go

@@ -258,8 +258,8 @@ func (tr *TestRun) runStep(step Step, verbose bool) {
 		tr.waitForSlashThrottleDequeue(action, verbose)
 	case startRelayerAction:
 		tr.startRelayer(action, verbose)
-	case registerConsumerRewardDenomAction:
-		tr.registerConsumerRewardDenom(action, verbose)
+	case submitChangeRewardDenomsProposalAction:
+		tr.submitChangeRewardDenomsProposal(action, verbose)
 	default:
 		log.Fatalf("unknown action in testRun %s: %#v", tr.name, action)
 	}

tests/e2e/steps_democracy.go

@@ -133,19 +133,29 @@ func stepsDemocracy(consumerName string) []Step {
 			},
 		},
 		{
-			action: registerConsumerRewardDenomAction{
-				chain: chainID("provi"),
-				from:  validatorID("bob"),
-				denom: consumerRewardDenom,
+			action: submitChangeRewardDenomsProposalAction{
+				denom:   consumerRewardDenom,
+				deposit: 10000001,
+				from:    validatorID("bob"),
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					// Denom not yet registered, gov prop needs to pass first
+					RegisteredConsumerRewardDenoms: &[]string{},
+				},
+			},
+		},
+		{
+			action: voteGovProposalAction{
+				chain:      chainID("provi"),
+				from:       []validatorID{validatorID("alice"), validatorID("bob"), validatorID("carol")},
+				vote:       []string{"yes", "yes", "yes"},
+				propNumber: 2,
 			},
 			state: State{
 				chainID("provi"): ChainState{
 					// Check that the denom is registered on provider chain
 					RegisteredConsumerRewardDenoms: &[]string{consumerRewardDenom},
-					ValBalances: &map[validatorID]uint{
-						// make sure that bob's account was debited
-						validatorID("bob"): 9490000000,
-					},
 				},
 			},
 		},

tests/e2e/steps_reward_denom.go

@@ -131,19 +131,29 @@ func stepsRewardDenomConsumer(consumerName string) []Step {
 			},
 		},
 		{
-			action: registerConsumerRewardDenomAction{
-				chain: chainID("provi"),
-				from:  validatorID("bob"),
-				denom: "ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9",
+			action: submitChangeRewardDenomsProposalAction{
+				denom:   "ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9",
+				deposit: 10000001,
+				from:    validatorID("bob"),
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					// Denom not yet registered, gov prop needs to pass first
+					RegisteredConsumerRewardDenoms: &[]string{},
+				},
+			},
+		},
+		{
+			action: voteGovProposalAction{
+				chain:      chainID("provi"),
+				from:       []validatorID{validatorID("alice"), validatorID("bob"), validatorID("carol")},
+				vote:       []string{"yes", "yes", "yes"},
+				propNumber: 2,
 			},
 			state: State{
 				chainID("provi"): ChainState{
 					// Check that the denom is registered on provider chain
 					RegisteredConsumerRewardDenoms: &[]string{"ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9"},
-					ValBalances: &map[validatorID]uint{
-						// make sure that bob's account was debited
-						validatorID("bob"): 9490000000,
-					},
 				},
 			},
 		},

tests/integration/distribution.go

@@ -7,7 +7,6 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
-	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 
 	icstestingutils "github.com/cosmos/interchain-security/v3/testutil/integration"
 	consumerkeeper "github.com/cosmos/interchain-security/v3/x/ccv/consumer/keeper"
@@ -96,41 +95,13 @@ func (s *CCVTestSuite) TestRewardsDistribution() {
 	// Check that the coins got into the ConsumerRewardsPool
 	s.Require().True(rewardCoins[ibcCoinIndex].Amount.Equal(providerExpectedRewards[0].Amount))
 
-	// Attempt to register the consumer reward denom, but fail because the account has no coins
-
-	// Get the balance of delAddr to send it out
-	senderCoins := providerBankKeeper.GetAllBalances(s.providerCtx(), delAddr)
-
-	// Send the coins to the governance module just to have a place to send them
-	err = providerBankKeeper.SendCoinsFromAccountToModule(s.providerCtx(), delAddr, govtypes.ModuleName, senderCoins)
-	s.Require().NoError(err)
-
-	// Attempt to register the consumer reward denom, but fail because the account has no coins
-	err = s.providerApp.GetProviderKeeper().RegisterConsumerRewardDenom(s.providerCtx(), rewardCoins[ibcCoinIndex].Denom, delAddr)
-	s.Require().Error(err)
-
 	// Advance a block and check that the coins are still in the ConsumerRewardsPool
 	s.providerChain.NextBlock()
 	rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool)
 	s.Require().True(rewardCoins[ibcCoinIndex].Amount.Equal(providerExpectedRewards[0].Amount))
 
-	// Successfully register the consumer reward denom this time
-
-	// Send the coins back to the delAddr
-	err = providerBankKeeper.SendCoinsFromModuleToAccount(s.providerCtx(), govtypes.ModuleName, delAddr, senderCoins)
-	s.Require().NoError(err)
-
-	// log the sender's coins
-	senderCoins1 := providerBankKeeper.GetAllBalances(s.providerCtx(), delAddr)
-
-	// Register the consumer reward denom
-	err = s.providerApp.GetProviderKeeper().RegisterConsumerRewardDenom(s.providerCtx(), rewardCoins[ibcCoinIndex].Denom, delAddr)
-	s.Require().NoError(err)
-
-	// Check that the delAddr has the right amount less money in it after paying the fee
-	senderCoins2 := providerBankKeeper.GetAllBalances(s.providerCtx(), delAddr)
-	consumerRewardDenomRegistrationFee := s.providerApp.GetProviderKeeper().GetConsumerRewardDenomRegistrationFee(s.providerCtx())
-	s.Require().Equal(senderCoins1.Sub(senderCoins2...), sdk.NewCoins(consumerRewardDenomRegistrationFee))
+	// Set the consumer reward denom. This would be done by a governance proposal in prod
+	s.providerApp.GetProviderKeeper().SetConsumerRewardDenom(s.providerCtx(), rewardCoins[ibcCoinIndex].Denom)
 
 	s.providerChain.NextBlock()
 

x/ccv/provider/client/cli/tx.go

@@ -2,15 +2,13 @@ package cli
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/spf13/cobra"
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/client/tx"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/cosmos/cosmos-sdk/version"
 
 	"github.com/cosmos/interchain-security/v3/x/ccv/provider/types"
 )
@@ -26,7 +24,6 @@ func GetTxCmd() *cobra.Command {
 	}
 
 	cmd.AddCommand(NewAssignConsumerKeyCmd())
-	cmd.AddCommand(NewRegisterConsumerRewardDenomCmd())
 
 	return cmd
 }
@@ -68,37 +65,3 @@ func NewAssignConsumerKeyCmd() *cobra.Command {
 
 	return cmd
 }
-
-func NewRegisterConsumerRewardDenomCmd() *cobra.Command {
-	cmd := &cobra.Command{
-		Use:   "register-consumer-reward-denom [denom]",
-		Args:  cobra.ExactArgs(1),
-		Short: "Registers a denom that can be sent from consumer chains to all validators and delegators as a reward",
-		Long: strings.TrimSpace(
-			fmt.Sprintf(`Registers a denom that can be sent from consumer chains to all validators and delegators as a reward.
-
-Costs a fee, which is specified in genesis.json under the "consumer_reward_denom_fee" key. Will fail if the sending account has an insufficient balance.
-
-Example:
-$ %s tx provider register-consumer-reward-denom untrn --from mykey
-`,
-				version.AppName,
-			),
-		),
-		RunE: func(cmd *cobra.Command, args []string) error {
-			clientCtx, err := client.GetClientTxContext(cmd)
-			if err != nil {
-				return err
-			}
-			depositorAddr := clientCtx.GetFromAddress()
-
-			msg := types.NewMsgRegisterConsumerRewardDenom(args[0], depositorAddr)
-
-			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
-		},
-	}
-
-	flags.AddTxFlagsToCmd(cmd)
-
-	return cmd
-}