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

chore: updating VerifyMembership and VerifyNonMembership methods to use Path interface #2736

Merged
merged 4 commits into from
Nov 15, 2022
Merged

chore: updating VerifyMembership and VerifyNonMembership methods to use Path interface #2736

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
cfab769
updating VerifyMembership and VerifyNonMembership interfaces to use e…
damiannolan Nov 14, 2022
a965700
adding mock struct KeyPath to ibcmock testing pkg, adding additional …
damiannolan Nov 15, 2022
7ed12bd
Merge branch 'main' into damian/1884-remove-byte-slice-args
damiannolan Nov 15, 2022
6afddbd
Merge branch 'main' into damian/1884-remove-byte-slice-args
damiannolan Nov 15, 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
4 changes: 2 additions & 2 deletions modules/core/02-client/legacy/v100/solomachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func (cs *ClientState) VerifyMembership(
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path []byte,
path exported.Path,
value []byte,
) error {
panic("legacy solo machine is deprecated!")
Expand All @@ -242,7 +242,7 @@ func (cs *ClientState) VerifyNonMembership(
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path []byte,
path exported.Path,
) error {
panic("legacy solo machine is deprecated")
}
Expand Down
56 changes: 8 additions & 48 deletions modules/core/03-connection/keeper/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ func (k Keeper) VerifyClientState(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

bz, err := k.cdc.MarshalInterface(clientState)
if err != nil {
return err
Expand All @@ -54,7 +49,7 @@ func (k Keeper) VerifyClientState(
if err := targetClient.VerifyMembership(
ctx, clientStore, k.cdc, height,
0, 0, // skip delay period checks for non-packet processing verification
proof, path, bz,
proof, merklePath, bz,
); err != nil {
return sdkerrors.Wrapf(err, "failed client state verification for target client: %s", clientID)
}
Expand Down Expand Up @@ -90,11 +85,6 @@ func (k Keeper) VerifyClientConsensusState(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

bz, err := k.cdc.MarshalInterface(consensusState)
if err != nil {
return err
Expand All @@ -103,7 +93,7 @@ func (k Keeper) VerifyClientConsensusState(
if err := clientState.VerifyMembership(
ctx, clientStore, k.cdc, height,
0, 0, // skip delay period checks for non-packet processing verification
proof, path, bz,
proof, merklePath, bz,
); err != nil {
return sdkerrors.Wrapf(err, "failed consensus state verification for client (%s)", clientID)
}
Expand Down Expand Up @@ -139,11 +129,6 @@ func (k Keeper) VerifyConnectionState(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

connectionEnd, ok := counterpartyConnection.(connectiontypes.ConnectionEnd)
if !ok {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "invalid connection type %T", counterpartyConnection)
Expand All @@ -157,7 +142,7 @@ func (k Keeper) VerifyConnectionState(
if err := clientState.VerifyMembership(
ctx, clientStore, k.cdc, height,
0, 0, // skip delay period checks for non-packet processing verification
proof, path, bz,
proof, merklePath, bz,
); err != nil {
return sdkerrors.Wrapf(err, "failed connection state verification for client (%s)", clientID)
}
Expand Down Expand Up @@ -194,11 +179,6 @@ func (k Keeper) VerifyChannelState(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

channelEnd, ok := channel.(channeltypes.Channel)
if !ok {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "invalid channel type %T", channel)
Expand All @@ -212,7 +192,7 @@ func (k Keeper) VerifyChannelState(
if err := clientState.VerifyMembership(
ctx, clientStore, k.cdc, height,
0, 0, // skip delay period checks for non-packet processing verification
proof, path, bz,
proof, merklePath, bz,
); err != nil {
return sdkerrors.Wrapf(err, "failed channel state verification for client (%s)", clientID)
}
Expand Down Expand Up @@ -254,15 +234,10 @@ func (k Keeper) VerifyPacketCommitment(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

if err := clientState.VerifyMembership(
ctx, clientStore, k.cdc, height,
timeDelay, blockDelay,
proof, path, commitmentBytes,
proof, merklePath, commitmentBytes,
); err != nil {
return sdkerrors.Wrapf(err, "failed packet commitment verification for client (%s)", clientID)
}
Expand Down Expand Up @@ -304,15 +279,10 @@ func (k Keeper) VerifyPacketAcknowledgement(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

if err := clientState.VerifyMembership(
ctx, clientStore, k.cdc, height,
timeDelay, blockDelay,
proof, path, channeltypes.CommitAcknowledgement(acknowledgement),
proof, merklePath, channeltypes.CommitAcknowledgement(acknowledgement),
); err != nil {
return sdkerrors.Wrapf(err, "failed packet acknowledgement verification for client (%s)", clientID)
}
Expand Down Expand Up @@ -354,15 +324,10 @@ func (k Keeper) VerifyPacketReceiptAbsence(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

if err := clientState.VerifyNonMembership(
ctx, clientStore, k.cdc, height,
timeDelay, blockDelay,
proof, path,
proof, merklePath,
); err != nil {
return sdkerrors.Wrapf(err, "failed packet receipt absence verification for client (%s)", clientID)
}
Expand Down Expand Up @@ -403,15 +368,10 @@ func (k Keeper) VerifyNextSequenceRecv(
return err
}

path, err := k.cdc.Marshal(&merklePath)
if err != nil {
return err
}

if err := clientState.VerifyMembership(
ctx, clientStore, k.cdc, height,
timeDelay, blockDelay,
proof, path, sdk.Uint64ToBigEndian(nextSequenceRecv),
proof, merklePath, sdk.Uint64ToBigEndian(nextSequenceRecv),
); err != nil {
return sdkerrors.Wrapf(err, "failed next sequence receive verification for client (%s)", clientID)
}
Expand Down
4 changes: 2 additions & 2 deletions modules/core/exported/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ type ClientState interface {
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path []byte,
path Path,
value []byte,
) error

Expand All @@ -88,7 +88,7 @@ type ClientState interface {
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path []byte,
path Path,
) error

// VerifyClientMessage must verify a ClientMessage. A ClientMessage could be a Header, Misbehaviour, or batch update.
Expand Down
16 changes: 8 additions & 8 deletions modules/light-clients/06-solomachine/client_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,17 @@ func (cs *ClientState) VerifyMembership(
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path []byte,
path exported.Path,
value []byte,
) error {
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
if err != nil {
return err
}

var merklePath commitmenttypes.MerklePath
if err := cdc.Unmarshal(path, &merklePath); err != nil {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal path into ICS 23 commitment merkle path")
merklePath, ok := path.(commitmenttypes.MerklePath)
if !ok {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", commitmenttypes.MerklePath{}, path)
}
Comment on lines +123 to 126
Copy link
Member Author

@damiannolan damiannolan Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Code coverage both here and in 07-tendermint will likely be reduced. I can cover the error case here by adding a mock path type to the tests like so:

type mockPath struct{}

func (mockPath) String() string {
	return ""
}

func (mockPath) Empty() bool {
	return false
}

This would fulfil the exported.Path interface but trigger the error here when converting to commitmenttypes.MerklePath

Copy link
Member Author

Choose a reason for hiding this comment

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

fwiw I don't feel super strongly about covering this case...

Copy link
Contributor

Choose a reason for hiding this comment

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

No preferences, probably doesn't hurt to add. Hopefully one day it can be removed. I'm fine if it is skipped as well


signBytes := &SignBytes{
Expand Down Expand Up @@ -159,16 +159,16 @@ func (cs *ClientState) VerifyNonMembership(
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path []byte,
path exported.Path,
) error {
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
if err != nil {
return err
}

var merklePath commitmenttypes.MerklePath
if err := cdc.Unmarshal(path, &merklePath); err != nil {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal path into ICS 23 commitment merkle path")
merklePath, ok := path.(commitmenttypes.MerklePath)
if !ok {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", commitmenttypes.MerklePath{}, path)
}

signBytes := &SignBytes{
Expand Down
Loading