feat(transfer)!: migrate transfer parameters to be self managed (#3553)

* added 'UpdateParams' rpc endpoint and its messages to 'tx.proto' * ran 'make proto-gen' * implemented 'sdk.Msg' for 'MsgUpdateParams' in 'types/msgs.go' * 'msgs_test.go' updated - todo test tests + create a 'NewMsgUpdateParams' function and update tests * added 'NewMsgUpdateParams' function to 'types/msgs.go' * improved test styling and used helper functions more * added boilerplate 'UpdateParams' function to MsgServer - todo implement the custom logic * removed paramtypes and added authority to Keeper struct - todo Keeper functions need to be updated * updated Keeper functions - todo add ParamsKey to types + update keeper tests * deleted unneeded keeper/params.go as this should be managed in keeper.go * moved params test from params_test to keeper_test as this is where they should be managed now * added ParamsKey to 'types/keys.go' * replaced 'k.GetSendEnabled(ctx)' with 'k.GetParams(ctx).SendEnabled' in msg_server * replaced 'k.GetReceiveEnabled(ctx)' with 'k.GetParams(ctx).ReceiveEnabled' in 'relay.go' * added 'MsgUpdateParams' to InterfaceRegistry * implemented 'UpdateParams' in msg_server * added one more test case to msgs_test for ParamsValidation * wrote tests for 'TestUpdateParams' rpc handler * removed params subspace - todo: add subspaces back as legacy for migrations * removed test cases that didn't make sense * fixed the tests here * fixed typo in function comment * fixed keeper tests * ran 'golangci-lint' on 'transfer/types/...' * added validation to SetParams keeper method * handled the case where 'SetParams' returns an error 'InitGenesis' * fix: forgot to check value of '.SetParams' to ensure it is not an error * ran 'gofumpt' on 'transfer/keeper/' * split 'types/params.go' into a legacy part * created 'exported.go' for migration purposes * added migration code to 'keeper/migrations.go' * increased module.go version and registered param migration * passed legacySubspace to transfer for migration * allow tests to compile and pass by passing the subspace to the migrator - todo: add param migration tests * removed unneeded if statement * ran gofumpt * added integration tests for successful migration of params * fix: forgot to check if '.SetParams' returns an err * ran gofumpt * broken: this test is work in progress and will fail * uncommented the two lines in test * fixed an important error on params_legacy.go * initialize ParamKeyTable for ibctransfer in app.go now * finally a working version of the test * style changes on the test * ran 'gofumpt' * moved comment * updated GetParams function's comment * updated docs * e2e: changed x/params query to fallback behavior * e2e: changed x/params query to fallback behavior * e2e: dropped the fallback behavior * e2e: trying to use the new gov proposal v1 to change params * e2e: created 'isSelfManagingParams' function * e2e: created fallback behaviour for param update gov proposal * chore: e2e code cleanup * chore: ran 'gofumpt' in e2e * style(transfer/test): moved success case to top Co-authored-by: Damian Nolan <[email protected]> * style(transfer/test): replaced request with msg * refactor(transfer): moved MsgUpdateParams' registry to Interface Registry * style(transfer/test): ran gofumpt * style(transfer): moved UpdateParams function to the bottom of the file * imp(transfer): using 'ibcerrors.ErrInvalidAddress' instead of 'govtypes.ErrInvalidSigner' * refactor(transfer/test): used ibctesting constants instead of defining my own * feat(e2e/transfer): using semver to handle gov prop fallback instead of queries * style(e2e/transfer): renamed 'selfParamFeatureReleases' to 'transferSelfParamFeatureReleases' * feat(transfer): keeping paramSpace in keeper and removing exported to reduce mods to simapp * feat(simapp): reduced the modifications to app.go to only one * fix(transfer/test): updated migration tests * imp(transfer/test): improved the efficiency and style of the test * style(transfer): ran gofumpt * style(transfer): ParamsKey is now stored via a string rather than bytes * imp(transfer): retired the old validate function, and new one makes more sense * style(transfer): ran gofumpt * refactor(transfer): removed the validate function for params as it currently does nothing and reduces codecov * fix(transfer): refactored to not use validate * imp(transfer): GetParams now panics if params are not set * imp(transfer/test): added a new test case to test when Params are unset * style(transfer/test): updated the test messages * docs(transfer): added tracking issue for removing params_legacy.go * imp(transfer): added a new error type for invalid authority * style(e2e/transfer): fixed typo * style(transfer/test): stopped using field names in test case declaration * style(transfer/test): stopped using field names in test case declaration * style(transfer/test): renamed test case * imp(transfer): added more info to the error message * style(e2e/transfer): updated ordering of deps * imp(transfer/test): reduced the test size * style(transfer): changed the err message for 'ErrInvalidAuthority' * imp(transfer): removed ErrInvalidAuthority * style(transfer): using 'len(bz) == 0' instead of 'bz == nil' * style(transfer): removed unneeded comment * imp(transfer): switched back to using 'bz == nil' * revert: '7813b39d7835eec26a3655bcd77587198ae90bea' * revert: "style(transfer): changed the err message for 'ErrInvalidAuthority'" This reverts commit 7813b39. * revert: e5f3960 * revert: 4c163f0 * style(transfer): added code comment * docs(migrations): added migration docs for changes in app.go * imp(transfer): improved ValidateBasic err message * style(transfer/test): improved the styling of the test * style(transfer/test): improved the styling of the test * docs(transfer): added godoc for UpdateParams method * style(transfer): removed named return arg * imp(e2e/transfer): reduced code duplication * style(transfer/test): updated test case names * imp(transfer/test): added whitespace test case --------- Co-authored-by: Damian Nolan <[email protected]>
cosmos · May 24, 2023 · 55149ca · 55149ca
1 parent d2df733
commit 55149ca
Show file tree

Hide file tree

Showing 24 changed files with 900 additions and 210 deletions.
diff --git a/docs/apps/transfer/params.md b/docs/apps/transfer/params.md
@@ -6,20 +6,26 @@ order: 7
 
 The IBC transfer application module contains the following parameters:
 
-| Key              | Type | Default Value |
-|------------------|------|---------------|
+| Name             | Type | Default Value |
+| ---------------- | ---- | ------------- |
 | `SendEnabled`    | bool | `true`        |
 | `ReceiveEnabled` | bool | `true`        |
 
+The IBC transfer module stores its parameters in its keeper with the prefix of `0x03`.
+
 ## `SendEnabled`
 
-The transfers enabled parameter controls send cross-chain transfer capabilities for all fungible tokens.
+The `SendEnabled` parameter controls send cross-chain transfer capabilities for all fungible tokens.
 
 To prevent a single token from being transferred from the chain, set the `SendEnabled` parameter to `true` and then, depending on the Cosmos SDK version, do one of the following:
 
 - For Cosmos SDK v0.46.x or earlier, set the bank module's [`SendEnabled` parameter](https://github.com/cosmos/cosmos-sdk/blob/release/v0.46.x/x/bank/spec/05_params.md#sendenabled) for the denomination to `false`.
 - For Cosmos SDK versions above v0.46.x, set the bank module's `SendEnabled` entry for the denomination to `false` using `MsgSetSendEnabled` as a governance proposal.
 
+::: warning
+Doing so will prevent the token from being transferred between any accounts in the blockchain.
+:::
+
 ## `ReceiveEnabled`
 
 The transfers enabled parameter controls receive cross-chain transfer capabilities for all fungible tokens.
@@ -28,3 +34,57 @@ To prevent a single token from being transferred to the chain, set the `ReceiveE
 
 - For Cosmos SDK v0.46.x or earlier, set the bank module's [`SendEnabled` parameter](https://github.com/cosmos/cosmos-sdk/blob/release/v0.46.x/x/bank/spec/05_params.md#sendenabled) for the denomination to `false`.
 - For Cosmos SDK versions above v0.46.x, set the bank module's `SendEnabled` entry for the denomination to `false` using `MsgSetSendEnabled` as a governance proposal.
+
+::: warning
+Doing so will prevent the token from being transferred between any accounts in the blockchain.
+:::
+
+## Queries
+
+Current parameter values can be queried via a query message.
+
+<!-- Turn it into a github code snippet in docusaurus: -->
+
+```protobuf
+// proto/ibc/applications/transfer/v1/query.proto
+
+// QueryParamsRequest is the request type for the Query/Params RPC method.
+message QueryParamsRequest {}
+
+// QueryParamsResponse is the response type for the Query/Params RPC method.
+message QueryParamsResponse {
+  // params defines the parameters of the module.
+  Params params = 1;
+}
+```
+
+To execute the query in `simd`, you use the following command:
+
+```bash
+simd query ibc-transfer params
+```
+
+## Changing Parameters
+
+To change the parameter values, you must make a governance proposal that executes the `MsgUpdateParams` message.
+
+<!-- Turn it into a github code snippet in docusaurus: -->
+
+```protobuf
+// proto/ibc/applications/transfer/v1/tx.proto
+
+// MsgUpdateParams is the Msg/UpdateParams request type.
+message MsgUpdateParams {
+  // authority is the address that controls the module (defaults to x/gov unless overwritten).
+  string authority = 1;
+
+  // params defines the transfer 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 {}
+```
diff --git a/docs/migrations/v7-to-v8.md b/docs/migrations/v7-to-v8.md
@@ -5,10 +5,10 @@ This guide provides instructions for migrating to version `v8.0.0` of ibc-go.
 There are four sections based on the four potential user groups of this document:
 
 - [Migrating from v7 to v8](#migrating-from-v7-to-v8)
-  - [Chains](#chains)
-  - [IBC Apps](#ibc-apps)
-  - [Relayers](#relayers)
-  - [IBC Light Clients](#ibc-light-clients)
+	- [Chains](#chains)
+	- [IBC Apps](#ibc-apps)
+	- [Relayers](#relayers)
+	- [IBC Light Clients](#ibc-light-clients)
 
 **Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated on major version releases.
 
@@ -31,6 +31,22 @@ TODO: https://github.com/cosmos/ibc-go/pull/3505 (extra parameter added to trans
 	)
 ```
 
+- You should pass the `authority` to the ibctransfer keeper. ([#3520](https://github.com/cosmos/ibc-go/pull/3520)) See [diff](https://github.com/cosmos/ibc-go/pull/3520/files#diff-d18972debee5e64f16e40807b2ae112ddbe609504a93ea5e1c80a5d489c3a08a). ([#3553](https://github.com/cosmos/ibc-go/pull/3553))
+
+```diff
+// app.go
+
+	// Create Transfer Keeper and pass IBCFeeKeeper as expected Channel and PortKeeper
+	// since fee middleware will wrap the IBCKeeper for underlying application.
+	app.TransferKeeper = ibctransferkeeper.NewKeeper(
+		appCodec, keys[ibctransfertypes.StoreKey], app.GetSubspace(ibctransfertypes.ModuleName),
+		app.IBCFeeKeeper, // ISC4 Wrapper: fee IBC middleware
+		app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
+		app.AccountKeeper, app.BankKeeper, scopedTransferKeeper,
++		authtypes.NewModuleAddress(govtypes.ModuleName).String(),
+	)
+```
+
 ## IBC Apps
 
 TODO: https://github.com/cosmos/ibc-go/pull/3303

diff --git a/e2e/tests/transfer/base_test.go b/e2e/tests/transfer/base_test.go
@@ -2,13 +2,14 @@ package transfer
 
 import (
 	"context"
-	"strconv"
 	"testing"
 	"time"
 
 	"cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 	paramsproposaltypes "github.com/cosmos/cosmos-sdk/x/params/types/proposal"
+	transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
 
 	"github.com/strangelove-ventures/interchaintest/v7/ibc"
 	test "github.com/strangelove-ventures/interchaintest/v7/testutil"
@@ -17,7 +18,6 @@ import (
 	"github.com/cosmos/ibc-go/e2e/semverutil"
 	"github.com/cosmos/ibc-go/e2e/testsuite"
 	"github.com/cosmos/ibc-go/e2e/testvalues"
-	transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 )
 
@@ -29,34 +29,17 @@ type TransferTestSuite struct {
 	testsuite.E2ETestSuite
 }
 
-// QueryTransferSendEnabledParam queries the on-chain send enabled param for the transfer module
-func (s *TransferTestSuite) QueryTransferSendEnabledParam(ctx context.Context, chain ibc.Chain) bool {
-	queryClient := s.GetChainGRCPClients(chain).ParamsQueryClient
-	res, err := queryClient.Params(ctx, &paramsproposaltypes.QueryParamsRequest{
-		Subspace: transfertypes.StoreKey,
-		Key:      string(transfertypes.KeySendEnabled),
-	})
-	s.Require().NoError(err)
-
-	enabled, err := strconv.ParseBool(res.Param.Value)
-	s.Require().NoError(err)
-
-	return enabled
+// transferSelfParamsFeatureReleases represents the releases the transfer module started managing its own params.
+var transferSelfParamsFeatureReleases = semverutil.FeatureReleases{
+	MajorVersion: "v8",
 }
 
-// QueryTransferReceiveEnabledParam queries the on-chain receive enabled param for the transfer module
-func (s *TransferTestSuite) QueryTransferReceiveEnabledParam(ctx context.Context, chain ibc.Chain) bool {
-	queryClient := s.GetChainGRCPClients(chain).ParamsQueryClient
-	res, err := queryClient.Params(ctx, &paramsproposaltypes.QueryParamsRequest{
-		Subspace: transfertypes.StoreKey,
-		Key:      string(transfertypes.KeyReceiveEnabled),
-	})
-	s.Require().NoError(err)
-
-	enabled, err := strconv.ParseBool(res.Param.Value)
+// QueryTransferSendEnabledParam queries the on-chain send enabled param for the transfer module
+func (s *TransferTestSuite) QueryTransferParams(ctx context.Context, chain ibc.Chain) transfertypes.Params {
+	queryClient := s.GetChainGRCPClients(chain).TransferQueryClient
+	res, err := queryClient.Params(ctx, &transfertypes.QueryParamsRequest{})
 	s.Require().NoError(err)
-
-	return enabled
+	return *res.Params
 }
 
 // TestMsgTransfer_Succeeds_Nonincentivized will test sending successful IBC transfers from chainA to chainB.
@@ -258,10 +241,17 @@ func (s *TransferTestSuite) TestSendEnabledParam() {
 	chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)
 	chainBAddress := chainBWallet.FormattedAddress()
 
+	chainAVersion := chainA.Config().Images[0].Version
+	isSelfManagingParams := transferSelfParamsFeatureReleases.IsSupported(chainAVersion)
+
+	govModuleAddress, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chainA)
+	s.Require().NoError(err)
+	s.Require().NotNil(govModuleAddress)
+
 	s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks")
 
 	t.Run("ensure transfer sending is enabled", func(t *testing.T) {
-		enabled := s.QueryTransferSendEnabledParam(ctx, chainA)
+		enabled := s.QueryTransferParams(ctx, chainA).SendEnabled
 		s.Require().True(enabled)
 	})
 
@@ -271,16 +261,21 @@ func (s *TransferTestSuite) TestSendEnabledParam() {
 	})
 
 	t.Run("change send enabled parameter to disabled", func(t *testing.T) {
-		changes := []paramsproposaltypes.ParamChange{
-			paramsproposaltypes.NewParamChange(transfertypes.StoreKey, string(transfertypes.KeySendEnabled), "false"),
-		}
+		if isSelfManagingParams {
+			msg := transfertypes.NewMsgUpdateParams(govModuleAddress.String(), transfertypes.NewParams(false, true))
+			s.ExecuteGovProposalV1(ctx, msg, chainA, chainAWallet, 1)
+		} else {
+			changes := []paramsproposaltypes.ParamChange{
+				paramsproposaltypes.NewParamChange(transfertypes.StoreKey, string(transfertypes.KeySendEnabled), "false"),
+			}
 
-		proposal := paramsproposaltypes.NewParameterChangeProposal(ibctesting.Title, ibctesting.Description, changes)
-		s.ExecuteGovProposal(ctx, chainA, chainAWallet, proposal)
+			proposal := paramsproposaltypes.NewParameterChangeProposal(ibctesting.Title, ibctesting.Description, changes)
+			s.ExecuteGovProposal(ctx, chainA, chainAWallet, proposal)
+		}
 	})
 
 	t.Run("ensure transfer params are disabled", func(t *testing.T) {
-		enabled := s.QueryTransferSendEnabledParam(ctx, chainA)
+		enabled := s.QueryTransferParams(ctx, chainA).SendEnabled
 		s.Require().False(enabled)
 	})
 
@@ -309,10 +304,17 @@ func (s *TransferTestSuite) TestReceiveEnabledParam() {
 		chainBAddress = chainBWallet.FormattedAddress()
 	)
 
+	chainAVersion := chainA.Config().Images[0].Version
+	isSelfManagingParams := transferSelfParamsFeatureReleases.IsSupported(chainAVersion)
+
+	govModuleAddress, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chainA)
+	s.Require().NoError(err)
+	s.Require().NotNil(govModuleAddress)
+
 	s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks")
 
 	t.Run("ensure transfer receive is enabled", func(t *testing.T) {
-		enabled := s.QueryTransferReceiveEnabledParam(ctx, chainA)
+		enabled := s.QueryTransferParams(ctx, chainA).ReceiveEnabled
 		s.Require().True(enabled)
 	})
 
@@ -350,16 +352,21 @@ func (s *TransferTestSuite) TestReceiveEnabledParam() {
 	})
 
 	t.Run("change receive enabled parameter to disabled ", func(t *testing.T) {
-		changes := []paramsproposaltypes.ParamChange{
-			paramsproposaltypes.NewParamChange(transfertypes.StoreKey, string(transfertypes.KeyReceiveEnabled), "false"),
-		}
+		if isSelfManagingParams {
+			msg := transfertypes.NewMsgUpdateParams(govModuleAddress.String(), transfertypes.NewParams(false, false))
+			s.ExecuteGovProposalV1(ctx, msg, chainA, chainAWallet, 1)
+		} else {
+			changes := []paramsproposaltypes.ParamChange{
+				paramsproposaltypes.NewParamChange(transfertypes.StoreKey, string(transfertypes.KeyReceiveEnabled), "false"),
+			}
 
-		proposal := paramsproposaltypes.NewParameterChangeProposal(ibctesting.Title, ibctesting.Description, changes)
-		s.ExecuteGovProposal(ctx, chainA, chainAWallet, proposal)
+			proposal := paramsproposaltypes.NewParameterChangeProposal(ibctesting.Title, ibctesting.Description, changes)
+			s.ExecuteGovProposal(ctx, chainA, chainAWallet, proposal)
+		}
 	})
 
 	t.Run("ensure transfer params are disabled", func(t *testing.T) {
-		enabled := s.QueryTransferReceiveEnabledParam(ctx, chainA)
+		enabled := s.QueryTransferParams(ctx, chainA).ReceiveEnabled
 		s.Require().False(enabled)
 	})