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.

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

[Documentation] Add clarifying comment to ClosestProof documentation #32

Merged
merged 5 commits into from
Dec 22, 2023

Conversation

h5law
Copy link
Collaborator

@h5law h5law commented Dec 22, 2023

Summary

Quick PR to add context to the closest proof method

Issue

Fixes N/A

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make test_all
  • Run all/relevant benchmarks (if optimising): make benchmark_{all | suite name}

Required Checklist

If Applicable Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated any relevant README(s)/documentation and left TODOs throughout the codebase
  • Add or update any relevant or supporting mermaid diagrams

@h5law h5law added the documentation Improvements or additions to documentation label Dec 22, 2023
@h5law h5law requested a review from Olshansk December 22, 2023 12:06
@h5law h5law self-assigned this Dec 22, 2023
@reviewpad reviewpad bot added small Pull request is small waiting-for-review This PR is currently waiting to be reviewed labels Dec 22, 2023
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (12d9ac6) 83.28% compared to head (4a984b1) 83.28%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   83.28%   83.28%           
=======================================
  Files           8        8           
  Lines        1406     1406           
=======================================
  Hits         1171     1171           
  Misses        178      178           
  Partials       57       57           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/SMT.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

👆

docs/SMT.md Outdated Show resolved Hide resolved
docs/SMT.md Outdated Show resolved Hide resolved
docs/SMT.md Outdated Show resolved Hide resolved
docs/SMT.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Sorry for continuing to push on such a small thing but I think we're deriving more value by thinking it through.

@h5law h5law force-pushed the feat/closest-proof-docs branch from cdde3cb to 35fbc54 Compare December 22, 2023 16:12
docs/SMT.md Outdated Show resolved Hide resolved
docs/SMT.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

❤️

@h5law h5law merged commit d88ea55 into main Dec 22, 2023
7 checks passed
@h5law h5law deleted the feat/closest-proof-docs branch December 22, 2023 17:22
@Olshansk
Copy link
Member

@h5law This is a good addition and on a technical level contains all the details.

However, it's a lot of text & content to parse. For someone with context or who is
willing to sit, think and read this many times, they'll understand. For 99% of OSS
contributors who will be too lazy (think about how many times we dive deep into other's
documentation), I think that bullet points + diagrams will make it easier to understand.

Even I feel like I need to read this multiple times and need to make notes along the way,
and the extra effort to make it easier to read will pay dividends down the road.

If you have time, I recommend approaching it like so.

## ClosestProof Use-Cases

The `CloestProof` function is intended for use in `commit & reveal` mechanism.

_NOTE: Throughout this document, `commitment` of the the tree's hashes is also referred to as closing the tree._

Take the following attack vector (**without** a commit prior to a reveal) into consideration:
1. The prover identifies the hash of a leaf they want to prove
2. The prover (prior to committing) inserts the hash ...
3. ...

Take the following normal flow (**with** a commit prior to reveal) as
1. ...
2. 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation small Pull request is small waiting-for-review This PR is currently waiting to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants