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

refactor(contracts): add natspec across contracts and cleanup #898

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

ctrlc03
Copy link
Collaborator

@ctrlc03 ctrlc03 commented Dec 11, 2023

Currently, the smart contracts have incorrect and incomplete NatSpec comments. This PR adds NatSpec comments to ensure that automated documentation can be generated, and that developers have a better experience when working on the smart contracts. Furthemore, file structure has been re-organized for better accessibility

re #843

Copy link

netlify bot commented Dec 11, 2023

Deploy Preview for maci-typedoc ready!

Name Link
🔨 Latest commit a6ad25e
🔍 Latest deploy log https://app.netlify.com/sites/maci-typedoc/deploys/6579753cea45e60008d5d7d2
😎 Deploy Preview https://deploy-preview-898--maci-typedoc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ctrlc03 ctrlc03 self-assigned this Dec 11, 2023
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
/// @notice This contracts contains a number of structures
/// which are to be passed as parameters to Poll contracts.
/// This way we can reduce the number of parameters
/// and avoid a stack too deep error during compilation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically in the EVM there's a limit to how many local variables you can have (params and return values included) - so basically instead of passing 4 params for tree depths (as an example), we pass only one (the struct) - hope this makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's clearer when you explain it :)
then maybe it should be avoid a stack of deep errors during compilation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"stack too deep" is the actual error name, you can read more here btw

contracts/contracts/utilities/Utilities.sol Outdated Show resolved Hide resolved
@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Dec 12, 2023

@kittybest thank you for the thorough review, I made some (a lot) changes, could you please take a look again?

@ctrlc03 ctrlc03 requested a review from kittybest December 12, 2023 12:56
@ctrlc03 ctrlc03 force-pushed the chore/natspec branch 3 times, most recently from 3398bed to c2f612c Compare December 12, 2023 14:04
@samajammin samajammin added this to the MACI v1.1.1 refactor milestone Dec 12, 2023
@ctrlc03 ctrlc03 merged commit b3bfd13 into dev Dec 13, 2023
12 checks passed
@ctrlc03 ctrlc03 deleted the chore/natspec branch December 13, 2023 09:32
@ctrlc03 ctrlc03 mentioned this pull request Jan 5, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants