-
Notifications
You must be signed in to change notification settings - Fork 623
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
feat: add rpc VerifyMembershipProof
- querier approach for conditional clients
#5821
Changes from 8 commits
7388af9
18e5404
1f720b6
37435a6
2569bd4
06d2f59
a2fc9a7
a2f139d
9cc6cdc
b72e541
c19d6ed
889be6e
7ef470e
e64af3a
da3b02e
143d7c9
1ecb660
83fbeec
8396c1c
294bacf
41b6e5c
00f59e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -328,3 +328,54 @@ func (k Keeper) UpgradedConsensusState(c context.Context, req *types.QueryUpgrad | |
UpgradedConsensusState: protoAny, | ||
}, nil | ||
} | ||
|
||
// VerifyMembershipProof implements the Query/VerifyMembershipProof gRPC method | ||
func (k Keeper) VerifyMembershipProof(c context.Context, req *types.QueryVerifyMembershipProofRequest) (*types.QueryVerifyMembershipProofResponse, error) { | ||
if req == nil { | ||
return nil, status.Error(codes.InvalidArgument, "empty request") | ||
} | ||
|
||
if err := host.ClientIdentifierValidator(req.ClientId); err != nil { | ||
return nil, status.Error(codes.InvalidArgument, err.Error()) | ||
} | ||
|
||
if len(req.Proof) == 0 { | ||
return nil, status.Error(codes.InvalidArgument, "empty proof") | ||
} | ||
|
||
if req.ProofHeight.IsZero() { | ||
return nil, status.Error(codes.InvalidArgument, "proof height must be non-zero") | ||
} | ||
|
||
Comment on lines
+348
to
+351
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think proof height is allowed to be empty? (for localhost?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we disable localhost then we should be sweet? :) |
||
if req.MerklePath.Empty() { | ||
return nil, status.Error(codes.InvalidArgument, "empty merkle path") | ||
} | ||
|
||
if len(req.Value) == 0 { | ||
return nil, status.Error(codes.InvalidArgument, "empty value") | ||
} | ||
|
||
ctx := sdk.UnwrapSDKContext(c) | ||
// cache the context to ensure clientState.VerifyMembership does not change state | ||
cachedCtx, _ := ctx.CacheContext() | ||
|
||
// make sure we charge the higher level context even on panic | ||
defer func() { | ||
ctx.GasMeter().ConsumeGas(cachedCtx.GasMeter().GasConsumed(), "verify membership query") | ||
}() | ||
|
||
clientState, found := k.GetClientState(cachedCtx, req.ClientId) | ||
if !found { | ||
return nil, status.Error(codes.NotFound, errorsmod.Wrap(types.ErrClientNotFound, req.ClientId).Error()) | ||
} | ||
|
||
if err := clientState.VerifyMembership(ctx, k.ClientStore(cachedCtx, req.ClientId), k.cdc, req.ProofHeight, req.TimeDelay, req.BlockDelay, req.Proof, req.MerklePath, req.Value); err != nil { | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return &types.QueryVerifyMembershipProofResponse{ | ||
Result: false, | ||
}, nil | ||
} | ||
DimitrisJim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return &types.QueryVerifyMembershipProofResponse{ | ||
Result: true, | ||
}, nil | ||
} |
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 we should error if the client type is solo machine?
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.
localhost as well?
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.
do we also want to error if the client is not active, i.e.
k.GetClientStatus(cachedCtx, clientState, req.ClientId) != exported.Active
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 would involve adding solomachine types as a concrete dependency in 02-client which I think is currently not the case, (it is already with localhost though).
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.
Why would we error if it is solomachine client type? State changes are discarded so no sequence incrementing will happen. Shouldn't it be fine to let it go through, just sig verify on whatever bytes? Is it that solomachine is only signing one particular set of bytes for a one sequence? i.e. you will be able to verify for example, a channel end signed at sequence x? so in theory the
VerifyMembership
query is only available for proof one value at a particular point in time. Does that make sense?Yeah, if you query a chain for a proof and have access to an rpc for a header to get the app hash then you should be able to verify off chain and it would be spammy(?) I guess to allow a query to go through like that. Fee like there's no reason to allow it.
Solomachine client type is in
exported
afaik.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.
Created this issue: #5848
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.
solomachine client type is a string, it can be duplicated
yes it is.