feat(proof): create a proto representation of the proof by vgonkivs · Pull Request #220 · celestiaorg/nmt

vgonkivs · 2023-07-14T08:57:41Z

Overview

Added and generated a proto representation of the Proof

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

codecov · 2023-07-14T08:59:30Z

Codecov Report

Merging #220 (4141b50) into master (3693c9a) will decrease coverage by 28.47%.
The diff coverage is 6.20%.

@@             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

Impacted Files	Coverage Δ
pb/proof.pb.go	`0.72% <0.72%> (ø)`
proof.go	`89.50% <100.00%> (+1.01%)`	⬆️

rootulp · 2023-07-14T13:49:51Z

pb/proof.proto

+message Proof {
+  int64 start     = 1;
+  int64 end       = 2;
+  repeated bytes nodes = 3;
+  bytes leafHash = 4;
+  bool isMaxNamespaceIDIgnored=5;
+}


[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

Hey @rootulp, thanks for the great suggestion. Added comments here 223b55a

rootulp · 2023-07-14T13:51:30Z

pb/proof.proto

+  int64 end       = 2;
+  repeated bytes nodes = 3;
+  bytes leafHash = 4;
+  bool isMaxNamespaceIDIgnored=5;


[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

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.

pb/proof.proto

staheri14

Thanks for the PR! I have added some comments.

staheri14 · 2023-07-14T16:45:39Z

pb/proof.proto

+  int64 end       = 2;
+  repeated bytes nodes = 3;
+  bytes leafHash = 4;
+  bool isMaxNamespaceIDIgnored=5;


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.

staheri14 · 2023-07-14T16:49:51Z

proof.go

 }

+// ProtoToProof creates a proof from its proto representation.
+func ProtoToProof(protoProof pb.Proof) Proof {


Don't we need an encoding counterpart i.e., ProofToProto?

The counterpart will look like a creation of a pb struct. I think it could be done in place(but can make a counterpart).

proof.go

Co-authored-by: Rootul P <rootulp@gmail.com>

rootulp

LGTM after lint fixes

proof_test.go

rootulp · 2023-07-17T13:48:37Z

@vgonkivs do you mind if we rename the PR title to feat(proof):... so that the squashed commit message conforms to conventional commits?

vgonkivs · 2023-07-17T14:23:27Z

Yes, please. You can name it whatever fits you.

Co-authored-by: Rootul P <rootulp@gmail.com>

staheri14

LGTM! I suggested an optional change, but I highly recommended it.

proof_test.go

rootulp

Still LGTM

staheri14 · 2023-07-18T17:31:55Z

It looks good to me, thanks for updating the test style @vgonkivs! my only concern is the failing checks in Snyk for which I cannot see the details. @rootulp can you see the details?

rootulp · 2023-07-18T18:53:52Z

Snyk claims this PR introduces 14 new vulnerabilities b/c the go.mod changes. I don't think we should block the PR on it though

staheri14 · 2023-07-18T19:17:07Z

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.

staheri14

LGTM!

vgonkivs requested review from evan-forbes, rootulp and staheri14 July 14, 2023 08:57

proof: create a proto representation of the proof

0c83717

vgonkivs force-pushed the add_proto_definition branch from 16791f9 to 0c83717 Compare July 14, 2023 09:06

rootulp previously approved these changes Jul 14, 2023

View reviewed changes

chore: add comments

223b55a

vgonkivs dismissed rootulp’s stale review via 223b55a July 14, 2023 14:35

vgonkivs requested a review from rootulp July 14, 2023 14:36

rootulp previously approved these changes Jul 14, 2023

View reviewed changes

pb/proof.proto Outdated Show resolved Hide resolved

pb/proof.proto Outdated Show resolved Hide resolved

staheri14 reviewed Jul 14, 2023

View reviewed changes

vgonkivs dismissed rootulp’s stale review via 46597ef July 17, 2023 08:59

apply suggestions

fe29c87

Co-authored-by: Rootul P <rootulp@gmail.com>

vgonkivs force-pushed the add_proto_definition branch from 46597ef to fe29c87 Compare July 17, 2023 09:07

chore: add tests

39c5918

vgonkivs requested review from rootulp and staheri14 July 17, 2023 09:16

rootulp previously approved these changes Jul 17, 2023

View reviewed changes

proof_test.go Outdated Show resolved Hide resolved

proof_test.go Outdated Show resolved Hide resolved

proof_test.go Outdated Show resolved Hide resolved

proof_test.go Outdated Show resolved Hide resolved

rootulp changed the title ~~proof: create a proto representation of the proof~~ feat(proof): create a proto representation of the proof Jul 17, 2023

rootulp assigned vgonkivs Jul 17, 2023

rootulp removed the request for review from evan-forbes July 17, 2023 13:49

rootulp added the enhancement New feature or request label Jul 17, 2023

fix: fix lint

b764dd1

vgonkivs dismissed rootulp’s stale review via b764dd1 July 17, 2023 14:36

apply suggestions

8382cad

Co-authored-by: Rootul P <rootulp@gmail.com>

vgonkivs requested a review from rootulp July 17, 2023 14:38

rootulp previously approved these changes Jul 17, 2023

View reviewed changes

staheri14 previously approved these changes Jul 17, 2023

View reviewed changes

proof_test.go Outdated Show resolved Hide resolved

chore: modify tests

4141b50

vgonkivs dismissed stale reviews from staheri14 and rootulp via 4141b50 July 18, 2023 09:50

vgonkivs requested review from rootulp and staheri14 July 18, 2023 09:51

rootulp approved these changes Jul 18, 2023

View reviewed changes

This was referenced Jul 18, 2023

Ignore proto generated files in codecover #222

Open

Document how to update Proto generated files #223

Open

Github workflow for proto-gen #224

Open

staheri14 mentioned this pull request Jul 18, 2023

Investigating Snyk complaints about go.mod #225

Closed

staheri14 approved these changes Jul 18, 2023

View reviewed changes

rootulp merged commit 9d22de9 into celestiaorg:master Jul 19, 2023

rootulp mentioned this pull request Aug 29, 2023

create proto rerpesentation for Proof #219

Closed

Conversation

vgonkivs commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checklist

Uh oh!

codecov bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rootulp Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

vgonkivs Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

rootulp Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

staheri14 Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

staheri14 left a comment

Choose a reason for hiding this comment

Uh oh!

staheri14 Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

staheri14 Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

vgonkivs Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rootulp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rootulp commented Jul 17, 2023

Uh oh!

vgonkivs commented Jul 17, 2023

Uh oh!

staheri14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rootulp left a comment

Choose a reason for hiding this comment

Uh oh!

staheri14 commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rootulp commented Jul 18, 2023

Uh oh!

staheri14 commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staheri14 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vgonkivs commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

staheri14 commented Jul 18, 2023 •

edited

Loading

staheri14 commented Jul 18, 2023 •

edited

Loading