feat: clientv2 module by aljo242 · Pull Request #7936 · cosmos/ibc-go

aljo242 · 2025-02-07T21:48:43Z

Description

closes: #7875
closes: #7923

move all Counterparty keeper and type logic to a v2 package
create query implementation
create and wire genesis into core module

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

gjermundgaraba

Similar to the comment in channel-v2. Genesis (including tests) and most of the wiring happens in the core module for v1. v2 should do the same.

AdityaSripal

I think I would prefer the channelV2Keeper still only taking a single clientV2Keeper since the fact that they are different is an implementation detail that only exists because ibc-go is implementing both IBC Classic and IBC Eureka

But open to pushback from the other engineers

AdityaSripal · 2025-02-10T17:26:19Z

modules/core/02-client/v2/keeper/grpc_query.go

+// CounterpartyInfo gets the CounterpartyInfo from the store corresponding to the request client ID.
+func (q queryServer) CounterpartyInfo(ctx context.Context, request *types.QueryCounterpartyInfoRequest) (*types.QueryCounterpartyInfoResponse, error) {
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+


We generally do a bit more validation before the grpc directly gets from store. Would be good to follow the same.

See the v1 chanKeeper grpc implementation

AdityaSripal · 2025-02-10T17:28:10Z

modules/core/02-client/v2/keeper/grpc_query.go

Should have a test file for this as well.

AdityaSripal · 2025-02-10T17:28:56Z

modules/core/02-client/v2/keeper/keeper_test.go

+	cdc            codec.Codec
+	ctx            sdk.Context
+	keeper         *keeper.Keeper
+	consensusState *ibctm.ConsensusState
+	valSet         *cmttypes.ValidatorSet
+	valSetHash     cmtbytes.HexBytes
+	privVal        cmttypes.PrivValidator
+	now            time.Time
+	past           time.Time
+	solomachine    *ibctesting.Solomachine
+
+	signers map[string]cmttypes.PrivValidator


Lot of unnecessary fields here

AdityaSripal · 2025-02-10T17:37:24Z

modules/core/04-channel/v2/types/expected_keepers.go

 	GetClientState(ctx context.Context, clientID string) (exported.ClientState, bool)
 	// GetClientConsensusState gets the stored consensus state from a client at a given height.
 	GetClientConsensusState(ctx context.Context, clientID string, height exported.Height) (exported.ConsensusState, bool)
-	// GetClientCounterparty returns the counterpartyInfo given a clientID
-	GetClientCounterparty(ctx context.Context, clientID string) (clienttypes.CounterpartyInfo, bool)


I kind of think this should remain but have clientV2Keeper implement this interface and only have channelKeeper take in a ClientV2Keeper.

What do you think @gjermundgaraba

To me it seems a bit strange to have it on v1, since it is not used there. The only reason to keep it on v1 would be to avoid having to deal with v2. I feel like it would be less indirection and clearer code if we use the v2 keeper directly when needed.

@AdityaSripal do you mean to have the ClientV2Keeper basically wrap the client V1 Keeper and extend it with this extra CounterParty functionality?

Yes, but i will do this in a separate PR probably after the audit

AdityaSripal · 2025-02-10T17:38:57Z

modules/core/04-channel/v2/keeper/keeper.go

@@ -37,13 +41,15 @@ func NewKeeper(
 	cdc codec.BinaryCodec,
 	env appmodule.Environment,
 	clientKeeper types.ClientKeeper,


Think we should still just have this interface and have clientV2Keeper implement it

Do you mean have the clientV2Keeper implement the full ClientKeeper interface?

sonarqubecloud · 2025-02-10T23:13:20Z

Quality Gate passed for 'ibc-go'

Issues
84 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

AdityaSripal · 2025-02-10T23:18:26Z

modules/core/02-client/v2/keeper/grpc_query_test.go

+				path1.SetupClients()
+				// counter party not set up
+
+				expInfo = types.NewCounterpartyInfo(path1.EndpointA.Counterparty.MerklePathPrefix.KeyPath, path1.EndpointA.ClientID)


this doesn't need to exist right since we have non-nil err

AdityaSripal · 2025-02-10T23:22:30Z

modules/core/02-client/v2/types/genesis.go

+
+// Validate checks the CounterpartyInfos added to the genesis for validity.
+func (gs GenesisState) Validate() error {
+	seenIDs := make(map[string]struct{})


why do you do this instead of a bool?

It's a good check but don't think we do this elsewhere

You think better to remove? The empty struct pattern is a bit of a go idiom for doing inclusion checks like this. Its technically more space / cache efficient because to the compiler it does not take up space whereas a bool does.

In this case it is such a minor use case that it wont have any impact.

ahh i see. no its fine keep it as-is

AdityaSripal · 2025-02-11T16:45:04Z

modules/core/04-channel/v2/types/expected_keepers.go

 	GetClientState(ctx context.Context, clientID string) (exported.ClientState, bool)
 	// GetClientConsensusState gets the stored consensus state from a client at a given height.
 	GetClientConsensusState(ctx context.Context, clientID string, height exported.Height) (exported.ConsensusState, bool)
-	// GetClientCounterparty returns the counterpartyInfo given a clientID
-	GetClientCounterparty(ctx context.Context, clientID string) (clienttypes.CounterpartyInfo, bool)


Yes, but i will do this in a separate PR probably after the audit

addressed

aljo242 added 9 commits February 7, 2025 14:23

init

71a3e9f

move logic

128c373

fix all tests and types

9973f79

genesis proto and gen

ee51649

gen

21fac15

godoc

290837a

core genesis

2e3d279

core genesis wiring

f7d8b45

wire and test

796236a

aljo242 requested review from AdityaSripal, gjermundgaraba and srdtrk as code owners February 7, 2025 21:48

aljo242 added 3 commits February 7, 2025 16:50

lint fix

2dbfaed

fmt

034b18e

fmt

0620d48

gjermundgaraba previously requested changes Feb 10, 2025

View reviewed changes

AdityaSripal suggested changes Feb 10, 2025

View reviewed changes

aljo242 added 3 commits February 10, 2025 16:44

Merge branch 'main' into feat/client-v2-module

801fa99

add defensive checks

640d996

fix all

687b196

aljo242 requested a review from gjermundgaraba February 10, 2025 22:54

Merge branch 'main' into feat/client-v2-module

b3615be

AdityaSripal reviewed Feb 10, 2025

View reviewed changes

Merge branch 'main' into feat/client-v2-module

69624f7

AdityaSripal approved these changes Feb 11, 2025

View reviewed changes

AdityaSripal enabled auto-merge February 11, 2025 17:09

Merge branch 'main' into feat/client-v2-module

8bf8a63

AdityaSripal added this pull request to the merge queue Feb 11, 2025

Merged via the queue into main with commit 5296070 Feb 11, 2025
58 of 64 checks passed

AdityaSripal deleted the feat/client-v2-module branch February 11, 2025 18:39

Conversation

aljo242 commented Feb 7, 2025

Description

Uh oh!

gjermundgaraba left a comment

Choose a reason for hiding this comment

Uh oh!

AdityaSripal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 10, 2025

Quality Gate passed for 'ibc-go'

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants