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: change counterparty structure. #7328

Merged
merged 4 commits into from
Sep 30, 2024
Merged

refactor: change counterparty structure. #7328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88d0c26
refactor: change counterparty structure.
DimitrisJim Sep 24, 2024
3c83fba
refactor: add channel id to Counterparty.
DimitrisJim Sep 24, 2024
d00541e
refactor: use client identifier in relay functions.
DimitrisJim Sep 25, 2024
e776c95
chore: suffix _id to identifier.
DimitrisJim Sep 30, 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
10 changes: 5 additions & 5 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (k *Keeper) IBCSoftwareUpgrade(goCtx context.Context, msg *clienttypes.MsgI
func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *packetservertypes.MsgProvideCounterparty) (*packetservertypes.MsgProvideCounterpartyResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

creator, found := k.ClientKeeper.GetCreator(ctx, msg.ClientId)
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference/recommendation would be to use the authority naming rather than "creator"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't that be more confusing? If used I'd assume the module's authority address should be what we're comparing with after (instead of msg.Signer). Either case, happy if issue is opened and discussed afterwards?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe its confusing intertwine terminology used for module permissions and client creation.

The way I thought about it was: msg.Signer creates a client, thus they have the authority to bind a counterparty to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. SetClientAuthority(ctx, clientID, msg.Signer) && GetClientAuthority(ctx, clientID)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no issue with me, if your pref is strong, I say we open issue and slam it through in follow up.

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 quite sure how strong my pref is! 😆

Copy link
Member

Choose a reason for hiding this comment

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

I find creator clearer personally. but maybe that's my own mental model. Don't have a strong preference

creator, found := k.ClientKeeper.GetCreator(ctx, msg.Counterparty.ClientId)
if !found {
return nil, errorsmod.Wrap(ibcerrors.ErrUnauthorized, "client creator must be set")
}
Expand All @@ -157,13 +157,13 @@ func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *packetservertyp
return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "client creator (%s) must match signer (%s)", creator, msg.Signer)
}

if _, ok := k.PacketServerKeeper.GetCounterparty(ctx, msg.ClientId); ok {
return nil, errorsmod.Wrapf(packetservertypes.ErrInvalidCounterparty, "counterparty already exists for client %s", msg.ClientId)
if _, ok := k.PacketServerKeeper.GetCounterparty(ctx, msg.ChannelId); ok {
return nil, errorsmod.Wrapf(packetservertypes.ErrInvalidCounterparty, "counterparty already exists for client %s", msg.ChannelId)
}

k.PacketServerKeeper.SetCounterparty(ctx, msg.ClientId, msg.Counterparty)
k.PacketServerKeeper.SetCounterparty(ctx, msg.ChannelId, msg.Counterparty)
// Delete client creator from state as it is not needed after this point.
k.ClientKeeper.DeleteCreator(ctx, msg.ClientId)
k.ClientKeeper.DeleteCreator(ctx, msg.Counterparty.ClientId)

return &packetservertypes.MsgProvideCounterpartyResponse{}, nil
}
Expand Down
4 changes: 2 additions & 2 deletions modules/core/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,7 +1222,7 @@ func (suite *KeeperTestSuite) TestProvideCounterparty() {
{
"failure: unknown client identifier",
func() {
msg.ClientId = ibctesting.InvalidID
msg.Counterparty.ClientId = ibctesting.InvalidID
},
ibcerrors.ErrUnauthorized,
},
Expand All @@ -1237,7 +1237,7 @@ func (suite *KeeperTestSuite) TestProvideCounterparty() {
"failure: counterparty already exists",
func() {
// set it before handler
suite.chainA.App.GetIBCKeeper().PacketServerKeeper.SetCounterparty(suite.chainA.GetContext(), msg.ClientId, msg.Counterparty)
suite.chainA.App.GetIBCKeeper().PacketServerKeeper.SetCounterparty(suite.chainA.GetContext(), msg.ChannelId, msg.Counterparty)
},
packetservertypes.ErrInvalidCounterparty,
},
Expand Down
4 changes: 2 additions & 2 deletions modules/core/packet-server/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ The [counterparty-merkle-path-prefix] is a comma-separated list of hex-encoded s
return err
}

counterparty := types.NewCounterparty(counterpartyClientIdentifier, counterpartyMerklePathPrefix)
counterparty := types.NewCounterparty(clientIdentifier, counterpartyClientIdentifier, counterpartyMerklePathPrefix)
msg := types.MsgProvideCounterparty{
ClientId: clientIdentifier,
ChannelId: clientIdentifier,
Counterparty: counterparty,
Signer: clientCtx.GetFromAddress().String(),
}
Expand Down
31 changes: 18 additions & 13 deletions modules/core/packet-server/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ func (k Keeper) SendPacket(
if !ok {
return 0, errorsmod.Wrap(types.ErrCounterpartyNotFound, sourceChannel)
}
destChannel := counterparty.ClientId
destChannel := counterparty.CounterpartyChannelId
clientID := counterparty.ClientId

// retrieve the sequence send for this channel
// if no packets have been sent yet, initialize the sequence to 1.
Expand All @@ -54,17 +55,17 @@ func (k Keeper) SendPacket(
}

// check that the client of counterparty chain is still active
if status := k.ClientKeeper.GetClientStatus(ctx, sourceChannel); status != exported.Active {
return 0, errorsmod.Wrapf(clienttypes.ErrClientNotActive, "client (%s) status is %s", sourceChannel, status)
if status := k.ClientKeeper.GetClientStatus(ctx, clientID); status != exported.Active {
return 0, errorsmod.Wrapf(clienttypes.ErrClientNotActive, "client (%s) status is %s", clientID, status)
}

// retrieve latest height and timestamp of the client of counterparty chain
latestHeight := k.ClientKeeper.GetClientLatestHeight(ctx, sourceChannel)
latestHeight := k.ClientKeeper.GetClientLatestHeight(ctx, clientID)
if latestHeight.IsZero() {
return 0, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", sourceChannel)
return 0, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", clientID)
}

latestTimestamp, err := k.ClientKeeper.GetClientTimestampAtHeight(ctx, sourceChannel, latestHeight)
latestTimestamp, err := k.ClientKeeper.GetClientTimestampAtHeight(ctx, clientID, latestHeight)
if err != nil {
return 0, err
}
Expand Down Expand Up @@ -113,9 +114,11 @@ func (k Keeper) RecvPacket(
if !ok {
return "", errorsmod.Wrap(types.ErrCounterpartyNotFound, packet.DestinationChannel)
}
if counterparty.ClientId != packet.SourceChannel {

if counterparty.CounterpartyChannelId != packet.SourceChannel {
return "", channeltypes.ErrInvalidChannelIdentifier
}
clientID := counterparty.ClientId

// check if packet timed out by comparing it with the latest height of the chain
sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/5917
Expand Down Expand Up @@ -144,14 +147,14 @@ func (k Keeper) RecvPacket(

if err := k.ClientKeeper.VerifyMembership(
ctx,
packet.DestinationChannel,
clientID,
proofHeight,
0, 0,
proof,
merklePath,
commitment,
); err != nil {
return "", errorsmod.Wrapf(err, "failed packet commitment verification for client (%s)", packet.DestinationChannel)
return "", errorsmod.Wrapf(err, "failed packet commitment verification for client (%s)", clientID)
}

// Set Packet Receipt to prevent timeout from occurring on counterparty
Expand Down Expand Up @@ -248,6 +251,7 @@ func (k Keeper) AcknowledgePacket(
if counterparty.ClientId != packet.DestinationChannel {
return "", channeltypes.ErrInvalidChannelIdentifier
}
clientID := counterparty.ClientId

commitment := k.ChannelKeeper.GetPacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)
if len(commitment) == 0 {
Expand All @@ -272,14 +276,14 @@ func (k Keeper) AcknowledgePacket(

if err := k.ClientKeeper.VerifyMembership(
ctx,
packet.SourceChannel,
clientID,
proofHeight,
0, 0,
proofAcked,
merklePath,
channeltypes.CommitAcknowledgement(acknowledgement),
); err != nil {
return "", errorsmod.Wrapf(err, "failed packet acknowledgement verification for client (%s)", packet.SourceChannel)
return "", errorsmod.Wrapf(err, "failed packet acknowledgement verification for client (%s)", clientID)
}

k.ChannelKeeper.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)
Expand Down Expand Up @@ -318,6 +322,7 @@ func (k Keeper) TimeoutPacket(
if counterparty.ClientId != packet.DestinationChannel {
return "", channeltypes.ErrInvalidChannelIdentifier
}
clientID := counterparty.ClientId

// check that timeout height or timeout timestamp has passed on the other end
proofTimestamp, err := k.ClientKeeper.GetClientTimestampAtHeight(ctx, packet.SourceChannel, proofHeight)
Expand Down Expand Up @@ -354,13 +359,13 @@ func (k Keeper) TimeoutPacket(

if err := k.ClientKeeper.VerifyNonMembership(
ctx,
packet.SourceChannel,
clientID,
proofHeight,
0, 0,
proof,
merklePath,
); err != nil {
return "", errorsmod.Wrapf(err, "failed packet receipt absence verification for client (%s)", packet.SourceChannel)
return "", errorsmod.Wrapf(err, "failed packet receipt absence verification for client (%s)", clientID)
}

// delete packet commitment to prevent replay
Expand Down
11 changes: 8 additions & 3 deletions modules/core/packet-server/types/counterparty.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import (
)

// NewCounterparty creates a new Counterparty instance
func NewCounterparty(clientID string, merklePathPrefix commitmenttypes.MerklePath) Counterparty {
func NewCounterparty(clientID, counterpartyChannelID string, merklePathPrefix commitmenttypes.MerklePath) Counterparty {
return Counterparty{
ClientId: clientID,
MerklePathPrefix: merklePathPrefix,
ClientId: clientID,
CounterpartyChannelId: counterpartyChannelID,
MerklePathPrefix: merklePathPrefix,
}
}

Expand All @@ -21,6 +22,10 @@ func (c Counterparty) Validate() error {
return err
}

if err := host.ChannelIdentifierValidator(c.CounterpartyChannelId); err != nil {
return err
}

if err := c.MerklePathPrefix.ValidateAsPrefix(); err != nil {
return errorsmod.Wrap(ErrInvalidCounterparty, err.Error())
}
Expand Down
102 changes: 79 additions & 23 deletions modules/core/packet-server/types/counterparty.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading