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.

Sign up for GitHub

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

refactor: rename IterateClients to IterateClientStates, add a prefix #2856

Merged
merged 6 commits into from
Nov 30, 2022
Merged

refactor: rename IterateClients to IterateClientStates, add a prefix #2856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b3fa48
refactor: rename IterateClients to IterateClientStates, add a prefix …
colin-axner Nov 30, 2022
e391ec9
test: add IterateClientStates test
colin-axner Nov 30, 2022
fd09362
chore: add changelog entry
colin-axner Nov 30, 2022
c94f1be
Merge branch 'main' into colin/2845-add-prefix-to-iterateclients
colin-axner Nov 30, 2022
89770cd
Update CHANGELOG.md
colin-axner Nov 30, 2022
f8a946c
Merge branch 'main' into colin/2845-add-prefix-to-iterateclients
colin-axner Nov 30, 2022
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking

* (core/02-client) [\#2856](https://github.com/cosmos/ibc-go/pull/2856) Rename `IterateClients` to `IterateClientStates`. The function now takes a prefix argument which may be used for prefix iteration over the client store.
* (apps/27-interchain-accounts) [\#2607](https://github.com/cosmos/ibc-go/pull/2607) `SerializeCosmosTx` now takes in a `[]proto.Message` instead of `[]sdk.Msg`.
* (apps/transfer) [\#2446](https://github.com/cosmos/ibc-go/pull/2446) Remove `SendTransfer` function in favor of a private `sendTransfer` function. All IBC transfers must be initiated with `MsgTransfer`.
* (apps/29-fee) [\#2395](https://github.com/cosmos/ibc-go/pull/2395) Remove param space from ics29 NewKeeper function. The field was unused.
Expand Down Expand Up @@ -115,6 +116,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* (core/24-host) [\#2856](https://github.com/cosmos/ibc-go/pull/2856) Add `PrefixedClientStorePath` and `PrefixedClientStoreKey` functions to 24-host
* (core/02-client) [\#2819](https://github.com/cosmos/ibc-go/pull/2819) Add automatic in-place store migrations to remove the localhost client and migrate existing solo machine definitions.
* (light-clients/06-solomachine) [\#2826](https://github.com/cosmos/ibc-go/pull/2826) Add `AppModuleBasic` for the 06-solomachine client and remove solo machine type registration from core IBC. Chains must register the `AppModuleBasic` of light clients.
* (light-clients/07-tendermint) [\#2825](https://github.com/cosmos/ibc-go/pull/2825) Add `AppModuleBasic` for the 07-tendermint client and remove tendermint type registration from core IBC. Chains must register the `AppModuleBasic` of light clients.
Expand Down
14 changes: 8 additions & 6 deletions modules/core/02-client/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func (k Keeper) IterateConsensusStates(ctx sdk.Context, cb func(clientID string,
// GetAllGenesisClients returns all the clients in state with their client ids returned as IdentifiedClientState
func (k Keeper) GetAllGenesisClients(ctx sdk.Context) types.IdentifiedClientStates {
var genClients types.IdentifiedClientStates
k.IterateClients(ctx, func(clientID string, cs exported.ClientState) bool {
k.IterateClientStates(ctx, nil, func(clientID string, cs exported.ClientState) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of method signatures where explicitly passing nil an intended/regular workflow. Maybe this is overkill, but I think I prefer the idea of an explicit sentinel value like

var EmptyPrefx []byte

And use this rather than nil. A value of nil could mean a lot of different things, with a value like this it is quite obvious what we are doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds fine to me. I'm impartial to this decision. The SDK already makes use of nil with iterators so it doesn't feel out of the normal for me

Where would you place the EmptyPrefix, 24-host?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, if it's already an existing pattern to pass nil to iterators then it probably is better to just stay consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I'm still happy to go with a sentinel value if we think that is cleaner. I guess we can use nil for now an update in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me 👍

genClients = append(genClients, types.NewIdentifiedClientState(clientID, cs))
return false
})
Expand Down Expand Up @@ -350,12 +350,12 @@ func (k Keeper) SetUpgradedConsensusState(ctx sdk.Context, planHeight int64, bz
return k.upgradeKeeper.SetUpgradedConsensusState(ctx, planHeight, bz)
}

// IterateClients provides an iterator over all stored light client State
// IterateClientStates provides an iterator over all stored light client State
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a section which says that nil is an allowed value for the prefix?

Unless we move to sentinel value in which case we can outline that.

// objects. For each State object, cb will be called. If the cb returns true,
// the iterator will close and stop.
func (k Keeper) IterateClients(ctx sdk.Context, cb func(clientID string, cs exported.ClientState) bool) {
func (k Keeper) IterateClientStates(ctx sdk.Context, prefix []byte, cb func(clientID string, cs exported.ClientState) bool) {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, host.KeyClientStorePrefix)
iterator := sdk.KVStorePrefixIterator(store, host.PrefixedClientStoreKey(prefix))

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
Expand All @@ -375,11 +375,13 @@ func (k Keeper) IterateClients(ctx sdk.Context, cb func(clientID string, cs expo
}

// GetAllClients returns all stored light client State objects.
func (k Keeper) GetAllClients(ctx sdk.Context) (states []exported.ClientState) {
k.IterateClients(ctx, func(_ string, state exported.ClientState) bool {
func (k Keeper) GetAllClients(ctx sdk.Context) []exported.ClientState {
var states []exported.ClientState
k.IterateClientStates(ctx, nil, func(_ string, state exported.ClientState) bool {
states = append(states, state)
return false
})

return states
}

Expand Down
64 changes: 64 additions & 0 deletions modules/core/02-client/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,3 +386,67 @@ func (suite KeeperTestSuite) TestGetAllConsensusStates() {
consStates := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetAllConsensusStates(suite.chainA.GetContext())
suite.Require().Equal(expConsensusStates, consStates, "%s \n\n%s", expConsensusStates, consStates)
}

func (suite KeeperTestSuite) TestIterateClientStates() {
paths := []*ibctesting.Path{
ibctesting.NewPath(suite.chainA, suite.chainB),
ibctesting.NewPath(suite.chainA, suite.chainB),
ibctesting.NewPath(suite.chainA, suite.chainB),
}

solomachines := []*ibctesting.Solomachine{
ibctesting.NewSolomachine(suite.T(), suite.chainA.Codec, "06-solomachine-0", "testing", 1),
ibctesting.NewSolomachine(suite.T(), suite.chainA.Codec, "06-solomachine-1", "testing", 4),
}

var (
expTMClientIDs = make([]string, len(paths))
expSMClientIDs = make([]string, len(solomachines))
)

// create tendermint clients
for i, path := range paths {
suite.coordinator.SetupClients(path)
expTMClientIDs[i] = path.EndpointA.ClientID
}

// create solomachine clients
for i, sm := range solomachines {
expSMClientIDs[i] = sm.CreateClient(suite.chainA)
}

testCases := []struct {
name string
prefix []byte
expClientIDs []string
}{
{
"all clientIDs",
nil,
append(expSMClientIDs, expTMClientIDs...),
},
{
"tendermint clientIDs",
[]byte(exported.Tendermint),
expTMClientIDs,
},
{
"solo machine clientIDs",
[]byte(exported.Solomachine),
expSMClientIDs,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
var clientIDs []string
suite.chainA.GetSimApp().IBCKeeper.ClientKeeper.IterateClientStates(suite.chainA.GetContext(), tc.prefix, func(clientID string, _ exported.ClientState) bool {
clientIDs = append(clientIDs, clientID)
return false
})

suite.Require().Equal(tc.expClientIDs, clientIDs)
})
}
}
2 changes: 1 addition & 1 deletion modules/core/03-connection/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func (k Keeper) SetNextConnectionSequence(ctx sdk.Context, sequence uint64) {
// no paths are stored.
func (k Keeper) GetAllClientConnectionPaths(ctx sdk.Context) []types.ConnectionPaths {
var allConnectionPaths []types.ConnectionPaths
k.clientKeeper.IterateClients(ctx, func(clientID string, cs exported.ClientState) bool {
k.clientKeeper.IterateClientStates(ctx, nil, func(clientID string, cs exported.ClientState) bool {
paths, found := k.GetClientConnectionPaths(ctx, clientID)
if !found {
// continue when connection handshake is not initialized
Expand Down
2 changes: 1 addition & 1 deletion modules/core/03-connection/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ type ClientKeeper interface {
GetClientConsensusState(ctx sdk.Context, clientID string, height exported.Height) (exported.ConsensusState, bool)
GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error)
ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error
IterateClients(ctx sdk.Context, cb func(string, exported.ClientState) bool)
IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool)
ClientStore(ctx sdk.Context, clientID string) sdk.KVStore
}
14 changes: 14 additions & 0 deletions modules/core/24-host/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,20 @@ func FullClientKey(clientID string, path []byte) []byte {
return []byte(FullClientPath(clientID, string(path)))
}

// PrefixedClientStorePath returns a key path which can be used for prefixed
// key store iteration. The prefix may be a clientType, clientID, or any
// valid key prefix which may be concatenated with the client store constant.
func PrefixedClientStorePath(prefix []byte) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This ends up returning clients/ when nil is passed, is it worth updating the doc string to indicate that nil is a valid/expected value and will result in using clients/ as the prefix iterator?

Or we could be explicit and do a nil check and return KeyClientStorePrefix directly. I don't think I'm a big fan of letting a nil get passed into fmt.Sprintf.

Copy link
Contributor

Choose a reason for hiding this comment

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

^^ this was written before the suggestion for a sentinel value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you want any differing behaviour if we used a sentinel value?

return fmt.Sprintf("%s/%s", KeyClientStorePrefix, prefix)
}

// PrefixedClientStoreKey returns a key which can be used for prefixed
// key store iteration. The prefix may be a clientType, clientID, or any
// valid key prefix which may be concatenated with the client store constant.
func PrefixedClientStoreKey(prefix []byte) []byte {
return []byte(PrefixedClientStorePath(prefix))
}

// ICS02
// The following paths are the keys to the store as defined in https://github.com/cosmos/ibc/tree/master/spec/core/ics-002-client-semantics#path-space

Expand Down