-
Notifications
You must be signed in to change notification settings - Fork 39
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(proof): create a proto representation of the proof #220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
===========================================
- Coverage 93.67% 65.20% -28.47%
===========================================
Files 5 6 +1
Lines 601 891 +290
===========================================
+ Hits 563 581 +18
- Misses 21 293 +272
Partials 17 17
|
16791f9
to
0c83717
Compare
pb/proof.proto
Outdated
message Proof { | ||
int64 start = 1; | ||
int64 end = 2; | ||
repeated bytes nodes = 3; | ||
bytes leafHash = 4; | ||
bool isMaxNamespaceIDIgnored=5; | ||
} |
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.
[optional] doc comments on what each field does would be super helpful. Content from the specs may help https://github.com/celestiaorg/nmt/blob/master/docs/spec/nmt.md#namespace-proof
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.
pb/proof.proto
Outdated
int64 end = 2; | ||
repeated bytes nodes = 3; | ||
bytes leafHash = 4; | ||
bool isMaxNamespaceIDIgnored=5; |
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.
[not blocking] Related: #206 . Once we make this a protobuf field, I wonder if renaming it would be breaking. I think renaming shouldn't block this PR but wanted to call it out. cc: @staheri14
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.
Thanks @rootulp for this suggestion. I also agree that in line with #206, it would be good to change this message field to isMaxNamespaceIgnored
. I don't think it has to match the same field in the Proof
struct, so it should be fine to use isMaxNamespaceIgnored
instead.
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.
Thanks for the PR! I have added some comments.
pb/proof.proto
Outdated
int64 end = 2; | ||
repeated bytes nodes = 3; | ||
bytes leafHash = 4; | ||
bool isMaxNamespaceIDIgnored=5; |
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.
Thanks @rootulp for this suggestion. I also agree that in line with #206, it would be good to change this message field to isMaxNamespaceIgnored
. I don't think it has to match the same field in the Proof
struct, so it should be fine to use isMaxNamespaceIgnored
instead.
@@ -453,6 +454,30 @@ func (proof Proof) VerifyInclusion(h hash.Hash, nid namespace.ID, leavesWithoutN | |||
return res | |||
} | |||
|
|||
// ProtoToProof creates a proof from its proto representation. | |||
func ProtoToProof(protoProof pb.Proof) Proof { |
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.
Don't we need an encoding counterpart i.e., ProofToProto
?
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 counterpart will look like a creation of a pb struct. I think it could be done in place(but can make a counterpart).
Co-authored-by: Rootul P <rootulp@gmail.com>
46597ef
to
fe29c87
Compare
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.
LGTM after lint fixes
@vgonkivs do you mind if we rename the PR title to |
Yes, please. You can name it whatever fits you. |
Co-authored-by: Rootul P <rootulp@gmail.com>
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.
LGTM! I suggested an optional change, but I highly recommended it.
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.
Still LGTM
Snyk claims this PR introduces 14 new vulnerabilities b/c the |
Thanks @rootulp for looking into this, I have created the tracking issue, feel free to add a snapshot of the Snyk errors in the issue for the record. |
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.
LGTM!
Overview
Added and generated a proto representation of the Proof
Checklist