Contracts cleanup #361
Merged
Contracts cleanup #361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@AntoineRondelet once we'v confirmed that #362 builds correctly, rebasing on to that should fix the CI error here. |
dtebbs
approved these changes
Apr 1, 2021
AntoineRondelet
force-pushed
the
contracts-cleanup
branch
from
4e32a14
to
026233d
Compare
|
See also: #335 |
AntoineRondelet
force-pushed
the
contracts-cleanup
branch
from
28c329c
to
6d96302
Compare
|
This branch contains all the solidity changes. Not sure why some of the "file renamings" do not appear as such and are rather treated a deleted files + new files... this makes this PR extremely annoying to review. Probably did something fishy here, or maybe the set of changes on the files has reached the "diff threshold" above which git doesn't detect "renames" anymore (se e.g. https://chelseatroy.com/2020/05/09/question-how-does-git-detect-renames/), not sure. |
Co-authored-by: Duncan Tebbs <dtebbs@users.noreply.github.com>
AntoineRondelet
force-pushed
the
contracts-cleanup
branch
from
c671041
to
93e36c6
Compare
This was referenced May 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor the solidity code to comply with the styling guide of the language and added more restrictions on function visibility and state mutation (which also allows to save gas by reading straight in the calldata instead of having the args copied).
Also removed
MerkleTreeSha256.solwhich wasn't used anymore and wasn't compiling.Ran
slither(https://github.com/crytic/slither) on the contracts. We're not fully compliant (still some contracts names that aren't fully in Caps, some variables that are unused in inherited contracts, uninitialized variables and ignored returns to only name a few) but the set of flags raised aren't very worrying when we dig into the code (the ignoredreturnvalues are alwaystrueand come from the ERC standard etc etc). We can surely make the whole set of contracts tick all boxes but that will take a bit more time and will affect the perfs if we want to move away from low level calls and assembly - which isn't desired for now.[EDIT] Slither doesn't seem to fully support the latest versions of Solidity (i.e. not above 0.7), so using the tool as part of the CI with the current code base (in Solidity
0.8) isn't possible for now. Running the tool manually can be done however, with a few tricks to remove syntax not yet supported (e.g.vk.slot) by modifying the code (e.g.mstore(pad, vk.slot)->mstore(pad, 0x20)to satisfy the tool and get reports for all other "Detectors")[EDIT 2]: The full 0.7 syntax (including
.slotand.offset) will soon be supported (see: crytic/slither#831 for more details)