chore(revenue): Deprecate x/params usage in x/revenue (#1129)

* (refactor): Added migration to v2 and deprecation of x/params logic across the revenue module

* added CHANGELOG entry

* typo fix

* Apply suggestions from code review

Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>

* (fix): typo fix in import

* (fix): organize imports

* (fix): remove HTTP endpoint exposure

* Apply suggestions from code review

Co-authored-by: Tomas Guerra <54514587+GAtom22@users.noreply.github.com>

* (fix): ran formatter

* fix: applied changes from code review

* tests: Added unit test for UpdateParams message

Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>
Co-authored-by: Tomas Guerra <54514587+GAtom22@users.noreply.github.com>

CHANGELOG.md

@@ -39,7 +39,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### State Machine Breaking
 
+
+- (revenue) [#1129](https://github.com/evmos/evmos/pull/1129) Deprecate usage of x/params in x/revenue
 - (deps) [\#1157](https://github.com/evmos/evmos/pull/1157) Bump Ethermint version to [`v0.20.0-rc4`](https://github.com/evmos/ethermint/releases/tag/v0.20.0-rc4)
+
 - (ante) [#1054](https://github.com/evmos/evmos/pull/1054) Remove validator commission `AnteHandler` decorator and replace it with the new `MinCommissionRate` staking parameter.
 - (deps) [\#1041](https://github.com/evmos/evmos/pull/1041) Add ics23 dragonberry replace in go.mod as mentioned in the [Cosmos SDK release](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.4)
 - (feat) [\#1070](https://github.com/evmos/evmos/pull/1070) Add amino support to the vesting module, it enables signing the module messages using EIP-712.

app/app.go

@@ -492,7 +492,7 @@ func NewEvmos(
 	)
 
 	app.RevenueKeeper = revenuekeeper.NewKeeper(
-		keys[revenuetypes.StoreKey], appCodec, app.GetSubspace(revenuetypes.ModuleName),
+		keys[revenuetypes.StoreKey], appCodec, authtypes.NewModuleAddress(govtypes.ModuleName),
 		app.BankKeeper, app.EvmKeeper,
 		authtypes.FeeCollectorName,
 	)
@@ -625,7 +625,7 @@ func NewEvmos(
 		claims.NewAppModule(appCodec, *app.ClaimsKeeper),
 		vesting.NewAppModule(app.VestingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
 		recovery.NewAppModule(*app.RecoveryKeeper),
-		revenue.NewAppModule(app.RevenueKeeper, app.AccountKeeper),
+		revenue.NewAppModule(app.RevenueKeeper, app.AccountKeeper, app.GetSubspace(revenuetypes.ModuleName)),
 	)
 
 	// During begin block slashing happens after distr.BeginBlocker so that

proto/evmos/revenue/v1/tx.proto

@@ -1,6 +1,9 @@
 syntax = "proto3";
 package evmos.revenue.v1;
 
+import "cosmos/msg/v1/msg.proto";
+import "cosmos_proto/cosmos.proto";
+import "evmos/revenue/v1/genesis.proto";
 import "gogoproto/gogo.proto";
 import "google/api/annotations.proto";
 
@@ -21,6 +24,9 @@ service Msg {
   rpc CancelRevenue(MsgCancelRevenue) returns (MsgCancelRevenueResponse) {
     option (google.api.http).post = "/evmos/revenue/v1/tx/cancel_revenue";
   };
+  // UpdateParams defined a governance operation for updating the x/revenue module parameters.
+  // The authority is hard-coded to the Cosmos SDK x/gov module account
+  rpc UpdateParams(MsgUpdateParams) returns (MsgUpdateParamsResponse);
 }
 
 // MsgRegisterRevenue defines a message that registers a Revenue
@@ -70,3 +76,17 @@ message MsgCancelRevenue {
 
 // MsgCancelRevenueResponse defines the MsgCancelRevenue response type
 message MsgCancelRevenueResponse {}
+
+// MsgUpdateParams defines a Msg for updating the x/revenue module parameters.
+message MsgUpdateParams {
+  option (cosmos.msg.v1.signer) = "authority";
+  // authority is the address of the governance account.
+  string authority = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  // params defines the x/revenue parameters to update.
+  // NOTE: All parameters must be supplied.
+  Params params = 2 [(gogoproto.nullable) = false];
+}
+
+// MsgUpdateParamsResponse defines the response structure for executing a
+// MsgUpdateParams message.
+message MsgUpdateParamsResponse {}

x/revenue/genesis.go

@@ -1,6 +1,7 @@
 package revenue
 
 import (
+	errorsmod "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/evmos/evmos/v10/x/revenue/keeper"
@@ -13,7 +14,10 @@ func InitGenesis(
 	k keeper.Keeper,
 	data types.GenesisState,
 ) {
-	k.SetParams(ctx, data.Params)
+	err := k.SetParams(ctx, data.Params)
+	if err != nil {
+		panic(errorsmod.Wrapf(err, "failed setting params"))
+	}
 
 	for _, revenue := range data.Revenues {
 		contract := revenue.GetContractAddr()

x/revenue/handler.go

@@ -23,6 +23,10 @@ func NewHandler(server types.MsgServer) sdk.Handler {
 		case *types.MsgCancelRevenue:
 			res, err := server.CancelRevenue(sdk.WrapSDKContext(ctx), msg)
 			return sdk.WrapServiceResult(ctx, res, err)
+		case *types.MsgUpdateParams:
+			res, err := server.UpdateParams(sdk.WrapSDKContext(ctx), msg)
+			return sdk.WrapServiceResult(ctx, res, err)
+
 		default:
 			return nil, errorsmod.Wrapf(errortypes.ErrUnknownRequest, "unrecognized %s message type: %T", types.ModuleName, msg)
 		}

x/revenue/keeper/keeper.go

@@ -6,7 +6,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/codec"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
 	"github.com/tendermint/tendermint/libs/log"
 
 	"github.com/evmos/evmos/v10/x/revenue/types"
@@ -15,10 +14,10 @@ import (
 // Keeper of this module maintains collections of revenues for contracts
 // registered to receive transaction fees.
 type Keeper struct {
-	storeKey   storetypes.StoreKey
-	cdc        codec.BinaryCodec
-	paramstore paramtypes.Subspace
-
+	storeKey storetypes.StoreKey
+	cdc      codec.BinaryCodec
+	// the address capable of executing a MsgUpdateParams message. Typically, this should be the x/gov module account.
+	authority        sdk.AccAddress
 	bankKeeper       types.BankKeeper
 	evmKeeper        types.EVMKeeper
 	feeCollectorName string
@@ -28,20 +27,15 @@ type Keeper struct {
 func NewKeeper(
 	storeKey storetypes.StoreKey,
 	cdc codec.BinaryCodec,
-	ps paramtypes.Subspace,
+	authority sdk.AccAddress,
 	bk types.BankKeeper,
 	evmKeeper types.EVMKeeper,
 	feeCollector string,
 ) Keeper {
-	// set KeyTable if it has not already been set
-	if !ps.HasKeyTable() {
-		ps = ps.WithKeyTable(types.ParamKeyTable())
-	}
-
 	return Keeper{
 		storeKey:         storeKey,
 		cdc:              cdc,
-		paramstore:       ps,
+		authority:        authority,
 		bankKeeper:       bk,
 		evmKeeper:        evmKeeper,
 		feeCollectorName: feeCollector,

x/revenue/keeper/migrations.go

@@ -0,0 +1,26 @@
+package keeper
+
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	v2 "github.com/evmos/evmos/v10/x/revenue/migrations/v2"
+	"github.com/evmos/evmos/v10/x/revenue/types"
+)
+
+// Migrator is a struct for handling in-place store migrations.
+type Migrator struct {
+	keeper         Keeper
+	legacySubspace types.Subspace
+}
+
+// NewMigrator returns a new Migrator.
+func NewMigrator(keeper Keeper, legacySubspace types.Subspace) Migrator {
+	return Migrator{
+		keeper:         keeper,
+		legacySubspace: legacySubspace,
+	}
+}
+
+// Migrate1to2 migrates the store from consensus version 1 to 2
+func (m Migrator) Migrate1to2(ctx sdk.Context) error {
+	return v2.MigrateStore(ctx, m.keeper.storeKey, m.legacySubspace, m.keeper.cdc)
+}

x/revenue/keeper/msg_server.go

@@ -6,6 +6,7 @@ import (
 	errorsmod "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	errortypes "github.com/cosmos/cosmos-sdk/types/errors"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/crypto"
@@ -260,3 +261,20 @@ func (k Keeper) CancelRevenue(
 
 	return &types.MsgCancelRevenueResponse{}, nil
 }
+
+// UpdateParams implements the gRPC MsgServer interface. When an UpdateParams
+// proposal passes, it updates the module parameters. The update can only be
+// performed if the requested authority is the Cosmos SDK governance module
+// account.
+func (k *Keeper) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
+	if k.authority.String() != req.Authority {
+		return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority.String(), req.Authority)
+	}
+
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	if err := k.SetParams(ctx, req.Params); err != nil {
+		return nil, err
+	}
+
+	return &types.MsgUpdateParamsResponse{}, nil
+}

x/revenue/keeper/msg_server_test.go

@@ -2,6 +2,8 @@ package keeper_test
 
 import (
 	"fmt"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 	"math/big"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -630,3 +632,37 @@ func (suite *KeeperTestSuite) TestCancelRevenue() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestUpdateParams() {
+	testCases := []struct {
+		name      string
+		request   *types.MsgUpdateParams
+		expectErr bool
+	}{
+		{
+			name:      "fail - invalid authority",
+			request:   &types.MsgUpdateParams{Authority: "foobar"},
+			expectErr: true,
+		},
+		{
+			name: "pass - valid Update msg",
+			request: &types.MsgUpdateParams{
+				Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(),
+				Params:    types.DefaultParams(),
+			},
+			expectErr: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.Run("MsgUpdateParams", func() {
+			_, err := suite.app.RevenueKeeper.UpdateParams(suite.ctx, tc.request)
+			if tc.expectErr {
+				suite.Require().Error(err)
+			} else {
+				suite.Require().NoError(err)
+			}
+		})
+	}
+}

x/revenue/keeper/params.go

@@ -6,13 +6,27 @@ import (
 	"github.com/evmos/evmos/v10/x/revenue/types"
 )
 
-// GetParams returns the total set of fees parameters.
+// GetParams returns the total set of revenue parameters.
 func (k Keeper) GetParams(ctx sdk.Context) (params types.Params) {
-	k.paramstore.GetParamSetIfExists(ctx, &params)
+	store := ctx.KVStore(k.storeKey)
+	bz := store.Get(types.ParamsKey)
+	if bz == nil {
+		return params
+	}
+
+	k.cdc.MustUnmarshal(bz, &params)
 	return params
 }
 
-// SetParams sets the fees parameters to the param space.
-func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
-	k.paramstore.SetParamSet(ctx, &params)
+// SetParams sets the revenue params in a single key
+func (k Keeper) SetParams(ctx sdk.Context, params types.Params) error {
+	store := ctx.KVStore(k.storeKey)
+	bz, err := k.cdc.Marshal(&params)
+	if err != nil {
+		return err
+	}
+
+	store.Set(types.ParamsKey, bz)
+
+	return nil
 }

x/revenue/migrations/v2/migrate.go

@@ -0,0 +1,37 @@
+package v2
+
+import (
+	"github.com/cosmos/cosmos-sdk/codec"
+	storetypes "github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	v2types "github.com/evmos/evmos/v10/x/revenue/migrations/v2/types"
+	"github.com/evmos/evmos/v10/x/revenue/types"
+)
+
+// MigrateStore migrates the x/revenue module state from the consensus version 1 to
+// version 2. Specifically, it takes the parameters that are currently stored
+// and managed by the Cosmos SDK params module and stores them directly into the x/evm module state.
+func MigrateStore(
+	ctx sdk.Context,
+	storeKey storetypes.StoreKey,
+	legacySubspace types.Subspace,
+	cdc codec.BinaryCodec,
+) error {
+	store := ctx.KVStore(storeKey)
+	var params v2types.Params
+
+	legacySubspace.GetParamSetIfExists(ctx, &params)
+
+	if err := params.Validate(); err != nil {
+		return err
+	}
+
+	bz, err := cdc.Marshal(&params)
+	if err != nil {
+		return err
+	}
+
+	store.Set(types.ParamsKey, bz)
+
+	return nil
+}

x/revenue/migrations/v2/migrate_test.go

@@ -0,0 +1,46 @@
+package v2_test
+
+import (
+	"testing"
+
+	"github.com/evmos/ethermint/encoding"
+	v2 "github.com/evmos/evmos/v10/x/revenue/migrations/v2"
+	v2types "github.com/evmos/evmos/v10/x/revenue/migrations/v2/types"
+	"github.com/evmos/evmos/v10/x/revenue/types"
+
+	"github.com/cosmos/cosmos-sdk/testutil"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/evmos/ethermint/app"
+	"github.com/stretchr/testify/require"
+)
+
+type mockSubspace struct {
+	ps v2types.Params
+}
+
+func newMockSubspace(ps v2types.Params) mockSubspace {
+	return mockSubspace{ps: ps}
+}
+
+func (ms mockSubspace) GetParamSetIfExists(ctx sdk.Context, ps types.LegacyParams) {
+	*ps.(*v2types.Params) = ms.ps
+}
+
+func TestMigrate(t *testing.T) {
+	encCfg := encoding.MakeConfig(app.ModuleBasics)
+	cdc := encCfg.Codec
+
+	storeKey := sdk.NewKVStoreKey(types.ModuleName)
+	tKey := sdk.NewTransientStoreKey("transient_test")
+	ctx := testutil.DefaultContext(storeKey, tKey)
+	kvStore := ctx.KVStore(storeKey)
+
+	legacySubspace := newMockSubspace(v2types.DefaultParams())
+	require.NoError(t, v2.MigrateStore(ctx, storeKey, legacySubspace, cdc))
+
+	paramsBz := kvStore.Get(v2types.ParamsKey)
+	var params v2types.Params
+	cdc.MustUnmarshal(paramsBz, &params)
+
+	require.Equal(t, params, legacySubspace.ps)
+}