imp: cleanup verifcation arg code for 23-commitment #7493
Conversation
| @@ -234,16 +226,8 @@ func (proof *MerkleProof) Empty() bool { | |||
| return proof == nil || proto.Equal(proof, blankMerkleProof) || proto.Equal(proof, blankProofOps) | |||
There was a problem hiding this comment.
blankProofOps check? This can be deleted right?? I don't understand the context of when one could provide that type as MerkleProof?
There was a problem hiding this comment.
Agree, doesn't seem to make sense to me
| // validateVerificationArgs verifies the proof arguments are valid | ||
| func (proof MerkleProof) validateVerificationArgs(specs []*ics23.ProofSpec, root exported.Root) error { | ||
| func validateVerificationArgs(proof MerkleProof, path v2.MerklePath, specs []*ics23.ProofSpec, root exported.Root) error { | ||
| if proof.Empty() { |
There was a problem hiding this comment.
maybe this should be if proof == nil || proof.GetProofs() == nil. I don't feel like the proof.Empty() is very effective and creates extra space in this file
There was a problem hiding this comment.
Could also just call this Validate(specs) as method on merkle proof and move root nil checks to where its being used.
This is also totally fine, and happy to move along with it! 🙏🏻
There was a problem hiding this comment.
made the change, but left off the Validate(specs) suggestion. Happy to approve a followup, but wasn't exactly sure what changes you had in mind
colin-axner
requested review from
AdityaSripal,
damiannolan and
chatton
as code owners
| @@ -234,16 +226,8 @@ func (proof *MerkleProof) Empty() bool { | |||
| return proof == nil || proto.Equal(proof, blankMerkleProof) || proto.Equal(proof, blankProofOps) | |||
There was a problem hiding this comment.
Agree, doesn't seem to make sense to me
| // validateVerificationArgs verifies the proof arguments are valid | ||
| func (proof MerkleProof) validateVerificationArgs(specs []*ics23.ProofSpec, root exported.Root) error { | ||
| func validateVerificationArgs(proof MerkleProof, path v2.MerklePath, specs []*ics23.ProofSpec, root exported.Root) error { | ||
| if proof.Empty() { |
| if len(specs) != len(proof.Proofs) { | ||
| return errorsmod.Wrapf(ErrInvalidMerkleProof, | ||
| "length of specs: %d not equal to length of proof: %d", | ||
| len(specs), len(proof.Proofs)) | ||
| return errorsmod.Wrapf(ErrInvalidMerkleProof, "length of specs: %d not equal to length of proof: %d", len(specs), len(proof.Proofs)) | ||
| } | ||
|
|
||
| if len(path.KeyPath) != len(specs) { | ||
| return errorsmod.Wrapf(ErrInvalidProof, "path length %d not same as proof %d", len(path.KeyPath), len(specs)) |
There was a problem hiding this comment.
Maybe we could indicate in godoc why this is. e.g. key path of length > 1 is expected to be a chained proof.
Every path element inside path.KeyPath is expected to have a corresponding proofSpec, right? In the normal case first we calculate subroots for inclusion/non-inclusion and then inclusion of the subroot in commitment root.
Only in the case of path.KeyPath == 1, would we be able to do a single inclusion or non-inclusion proof.
There was a problem hiding this comment.
Correct. This code is expecting to receive a list of keys which corresponds to different trees (which is why it must be a list of specs, 1 per tree)
| // validateVerificationArgs verifies the proof arguments are valid | ||
| func (proof MerkleProof) validateVerificationArgs(specs []*ics23.ProofSpec, root exported.Root) error { | ||
| func validateVerificationArgs(proof MerkleProof, path v2.MerklePath, specs []*ics23.ProofSpec, root exported.Root) error { | ||
| if proof.Empty() { |
There was a problem hiding this comment.
Could also just call this Validate(specs) as method on merkle proof and move root nil checks to where its being used.
This is also totally fine, and happy to move along with it! 🙏🏻
…o into colin/23-commitment-cleanup
|
Description
couldn't help myself 😄
closes: #XXXX
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.