Move channel back to counterparty #7842

AdityaSripal · 2025-01-13T16:10:42Z

Description

closes: #7739

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

Thanks! This is going to be very nice! 🙌

The general structure and logic seems straightforward and understandable. I have some questions on naming that I left in comments.

proto/ibc/core/client/v2/counterparty.proto

gjermundgaraba · 2025-01-15T11:18:33Z

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

+// resolveV2Identifiers returns the client identifier and the counterpartyInfo for the client given the packetId
+// Note: For fresh eureka channels, the client identifier and packet identifier are the same.
+// For aliased channels, the packet identifier will be the original channel ID and the counterpartyInfo will be constructed from the channel
+func (k *Keeper) resolveV2Identifiers(ctx context.Context, portId string, packetId string) (string, clienttypes.CounterpartyInfo, error) {


I think maybe the name could be better here. If I have to read the comment to understand what the function does for me, I consider it a code smell :)

After seeing it used further down I am even more confused. Is it taking in packetId or a client id?

Yea this was because it could also be an original channelID for aliasing, in which case I still need to retrieve the original clientiD

gjermundgaraba · 2025-01-15T11:20:22Z

proto/ibc/core/channel/v2/packet.proto

@@ -14,9 +14,9 @@ message Packet {
  // with a later sequence number.
  uint64 sequence = 1;
  // identifies the sending chain.
-  string source_channel = 2;
+  string source_id = 2;


It will be the last time I bring this up (I hope), but what is the reason we don't want to name source and destination identifier client_id (as in source_client_id)? Can it be anything else?

gjermundgaraba · 2025-01-15T11:22:02Z

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

-			return 0, "", errorsmod.Wrap(types.ErrChannelNotFound, sourceChannel)
-		}
+	// lookup counterparty and clientid from packet identifiers
+	clientID, counterparty, err := k.resolveV2Identifiers(ctx, payloads[0].SourcePort, sourceId)


I thought this function was taking in a packet identifier? But it is taking in the source (client) identifier?

AdityaSripal · 2025-01-16T09:34:07Z

modules/apps/transfer/v2/keeper/msg_server_test.go

+			transfertypes.NewHop(transfertypes.ModuleName, pathv2.EndpointB.ClientID),
 			transfertypes.NewHop(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID),


First hop used IBC Classic, second hop used IBC Eureka

AdityaSripal · 2025-01-16T09:37:27Z

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

 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}

 	if req.Sequence == 0 {
 		return nil, status.Error(codes.InvalidArgument, "packet sequence cannot be 0")
 	}

-	if !q.HasChannel(ctx, req.ChannelId) {


This was a pain to find the analog of especially in a world with aliasing. Just decided to remove it since if client (or aliased client) doesn't exist, none of these other auxilliary state will exist either

AdityaSripal · 2025-01-16T09:39:58Z

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

 				}
 			},
 			status.Error(codes.InvalidArgument, "packet sequence cannot be 0"),
 		},
-		{
-			"channel not found",


Removed these cases. They still error but now equivalent to the queried key not existing

AdityaSripal · 2025-01-16T09:50:09Z

proto/ibc/core/channel/v2/query.proto

  // NextSequenceSend returns the next send sequence for a given channel.
  rpc NextSequenceSend(QueryNextSequenceSendRequest) returns (QueryNextSequenceSendResponse) {
-    option (google.api.http).get = "/ibc/core/channel/v2/channels/{channel_id}/next_sequence_send";
+    option (google.api.http).get = "/ibc/core/channel/v2/clients/{client_id}/next_sequence_send";


There rpc urls are very ugly since they still contain channel/v2. Should we pull this whole thing back out to its own package packetserver? 🫣

I think that would be cleaner, yes.

I don't mind the urls but up to you

AdityaSripal · 2025-01-16T09:51:55Z

proto/ibc/core/client/v2/counterparty.proto

+// CounterpartyInfo defines the key that the counterparty will use to message our client
+message CounterpartyInfo {
+  // merkle prefix key is the prefix that ics provable keys are stored under
+  repeated bytes merkle_prefix = 1;


I decided to just make this repeated bytes rather than importing ics23.commitment MerklePath

I think the inclusion of ics23 into the required external structs has caused more confusion than anything else. It is also an unnecessary import here

I convert to ics23 commitment MerklePath internally

I think it is ok to have this be ics23.commitment.MerklePath but use repeated bytes in MsgRegisterCounterparty since CounterpartyInfo is used internally anyway?

But this is also good :)

AdityaSripal · 2025-01-16T11:48:00Z

Now that we have channel completely removed should we move channel/v2 into a different package? Perhaps back to packetserver

gjermundgaraba · 2025-01-16T12:15:04Z

Now that we have channel completely removed should we move channel/v2 into a different package? Perhaps back to packetserver

I think that would make sense and keep things much cleaner. Not sure what the best name is, but separate package for sure.

gjermundgaraba

I like where this is going! Left some mostly nit comments :)

gjermundgaraba · 2025-01-16T12:16:29Z

proto/ibc/core/channel/v2/query.proto

  // NextSequenceSend returns the next send sequence for a given channel.
  rpc NextSequenceSend(QueryNextSequenceSendRequest) returns (QueryNextSequenceSendResponse) {
-    option (google.api.http).get = "/ibc/core/channel/v2/channels/{channel_id}/next_sequence_send";
+    option (google.api.http).get = "/ibc/core/channel/v2/clients/{client_id}/next_sequence_send";


I think that would be cleaner, yes.

gjermundgaraba · 2025-01-16T12:17:40Z

proto/ibc/core/channel/v2/packet.proto

-  // identifies the receiving chain.
-  string destination_channel = 3;
+  // identifies the sending client on the sending chain.
+  string source_client = 2;


nit: but why not source_client_id? Since we use client_id everywhere else?

I will leave this out for now, since this is analogous to what has existed in ibc-go forever. We can make a decision to change in a different PR/issue

Created an issue to track this here #7854

gjermundgaraba · 2025-01-16T12:21:14Z

modules/apps/transfer/v2/keeper/msg_server_test.go

 			func() {},
 			func() {
-				packet.SourceChannel = ibctesting.InvalidID
+				packet.SourceClient = ibctesting.InvalidID


This is a bit of a nit, but I don't get why we don't just add the ID to all these names? If we're dealing with the ID, and we use that name in most other contexts, why not keep it consistent and always refer to it as SourceClientID and so on?

gjermundgaraba · 2025-01-16T12:30:24Z

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

@@ -19,50 +20,40 @@ import (
 // in order for the packet to be sent to the counterparty.
 func (k *Keeper) sendPacket(
 	ctx context.Context,
-	sourceChannel string,
+	sourceClient string,


Same here with the naming. It's a bit of a nit, but the more I see it, the more I feel like we should just use ClientID in all names where it that is really what it is.

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

srdtrk

lgtm. Surprised so many lines were deleted. This is the right way it seems.

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

modules/core/02-client/types/counterparty.go

modules/core/02-client/types/keys.go

modules/core/04-channel/v2/client/cli/cli.go

srdtrk · 2025-01-17T11:17:23Z

modules/core/keeper/msg_server.go

+// it must be called by the same relayer that called CreateClient
+func (k *Keeper) RegisterCounterparty(ctx context.Context, msg *clienttypes.MsgRegisterCounterparty) (*clienttypes.MsgRegisterCounterpartyResponse, error) {
+	creator := k.ClientKeeper.GetClientCreator(ctx, msg.ClientId)
+	if !creator.Equals(sdk.AccAddress(msg.Signer)) {


Should you instead have the creator become a string? Should be consistent with how we do these comparisons elsewhere?

Left as is now, since it that case, we may as well just have the keeper return a string. Maybe we can do later as a small followup if really necessary

srdtrk · 2025-01-17T11:19:29Z

proto/ibc/core/channel/v2/query.proto

  // NextSequenceSend returns the next send sequence for a given channel.
  rpc NextSequenceSend(QueryNextSequenceSendRequest) returns (QueryNextSequenceSendResponse) {
-    option (google.api.http).get = "/ibc/core/channel/v2/channels/{channel_id}/next_sequence_send";
+    option (google.api.http).get = "/ibc/core/channel/v2/clients/{client_id}/next_sequence_send";


I don't mind the urls but up to you

srdtrk · 2025-01-17T11:21:55Z

proto/ibc/core/client/v2/counterparty.proto

+// CounterpartyInfo defines the key that the counterparty will use to message our client
+message CounterpartyInfo {
+  // merkle prefix key is the prefix that ics provable keys are stored under
+  repeated bytes merkle_prefix = 1;


I think it is ok to have this be ics23.commitment.MerklePath but use repeated bytes in MsgRegisterCounterparty since CounterpartyInfo is used internally anyway?

But this is also good :)

sonarqubecloud · 2025-01-17T15:41:35Z

Quality Gate passed for 'ibc-go'

Issues
0 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

add back client registerCounterparty

e1167c9

AdityaSripal changed the base branch from main to feat/ibc-eureka January 13, 2025 16:10

AdityaSripal added 4 commits January 14, 2025 14:05

change channel to id in packet

09eacf4

fix tests

70d5872

split messaging key to prefix and clientId

f2bf1ed

change eureka packet handlers to no longer rely on channel

180e5f1

gjermundgaraba reviewed Jan 15, 2025

View reviewed changes

AdityaSripal added 6 commits January 15, 2025 14:32

naming suggestions

480ba8e

fix test file

63bed6b

start to fix tests

249c170

fix v2/keeper tests

be89aaf

fix tests

b632f4c

DELETE channel from v2

0ba8de7

AdityaSripal commented Jan 16, 2025

View reviewed changes

AdityaSripal added 4 commits January 16, 2025 10:52

rm commented code

a5aa5ff

move nextSeqSend to a better place

adcb847

remove resolveV2Identifiers for now

25b8bb6

lint

a0eb482

AdityaSripal marked this pull request as ready for review January 16, 2025 11:47

AdityaSripal requested a review from srdtrk as a code owner January 16, 2025 11:47

lint

b0c1f22

gjermundgaraba reviewed Jan 16, 2025

View reviewed changes

srdtrk approved these changes Jan 17, 2025

View reviewed changes

AdityaSripal added 3 commits January 17, 2025 15:26

review suggestions

b759f29

Update cli.go

09a4ac8

Merge branch 'feat/ibc-eureka' into aditya/back-to-counterparty

7151c12

gjermundgaraba mentioned this pull request Jan 17, 2025

Use client ID naming consistently #7854

Open

AdityaSripal merged commit b3184b4 into feat/ibc-eureka Jan 17, 2025
67 checks passed

AdityaSripal deleted the aditya/back-to-counterparty branch January 17, 2025 16:19

AdityaSripal mentioned this pull request Jan 20, 2025

chore: add channels rpc #7560

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move channel back to counterparty #7842

Move channel back to counterparty #7842

AdityaSripal commented Jan 13, 2025 •

edited

Loading

gjermundgaraba left a comment

gjermundgaraba Jan 15, 2025

gjermundgaraba Jan 15, 2025

AdityaSripal Jan 15, 2025

gjermundgaraba Jan 15, 2025

gjermundgaraba Jan 15, 2025

AdityaSripal Jan 16, 2025

AdityaSripal Jan 16, 2025

AdityaSripal Jan 16, 2025

AdityaSripal Jan 16, 2025

gjermundgaraba Jan 16, 2025

srdtrk Jan 17, 2025

AdityaSripal Jan 16, 2025

srdtrk Jan 17, 2025

AdityaSripal commented Jan 16, 2025

gjermundgaraba commented Jan 16, 2025

gjermundgaraba left a comment •

edited

Loading

gjermundgaraba Jan 16, 2025

gjermundgaraba Jan 16, 2025

AdityaSripal Jan 17, 2025

gjermundgaraba Jan 18, 2025

gjermundgaraba Jan 16, 2025

gjermundgaraba Jan 16, 2025

srdtrk left a comment

srdtrk Jan 17, 2025

AdityaSripal Jan 17, 2025

srdtrk Jan 17, 2025

srdtrk Jan 17, 2025

sonarqubecloud bot commented Jan 17, 2025

		transfertypes.NewHop(transfertypes.ModuleName, pathv2.EndpointB.ClientID),
		transfertypes.NewHop(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID),

Move channel back to counterparty #7842

Move channel back to counterparty #7842

Conversation

AdityaSripal commented Jan 13, 2025 • edited Loading

Description

gjermundgaraba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jan 16, 2025

gjermundgaraba commented Jan 16, 2025

gjermundgaraba left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 17, 2025

Quality Gate passed for 'ibc-go'

AdityaSripal commented Jan 13, 2025 •

edited

Loading

gjermundgaraba left a comment •

edited

Loading