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

README update & Solidity NatSpec comments #44

Merged

Conversation

tchataigner
Copy link
Member

Goal of this PR

The main goal of this PR was for me to dive in cryptographic block used in our Nova Verifier ported in Solidity.

To do so, I've refactored the README and tried to produce NAtSpec documentation for our Solidity files.

Changelog

  • Fixed typo for EqPolynomial.sol: EqPolinomialLib -> EqPolynomialLib
  • Added NatSpec comments in all *.sol files in src
  • Updated repository README.md to present repository structure, along with description and state of feature branches
  • Added README in src/blocks, src/blocks/grumpkin, src/blocks/pasta and src/blocks/poseidon to describe their content.

Related issue

N/A

storojs72 and others added 10 commits December 11, 2023 20:07
- Repository structure
- Overview of the different feature branches
- Fix NatSpec comment order over Pallas.sol and Vesta.sol
- Added Solidity comments over PoseidonNeptuneU24Optimized.sol and Sponge.sol
- Added README in poseidon folder
- Solidity doc for all cryptographic blocks file
- README in src/blocks to explain each file and their content
- Fixed typo in library name
Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

Thanks for such a diligent work, @tchataigner!

I haven't yet finished reviewing all NatSpec comments - but so far I have two common remarks (not directly related to the work itself):

  • For future PRs I would recommend to setup GPG commits signing
  • Your target branch is cleanup-main, which means it also picks some commits from there. I expect some more commits added from @samuelburnham (ci: Fail e2e tests when PR base is main #45), so eventually we will need to specifying the proper order of PRs to be merged

@tchataigner
Copy link
Member Author

tchataigner commented Dec 14, 2023

Yes! I guess the way to go would be merge @samuelburnham PR in #42 , i'll merge the commit in mine, there should be no conflict. And if we plan to merge #42 in main I can change the target branch of this PR. Otherwise, we can wait for this PR to be merge in cleanup-main before merging it.

Copy link
Member

@storojs72 storojs72 left a comment

Choose a reason for hiding this comment

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

I finished reviewing NatSpec. Thanks again for this work!

One thing, that I actually missed in #42 is moving Sponge library that supports optimised Poseidon. While reviewing this PR it has been discovered and the 512e8b5 has been added. So, it makes sense to probably add NatSpec to NovaSpongeLib library.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/blocks/grumpkin/Grumpkin.sol Outdated Show resolved Hide resolved
@tchataigner tchataigner merged commit 07d04f0 into lurk-lab:cleanup-main Dec 14, 2023
1 check passed
@tchataigner tchataigner deleted the feature/readme-doc-cleanup branch December 14, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants