-
Notifications
You must be signed in to change notification settings - Fork 577
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
Draft: Aliasing the v1 channels to v2 counterparty so that v1 channels can support v2 packets #7174
base: feat/ibc-eureka
Are you sure you want to change the base?
Conversation
@@ -95,6 +96,30 @@ func (k *Keeper) SetChannel(ctx sdk.Context, portID, channelID string, channel t | |||
store.Set(host.ChannelKey(portID, channelID), bz) | |||
} | |||
|
|||
// GetV2Counterparty returns a version 2 counterparty for the given port and channel ID | |||
// by converting the channel into a version 2 counterparty | |||
func (k *Keeper) GetV2Counterparty(ctx sdk.Context, portID, channelID string) (clienttypes.Counterparty, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effectively resolves the portID and channel ID into a counterparty. So the portID and channelID are in effect the clientID here. It's just a different arbitrary string identifier.
The one annoying thing is that the portID here is both acting as part of the v2 "client-id" (though we could move away from this with a different storage format since channel ids are unique) and also selecting the application to use.
This is fine for packets that are only sending one app data.
For MULTI_PACKET_DATA, we can deal with this by leaving the top portID as is, with the plan to eventually deprecate it. And then still allowing different ports in the internal packet data list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @AdityaSripal.
Taking transfer packets as an example, with the channel aliasing it will be possible to send a packet from an ibc-go v10 chain to another ibc-go v10 chain either on a existing transfer channel that was already set up or over eureka, right? Are we going to leave the decision of over which channel to send the packet to the user? If that's the case, would we need to add a protocol version field to MsgSendPacket
? And the transfer application, which also sends a MsgSendPacket
, should specify decide if the packet should be sent over a classic or an eureka channel, right?
It would be nice if there was a way by which we could internally determine if a counterparty chain supports eureka and then we prioritise sending the packet over a eureka channel instead of the existing classic channel. But I don't think that's going to be easy, right?
@@ -125,7 +125,7 @@ func (k Keeper) RecvPacket( | |||
// Lookup counterparty associated with our channel and ensure that it was packet was indeed | |||
// sent by our counterparty. | |||
// Note: This can be implemented by the current keeper | |||
counterparty, ok := k.ClientKeeper.GetCounterparty(ctx, packet.DestinationChannel) | |||
counterparty, ok := k.GetCounterparty(ctx, packet.DestinationPort, packet.DestinationChannel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a transfer V2 packet sent from an ibc-go v10 chain to another ibc-go v10 chain over an existing channel, the packet's DestinationChannel
will be filled in with the client ID of the destination chain, right? But then in the OnRecvPacket
callback of the transfer the hash of the denomination will not match that of the tokens received previously (since those used the destination channel ID and not a client ID). Then those vouchers would not be fungible, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the DestinationChannel
will continue to be the existing channel identifier (that's the purpose of this adjustment, to keep fungibility but allow existing channels to send v2 packets)
# Conflicts: # modules/core/02-client/keeper/keeper_test.go # modules/core/02-client/types/client.go # modules/core/02-client/types/client.pb.go # modules/core/02-client/types/client_test.go # modules/core/02-client/types/errors.go # modules/core/02-client/types/msgs.go # modules/core/keeper/msg_server_test.go # modules/core/packet-server/keeper/keeper.go # modules/core/packet-server/keeper/keeper_test.go # modules/core/packet-server/types/keys.go # proto/ibc/core/client/v1/client.proto # testing/endpoint.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of changes had been done on the PR branch before it was synced with the feature branch, so I took me a bit of time to fix the conflicts and sync. I had to comment out some things (for which we can open an issue to keep track). I hope I didn't miss anything while fixing the conflicts. 🤞
flags.AddTxFlagsToCmd(cmd) | ||
return cmd | ||
} | ||
// func newProvideCounterpartyCmd() *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented out this for now because in this PR MsgProvideCounterparty
has changed and needs more information from the CLI to fill it all in.
counterparty, _ := q.GetCounterparty(sdkCtx, req.ClientId) | ||
res.Counterparty = counterparty | ||
// counterparty, _ := q.GetCounterparty(sdkCtx, req.ClientId) | ||
// res.Counterparty = counterparty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented this also out because there is a circular dependency in the pb.go file between 02-client and packet-server types.
string creator = 1; | ||
Counterparty counterparty = 2 [(gogoproto.nullable) = false]; | ||
string creator = 1; | ||
// ibc.core.packetserver.v1.Counterparty counterparty = 2 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented this out to prevent the circular dependency I mentioned above. We might need to move Counterparty
to a different place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assume we'd simply move the grpc query? We can leave this commented out for now until the core logic is complete, though.
// client id of the counterparty stored on our chain | ||
string client_id = 1; | ||
// the counterparty channel identifier that must be used by the packet | ||
string counterparty_channel = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string counterparty_channel = 2; | |
string counterparty_id = 2; |
naming suggestion, fine as is
// Counterparty defines the counterparty for a light client to implement IBC eureka protocol | ||
// this value is stored under our side's channel ID. which we will use to write all packet messages | ||
// sent to counterparty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Counterparty defines the counterparty for a light client to implement IBC eureka protocol | |
// this value is stored under our side's channel ID. which we will use to write all packet messages | |
// sent to counterparty | |
// Counterparty defines the counterparty for a light client to implement IBC eureka protocol | |
// this value is stored under our side's channel ID, which we will use to write all packet messages | |
// sent to counterparty. |
// the client identifier of the counterparty chain | ||
// client id of the counterparty stored on our chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the client identifier of the counterparty chain | |
// client id of the counterparty stored on our chain | |
// the client identifier of the light client representing the counterparty chain |
// the counterparty channel identifier that must be used by the packet | ||
string counterparty_channel = 2; | ||
// the key path used to store packet flow messages that the counterparty | ||
// will use to send to us. We will append the channelID and sequence in order to create the final path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// will use to send to us. We will append the channelID and sequence in order to create the final path. | |
// will use to send to us. In backwards compatible cases, we will append the channelID and sequence in order to create the final path. |
// Counterparty defines the counterparty for a light client to implement IBC eureka protocol | ||
// this value is stored under our side's channel ID. which we will use to write all packet messages | ||
// sent to counterparty | ||
message Counterparty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it'd be nice to see this Counterparty
type as something used by historical situations (channel/port), but not necessarily only for that
option (gogoproto.goproto_getters) = false; | ||
|
||
// unique identifier we will use to write all packet messages sent to counterparty | ||
string channel_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string channel_id = 1; | |
string id = 1; |
similar naming suggestion
// CounterpartyKey is the key used to store counterparty in the client store. | ||
// the counterparty key is imported from types instead of host because | ||
// the counterparty key is not a part of the ics-24 host specification | ||
CounterpartyKey = "counterparty" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if we should be using collections when creating a new store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, if we are reusing the channel store, fine as is
@@ -95,6 +97,30 @@ func (k *Keeper) SetChannel(ctx sdk.Context, portID, channelID string, channel t | |||
store.Set(host.ChannelKey(portID, channelID), bz) | |||
} | |||
|
|||
// GetV2Counterparty returns a version 2 counterparty for the given port and channel ID | |||
// by converting the channel into a version 2 counterparty | |||
func (k *Keeper) GetV2Counterparty(ctx sdk.Context, portID, channelID string) (packetserver.Counterparty, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting this isn't a function of the packet server keeper? I'm assuming usage still needs to be implemented?
if !ok { | ||
return packetserver.Counterparty{}, false | ||
} | ||
merklePathPrefix := commitmentv2types.NewMerklePath(connection.Counterparty.Prefix.KeyPrefix, []byte("")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
byte slice needs channel id info?
@@ -143,10 +145,10 @@ func (k *Keeper) IBCSoftwareUpgrade(goCtx context.Context, msg *clienttypes.MsgI | |||
} | |||
|
|||
// ProvideCounterparty defines a rpc handler method for MsgProvideCounterparty. | |||
func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *clienttypes.MsgProvideCounterparty) (*clienttypes.MsgProvideCounterpartyResponse, error) { | |||
func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *packetservertypes.MsgProvideCounterparty) (*packetservertypes.MsgProvideCounterpartyResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets initialize the packet sequence when setting the counterparty? This would allow us to remove the if's in send packet. I feel like calling ProvideCounterparty
should be viewed as finalizing the communication details
counterparty, ok := k.GetCounterparty(ctx, sourceChannel) | ||
if !ok { | ||
return 0, errorsmod.Wrap(types.ErrCounterpartyNotFound, sourceChannel) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that we basically want logic:
if channeltypes.IsChannelIDFormat(sourceChannel) {
counterparty, ok = k.channelKeeper.GetV2Counterparty(ctx, sourcePort, sourceChannel)
} else {
counterparty, ok = k.GetCounterparty(ctx, sourceChannel)
}
if !ok {
return 0, errorsmod.Wrap(types.ErrCounterpartyNotFound, sourceChannel)
}
I think we should consider breaking up this pr for digestibility, I think there are some changes that should have been left for a followup. |
message Counterparty { | ||
// the client identifier of the counterparty chain | ||
// client id of the counterparty stored on our chain | ||
string client_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be removed now. I see this mapping as:
key: my identifier (v1: channel/port, v2: clientid)
value: counterparty id, merkle path
The client id used to verify the counterparty is in the key, or can be resolved in the v1 case. It seems redundant atm, I would guess in the actual implementation it would only be referenced by the grpc, but we should probably create a new struct for grpc, like we did with the other types IdentifiedClientState
for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really we have a mapping from: our reserved key path -> counterparty reserved key path
resolving the client id responsible for the key path, should happen outside of this mapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I forget, I went down the thought rabbit hole on this. The proposed construction works perfectly fine, but there's just some minor considerations one could have:
The current construction embeds the clientID into the Counterparty
. When we handle v1 cases, the clientID is resolved and set into the counterparty type. In the v2 case, the value is redundant as it is in the key setting the value. The main benefit is code structure as it bundles all the necessary components into a single type. One could also return the clientID in the get counterparty function to achieve the same functionality.
The main reason to keep this approach, would be for future scenarios where we delete the channel/connection mappings and you want to migrate such that all v1 mappings are directly embedded into the counterparty type. One could have two mappings, as I hint at in the above comment. One to obtain the counterparties keyspace which is set for all v1/v2 and one to obtain the light client either directly in v2 case, or via a lookup in v1 case.
The reason why I went this rabbit hole was primarily due to naming. counterparty.clientID
referring to your local client identifier, and not a client identifier on the counterparty, could be confusing. Alternatively, if you renamed the Counterparty
to something like Connection
or Channel
, then connection.clientId
and connection.counterpartyID
could make sense. Either way, just a minor consideration.
Description
closes: #6975
The following is the simplest possible aliasing that preserves two different code paths for v1 and v2 packets.
The channel is converted into a v2 counterparty for the purpose of handling v2 packets. In this way, you can consider the connection and channel handshake as being very complicated ways to map the identifier
(portID, channelID)
to the client, and(cpPortId, cpChannelId)
to the client counterparty.This will allow v1 channels to immediately start using v2 packets while retaining the identifiers that they already use. This is critical for immediately enabling atomtic multi-packet data on existing transfer channels. As well as allowing existing channels to very easily upgrade their versions by simply sending a v2 packet with a new version rather than going through channel upgradability
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.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.