Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(statemachine)!: Add allow all client wildcard to AllowedClients param #5429

Merged
merged 34 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
26d5bfb
add wildcard allow all client
sontrinh16 Dec 16, 2023
58a5d82
minor
sontrinh16 Dec 17, 2023
67836b7
fix lint
sontrinh16 Dec 17, 2023
9a94db1
minor and doc update
sontrinh16 Dec 17, 2023
ff770e3
Merge branch 'main' into add_wildcard_allow_all_client
GNaD13 Dec 18, 2023
feaf533
Merge branch 'main' into add_wildcard_allow_all_client
lichdu29 Dec 18, 2023
d087f50
Merge branch 'main' into add_wildcard_allow_all_client
sontrinh16 Dec 19, 2023
fb1920e
Merge branch 'main' into add_wildcard_allow_all_client
crodriguezvega Dec 19, 2023
90a252e
review comments
crodriguezvega Dec 19, 2023
2600e80
Merge branch 'main' into add_wildcard_allow_all_client
sontrinh16 Dec 19, 2023
d832839
Merge branch 'main' into add_wildcard_allow_all_client
crodriguezvega Dec 19, 2023
af912b6
if allow all clients is present, no other client types should be in t…
crodriguezvega Dec 19, 2023
d224be2
clean up code to allow wasm client type
crodriguezvega Dec 19, 2023
d98ed8b
remove unused variable
crodriguezvega Dec 19, 2023
482ca8e
Fix build failures, tweak err message.
DimitrisJim Dec 20, 2023
67d6ccd
Merge branch 'main' into add_wildcard_allow_all_client
DimitrisJim Dec 20, 2023
861c9b6
Merge branch 'main' into add_wildcard_allow_all_client
crodriguezvega Dec 20, 2023
8be427e
Merge branch 'main' into add_wildcard_allow_all_client
crodriguezvega Dec 20, 2023
9625eb5
Merge branch 'main' into add_wildcard_allow_all_client
crodriguezvega Dec 20, 2023
e736817
modify allow clients list in genesis with feature releases
crodriguezvega Dec 20, 2023
a1c0868
Merge branch 'main' into add_wildcard_allow_all_client
damiannolan Dec 21, 2023
a4728cd
Merge branch 'main' into add_wildcard_allow_all_client
hoangdv2429 Dec 21, 2023
da722e9
Merge branch 'main' into add_wildcard_allow_all_client
sontrinh16 Jan 2, 2024
079ef2b
Merge branch 'main' into add_wildcard_allow_all_client
DimitrisJim Jan 9, 2024
8d83328
Merge branch 'main' into add_wildcard_allow_all_client
damiannolan Jan 9, 2024
a9ddbf1
chore: adding v8.1 to minor versions in e2e feat releases struct
damiannolan Jan 9, 2024
955111c
Merge branch 'main' into add_wildcard_allow_all_client
DimitrisJim Jan 9, 2024
1c4cb91
Merge branch 'main' into add_wildcard_allow_all_client
sontrinh16 Jan 10, 2024
0563919
Merge branch 'main' into add_wildcard_allow_all_client
charleenfei Jan 12, 2024
89ce4a6
Merge branch 'main' into add_wildcard_allow_all_client
sontrinh16 Jan 12, 2024
7796dcf
update docs
crodriguezvega Jan 14, 2024
71323d4
chore: doc lint fixes
DimitrisJim Jan 15, 2024
a03702e
Merge branch 'main' into add_wildcard_allow_all_client
damiannolan Jan 15, 2024
93f07ea
Merge branch 'main' into add_wildcard_allow_all_client
crodriguezvega Jan 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/params/params.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ The 02-client submodule contains the following parameters:

### AllowedClients

The allowed clients parameter defines an allowlist of client types supported by the chain. A client
The allowed clients parameter defines an allow list of client types supported by the chain. A client
that is not registered on this list will fail upon creation or on genesis validation. Note that,
since the client type is an arbitrary string, chains they must not register two light clients which
return the same value for the `ClientType()` function, otherwise the allowlist check can be
bypassed.
since the client type is an arbitrary string, chains must not register two light clients which
return the same value for the `ClientType()` function, otherwise the allow list check can be
bypassed. If `AllowAllClients` wildcard (`*`) is set, then all client type are supported.
76 changes: 0 additions & 76 deletions e2e/tests/wasm/grandpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ import (

authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
paramsproposaltypes "github.com/cosmos/cosmos-sdk/x/params/types/proposal"

cmtjson "github.com/cometbft/cometbft/libs/json"

"github.com/cosmos/ibc-go/e2e/testsuite"
"github.com/cosmos/ibc-go/e2e/testvalues"
Expand Down Expand Up @@ -128,11 +125,6 @@ func (s *GrandpaTestSuite) TestMsgTransfer_Succeeds_GrandpaContract() {
err = r.GeneratePath(ctx, eRep, cosmosChain.Config().ChainID, polkadotChain.Config().ChainID, pathName)
s.Require().NoError(err)

t.Run("add 08-wasm to list of allowed clients", func(t *testing.T) {
allowedClients := s.queryAllowedClientsParam(ctx, cosmosChain)
s.allowWasmClients(ctx, cosmosChain, cosmosWallet, allowedClients)
})

// Create new clients
err = r.CreateClients(ctx, eRep, pathName, ibc.DefaultClientOpts())
s.Require().NoError(err)
Expand Down Expand Up @@ -285,11 +277,6 @@ func (s *GrandpaTestSuite) TestMsgTransfer_TimesOut_GrandpaContract() {
err = r.GeneratePath(ctx, eRep, cosmosChain.Config().ChainID, polkadotChain.Config().ChainID, pathName)
s.Require().NoError(err)

t.Run("add 08-wasm to list of allowed clients", func(t *testing.T) {
allowedClients := s.queryAllowedClientsParam(ctx, cosmosChain)
s.allowWasmClients(ctx, cosmosChain, cosmosWallet, allowedClients)
})

// Create new clients
err = r.CreateClients(ctx, eRep, pathName, ibc.DefaultClientOpts())
s.Require().NoError(err)
Expand Down Expand Up @@ -354,7 +341,6 @@ func (s *GrandpaTestSuite) TestMsgTransfer_TimesOut_GrandpaContract() {
// * Migrates the wasm client contract
func (s *GrandpaTestSuite) TestMsgMigrateContract_Success_GrandpaContract() {
ctx := context.Background()
t := s.T()

chainA, chainB := s.GetGrandpaTestChains()

Expand Down Expand Up @@ -392,11 +378,6 @@ func (s *GrandpaTestSuite) TestMsgMigrateContract_Success_GrandpaContract() {
err = r.GeneratePath(ctx, eRep, cosmosChain.Config().ChainID, polkadotChain.Config().ChainID, pathName)
s.Require().NoError(err)

t.Run("add 08-wasm to list of allowed clients", func(t *testing.T) {
allowedClients := s.queryAllowedClientsParam(ctx, cosmosChain)
s.allowWasmClients(ctx, cosmosChain, cosmosWallet, allowedClients)
})

// Create new clients
err = r.CreateClients(ctx, eRep, pathName, ibc.DefaultClientOpts())
s.Require().NoError(err)
Expand Down Expand Up @@ -447,7 +428,6 @@ func (s *GrandpaTestSuite) TestMsgMigrateContract_Success_GrandpaContract() {
// * Migrates the wasm client contract with a contract that will always fail migration
func (s *GrandpaTestSuite) TestMsgMigrateContract_ContractError_GrandpaContract() {
ctx := context.Background()
t := s.T()

chainA, chainB := s.GetGrandpaTestChains()

Expand Down Expand Up @@ -484,11 +464,6 @@ func (s *GrandpaTestSuite) TestMsgMigrateContract_ContractError_GrandpaContract(
err = r.GeneratePath(ctx, eRep, cosmosChain.Config().ChainID, polkadotChain.Config().ChainID, pathName)
s.Require().NoError(err)

t.Run("add 08-wasm to list of allowed clients", func(t *testing.T) {
allowedClients := s.queryAllowedClientsParam(ctx, cosmosChain)
s.allowWasmClients(ctx, cosmosChain, cosmosWallet, allowedClients)
})

// Create new clients
err = r.CreateClients(ctx, eRep, pathName, ibc.DefaultClientOpts())
s.Require().NoError(err)
Expand Down Expand Up @@ -541,7 +516,6 @@ func (s *GrandpaTestSuite) TestMsgMigrateContract_ContractError_GrandpaContract(
// This contract modifies the unbonding period to 1600s with the trusting period being calculated as (unbonding period / 3).
func (s *GrandpaTestSuite) TestRecoverClient_Succeeds_GrandpaContract() {
ctx := context.Background()
t := s.T()

// set the trusting period to a value which will still be valid upon client creation, but invalid before the first update
// the contract uses 1600s as the unbonding period with the trusting period evaluating to (unbonding period / 3)
Expand Down Expand Up @@ -585,11 +559,6 @@ func (s *GrandpaTestSuite) TestRecoverClient_Succeeds_GrandpaContract() {
err = r.GeneratePath(ctx, eRep, cosmosChain.Config().ChainID, polkadotChain.Config().ChainID, pathName)
s.Require().NoError(err)

t.Run("add 08-wasm to list of allowed clients", func(t *testing.T) {
allowedClients := s.queryAllowedClientsParam(ctx, cosmosChain)
s.allowWasmClients(ctx, cosmosChain, cosmosWallet, allowedClients)
})

// create client pair with subject (bad trusting period)
subjectClientID := clienttypes.FormatClientIdentifier(wasmtypes.Wasm, 0)
// TODO: The hyperspace relayer makes no use of create client opts
Expand Down Expand Up @@ -797,48 +766,3 @@ func (s *GrandpaTestSuite) GetGrandpaTestChains() (ibc.Chain, ibc.Chain) {
options.ChainBSpec.EncodingConfig = testsuite.SDKEncodingConfig()
})
}

// queryAllowedClientsParam queries the on-chain allowed clients param for 02-client
func (s *GrandpaTestSuite) queryAllowedClientsParam(ctx context.Context, chain ibc.Chain) []string {
if testvalues.SelfParamsFeatureReleases.IsSupported(chain.Config().Images[0].Version) {
queryClient := s.GetChainGRCPClients(chain).ClientQueryClient
res, err := queryClient.ClientParams(ctx, &clienttypes.QueryClientParamsRequest{})
s.Require().NoError(err)

return res.Params.AllowedClients
}
queryClient := s.GetChainGRCPClients(chain).ParamsQueryClient
res, err := queryClient.Params(ctx, &paramsproposaltypes.QueryParamsRequest{
Subspace: ibcexported.ModuleName,
Key: string(clienttypes.KeyAllowedClients),
})
s.Require().NoError(err)

var allowedClients []string
err = cmtjson.Unmarshal([]byte(res.Param.Value), &allowedClients)
s.Require().NoError(err)

return allowedClients
}

// allowWasmClients adds 08-wasm to the on-chain allowed clients param for 02-client
func (s *GrandpaTestSuite) allowWasmClients(ctx context.Context, chain ibc.Chain, wallet ibc.Wallet, allowedClients []string) {
govModuleAddress, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chain)
s.Require().NoError(err)
s.Require().NotNil(govModuleAddress)

allowedClients = append(allowedClients, wasmtypes.Wasm)
if testvalues.SelfParamsFeatureReleases.IsSupported(chain.Config().Images[0].Version) {
msg := clienttypes.NewMsgUpdateParams(govModuleAddress.String(), clienttypes.NewParams(allowedClients...))
s.ExecuteAndPassGovV1Proposal(ctx, msg, chain, wallet)
} else {
value, err := cmtjson.Marshal(allowedClients)
s.Require().NoError(err)
changes := []paramsproposaltypes.ParamChange{
paramsproposaltypes.NewParamChange(ibcexported.ModuleName, string(clienttypes.KeyAllowedClients), string(value)),
}

proposal := paramsproposaltypes.NewParameterChangeProposal(ibctesting.Title, ibctesting.Description, changes)
s.ExecuteAndPassGovV1Beta1Proposal(ctx, chain, wallet, proposal)
}
}
56 changes: 53 additions & 3 deletions e2e/testsuite/testconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ import (
"github.com/cosmos/ibc-go/e2e/relayer"
"github.com/cosmos/ibc-go/e2e/semverutil"
"github.com/cosmos/ibc-go/e2e/testvalues"
wasmtypes "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/types"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported"
ibctypes "github.com/cosmos/ibc-go/v8/modules/core/types"
)

const (
Expand Down Expand Up @@ -532,7 +536,7 @@ func getGenesisModificationFunction(cc ChainConfig) func(ibc.ChainConfig, []byte
return defaultGovv1ModifyGenesis(version)
}

return defaultGovv1Beta1ModifyGenesis()
return defaultGovv1Beta1ModifyGenesis(version)
}

// defaultGovv1ModifyGenesis will only modify governance params to ensure the voting period and minimum deposit
Expand All @@ -554,9 +558,16 @@ func defaultGovv1ModifyGenesis(version string) func(ibc.ChainConfig, []byte) ([]
if err != nil {
return nil, err
}

appState[govtypes.ModuleName] = govGenBz

if !testvalues.AllowAllClientsWildcardFeatureReleases.IsSupported(version) {
ibcGenBz, err := modifyClientGenesisAppState(chainConfig, appState[ibcexported.ModuleName])
if err != nil {
return nil, err
}
appState[ibcexported.ModuleName] = ibcGenBz
}

appGenesis.AppState, err = json.Marshal(appState)
if err != nil {
return nil, err
Expand All @@ -581,7 +592,7 @@ func defaultGovv1ModifyGenesis(version string) func(ibc.ChainConfig, []byte) ([]

// defaultGovv1Beta1ModifyGenesis will only modify governance params to ensure the voting period and minimum deposit
// // are functional for e2e testing purposes.
func defaultGovv1Beta1ModifyGenesis() func(ibc.ChainConfig, []byte) ([]byte, error) {
func defaultGovv1Beta1ModifyGenesis(version string) func(ibc.ChainConfig, []byte) ([]byte, error) {
const appStateKey = "app_state"
return func(chainConfig ibc.ChainConfig, genbz []byte) ([]byte, error) {
genesisDocMap := map[string]interface{}{}
Expand Down Expand Up @@ -611,6 +622,24 @@ func defaultGovv1Beta1ModifyGenesis() func(ibc.ChainConfig, []byte) ([]byte, err
return nil, fmt.Errorf("failed to unmarshal gov genesis bytes into map: %w", err)
}

if !testvalues.AllowAllClientsWildcardFeatureReleases.IsSupported(version) {
ibcModuleBytes, err := json.Marshal(appStateMap[ibcexported.ModuleName])
if err != nil {
return nil, fmt.Errorf("failed to extract ibc genesis bytes: %s", err)
}

ibcGenesisBytes, err := modifyClientGenesisAppState(chainConfig, ibcModuleBytes)
if err != nil {
return nil, err
}

ibcModuleGenesisMap := map[string]interface{}{}
err = json.Unmarshal(ibcGenesisBytes, &ibcModuleGenesisMap)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal gov genesis bytes into map: %w", err)
}
}

appStateMap[govtypes.ModuleName] = govModuleGenesisMap
genesisDocMap[appStateKey] = appStateMap

Expand Down Expand Up @@ -673,3 +702,24 @@ func modifyGovv1Beta1AppState(chainConfig ibc.ChainConfig, govAppState []byte) (

return govGenBz, nil
}

// modifyClientGenesisAppState takes the existing ibc app state and marshals it to a ibc GenesisState.
func modifyClientGenesisAppState(chainConfig ibc.ChainConfig, ibcAppState []byte) ([]byte, error) {
cfg := testutil.MakeTestEncodingConfig()

cdc := codec.NewProtoCodec(cfg.InterfaceRegistry)
clienttypes.RegisterInterfaces(cfg.InterfaceRegistry)

ibcGenesisState := &ibctypes.GenesisState{}
if err := cdc.UnmarshalJSON(ibcAppState, ibcGenesisState); err != nil {
return nil, fmt.Errorf("failed to unmarshal genesis bytes into client genesis state: %w", err)
}

ibcGenesisState.ClientGenesis.Params.AllowedClients = append(ibcGenesisState.ClientGenesis.Params.AllowedClients, wasmtypes.Wasm)
ibcGenBz, err := cdc.MarshalJSON(ibcGenesisState)
if err != nil {
return nil, fmt.Errorf("failed to marshal gov genesis state: %w", err)
}

return ibcGenBz, nil
}
5 changes: 5 additions & 0 deletions e2e/testvalues/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,8 @@ var LocalhostClientFeatureReleases = semverutil.FeatureReleases{
"v7.1",
},
}

// AllowAllClientsWildcardFeatureReleases represents the releases the allow all clients wildcard was released in.
var AllowAllClientsWildcardFeatureReleases = semverutil.FeatureReleases{
MajorVersion: "v9",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we add this feature in v8.1?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left v9 as the major version and added v8.1 in the minor versions.

Also added the backport label

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me update the docs before merging, because we probably have a few places where we should now mention that you don't need to update the allowed clients if you add a new client.

}
4 changes: 4 additions & 0 deletions modules/core/02-client/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ const (

// ParamsKey is the store key for the IBC client parameters
ParamsKey = "clientParams"

// AllowAllClients is the value that if set in AllowedClients param
// would allow any wired up light client modules to be allowed
AllowAllClients = "*"
)

// FormatClientIdentifier returns the client identifier with the sequence appended.
Expand Down
21 changes: 18 additions & 3 deletions modules/core/02-client/types/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ import (
"fmt"
"slices"
"strings"

"github.com/cosmos/ibc-go/v8/modules/core/exported"
)

// DefaultAllowedClients are the default clients for the AllowedClients parameter.
var DefaultAllowedClients = []string{exported.Solomachine, exported.Tendermint, exported.Localhost}
// By default it allows all client types.
var DefaultAllowedClients = []string{AllowAllClients}

// NewParams creates a new parameter configuration for the ibc client module
func NewParams(allowedClients ...string) Params {
Expand All @@ -30,11 +29,27 @@ func (p Params) Validate() error {

// IsAllowedClient checks if the given client type is registered on the allowlist.
func (p Params) IsAllowedClient(clientType string) bool {
// Still need to check for blank client type
if strings.TrimSpace(clientType) == "" {
return false
}

// Check for allow all client wildcard
// If exist then allow all type of client
if len(p.AllowedClients) == 1 && p.AllowedClients[0] == AllowAllClients {
return true
}

return slices.Contains(p.AllowedClients, clientType)
}

// validateClients checks that the given clients are not blank and there are no duplicates.
// If AllowAllClients wildcard (*) is used, then there should no other client types in the allow list
func validateClients(clients []string) error {
if slices.Contains(clients, AllowAllClients) && len(clients) > 1 {
return fmt.Errorf("allow list must have only one element because the allow all clients wildcard (%s) is present", AllowAllClients)
}

foundClients := make(map[string]bool, len(clients))
for i, clientType := range clients {
if strings.TrimSpace(clientType) == "" {
Expand Down
3 changes: 3 additions & 0 deletions modules/core/02-client/types/params_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ func TestIsAllowedClient(t *testing.T) {
{"success: valid client with custom params", exported.Tendermint, NewParams(exported.Tendermint), true},
{"success: invalid blank client", " ", DefaultParams(), false},
{"success: invalid client with custom params", exported.Localhost, NewParams(exported.Tendermint), false},
{"success: wildcard allow all clients", "test-client-type", NewParams(AllowAllClients), true},
{"success: wildcard allow all clients with blank client", " ", NewParams(AllowAllClients), false},
}

for _, tc := range testCases {
Expand All @@ -37,6 +39,7 @@ func TestValidateParams(t *testing.T) {
{"custom params", NewParams(exported.Tendermint), true},
{"blank client", NewParams(" "), false},
{"duplicate clients", NewParams(exported.Tendermint, exported.Tendermint), false},
{"allow all clients plus valid client", NewParams(AllowAllClients, exported.Tendermint), false},
}

for _, tc := range testCases {
Expand Down
2 changes: 0 additions & 2 deletions modules/light-clients/08-wasm/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,6 @@ func (suite *KeeperTestSuite) SetupWasmWithMockVM() {

suite.coordinator = ibctesting.NewCoordinator(suite.T(), 1)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))

wasmtesting.AllowWasmClients(suite.chainA)
}

func (suite *KeeperTestSuite) setupWasmWithMockVM() (ibctesting.TestingApp, map[string]json.RawMessage) {
Expand Down
9 changes: 0 additions & 9 deletions modules/light-clients/08-wasm/testing/wasm_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,3 @@ func (endpoint *WasmEndpoint) CreateClient() error {

return nil
}

// AllowWasmClients adds 08-wasm to the list of allowed clients
func AllowWasmClients(chain *ibctesting.TestChain) {
ctx := chain.GetContext()
clientKeeper := chain.App.GetIBCKeeper().ClientKeeper
params := clientKeeper.GetParams(ctx)
params.AllowedClients = append(params.AllowedClients, types.Wasm)
clientKeeper.SetParams(ctx, params)
}
2 changes: 0 additions & 2 deletions modules/light-clients/08-wasm/types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,6 @@ func (suite *TypesTestSuite) SetupWasmWithMockVM() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 1)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.checksum = storeWasmCode(suite, wasmtesting.Code)

wasmtesting.AllowWasmClients(suite.chainA)
}

func (suite *TypesTestSuite) setupWasmWithMockVM() (ibctesting.TestingApp, map[string]json.RawMessage) {
Expand Down
Loading