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

Shallow clean-up of the solidity code #333

Merged
merged 18 commits into from
Dec 21, 2020
Merged

Conversation

AntoineRondelet
Copy link
Contributor

@AntoineRondelet AntoineRondelet commented Dec 17, 2020

Related to: #321 and #192

This PR:

  • Adjusts the Zeth solidity coding standards to rather align with the "language standards" proposed by the community and the solidity team. The driving factors for that are:
    • If there is a standard we should do our best to follow it (good practice) - it removes friction as to "what looks better" and just follows the norm.
    • Removes sources of inconsistencies between the code we write and the code we import (which catches the eye pretty badly when we call a function from an imported contract, like an ERC token for instance)
  • Renames all solidity constants in UPPER_SNAKE_CASE
  • Adds explicit scopes to all functions and storage variables (this should become systematic from now on as more verbosity with regard to the scopes can only be better)
  • Switches the linter to solhint (solium then replaced by ethlint seems to be poorly maintained, last release was more than a year ago. solhint is actively maintained offers a good configuration set and is used by openZeppelin (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/.solhint.json) and other serious projects). Several checks are voluntarily disabled for now because I didn't want to end up with a PR that shuffles the entire code base (renaming public functions, contract names etc => cascading these changes to the python code etc which will become a nightmare to review. Let's take it incrementally to avoid adding mistakes to the code base and breaking things)
  • Uses NatSpec for documentation comments
  • Orders functions (as per: https://docs.soliditylang.org/en/develop/style-guide.html#order-of-functions)
  • Fixes the order of the modifiers

TODO:

  • Fix the test_merkle_tree_contract.py test (we have a MerkleTreeMiMC7_test test contract but its import is broken since MerkleTreeMiMC7.sol does not exist anymore)
  • Fix the test_contract_base_mixer.py test. It is now broken because the scope of some functions (e.g. test_assemble_nullifiers) has been changed in this PR, so these functions are not in the ABI anymore. Since we should not expose unnecessary function simply to be able to call them in the test scripts, let's add a MixerBase_test.sol "test proxy contract" to re-enable the tests despite the functions visibility change. - Fixed in Contract tests fix (depends on #333) #338

@AntoineRondelet AntoineRondelet removed the request for review from dtebbs December 17, 2020 17:54
zeth_contracts/contracts/BLS12_377_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BLS12_377_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BLS12_377_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BLS12_377_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BLS12_377_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BW6_761_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BW6_761_test.sol Outdated Show resolved Hide resolved
zeth_contracts/contracts/BW6_761_test.sol Outdated Show resolved Hide resolved
@AntoineRondelet AntoineRondelet changed the title [WIP] Shallow clean-up of the solidity code Shallow clean-up of the solidity code Dec 17, 2020
@AntoineRondelet
Copy link
Contributor Author

@dtebbs there are some solidity variables that are seemingly unused. Could you double check this before we go further and consider removing them? (Are you using them somewhere I missed?)

@dtebbs
Copy link
Contributor

dtebbs commented Dec 17, 2020

@dtebbs there are some solidity variables that are seemingly unused. Could you double check this before we go further and consider removing them? (Are you using them somewhere I missed?)

@AntoineRondelet Yes, they do seem to unused. Good catch.

@AntoineRondelet AntoineRondelet changed the title Shallow clean-up of the solidity code [WIP] Shallow clean-up of the solidity code Dec 17, 2020
Copy link
Contributor

@dtebbs dtebbs left a comment

Choose a reason for hiding this comment

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

Looks good. Would be nice to have the CI catch violations. (I'll look into that)

zeth_contracts/.solhint.json Show resolved Hide resolved
zeth_contracts/package.json Outdated Show resolved Hide resolved
@dtebbs
Copy link
Contributor

dtebbs commented Dec 18, 2020

I've tried to address my comments in #334

@AntoineRondelet
Copy link
Contributor Author

Note: the test_contract_base_mixer.py test is broken. To properly fix it we would need to use a "test proxy contract", but this is for later (see #335). For now, let's just roll back and set the scopes of the tested functions back to public and update the tests to make sure that the tested functions behave correctly.

@dtebbs
Copy link
Contributor

dtebbs commented Dec 18, 2020

(Worth rebasing this onto origin/dvelop before merging)

@dtebbs dtebbs mentioned this pull request Dec 21, 2020
4 tasks
@dtebbs
Copy link
Contributor

dtebbs commented Dec 21, 2020

#338 fixes the tests and provides public wrappers around the internal functions (so no need to adjust this code)

@AntoineRondelet AntoineRondelet changed the title [WIP] Shallow clean-up of the solidity code Shallow clean-up of the solidity code Dec 21, 2020
AntoineRondelet added a commit that referenced this pull request Dec 21, 2020
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.

2 participants