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

audit: Better Storage and Struct Packing Possible #204

Merged
merged 4 commits into from
Jun 15, 2023

Conversation

Orland0x
Copy link
Contributor

closes #177

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #204 (1fd3446) into main (ea3d686) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #204   +/-   ##
=======================================
  Coverage   82.15%   82.15%           
=======================================
  Files          29       29           
  Lines         454      454           
  Branches      114      114           
=======================================
  Hits          373      373           
  Misses         65       65           
  Partials       16       16           
Impacted Files Coverage Δ
src/Space.sol 96.21% <ø> (ø)
src/voting-strategies/WhitelistVotingStrategy.sol 100.00% <ø> (ø)

@Orland0x Orland0x requested a review from pscott June 15, 2023 09:50
Copy link
Collaborator

@pscott pscott left a comment

Choose a reason for hiding this comment

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

tACK

not as much gas decrease as I was expecting :p but that's ok

150029
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀

51840
Copy link
Collaborator

Choose a reason for hiding this comment

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

erm sir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are outdated

Comment on lines -12 to -14
// We store the following 3 timestamps for each proposal despite the fact that they can be
// inferred from the votingDelay, minVotingDuration, and maxVotingDuration state variables
// because those variables may be updated during the lifetime of a proposal.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should keep that in the code as it's a useful comment for the reader imho

@@ -16,7 +16,7 @@ contract WhitelistVotingStrategy is IVotingStrategy {
// The address of the member.
address addr;
// The voting power of the member.
uint256 vp;
uint96 vp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this was mentioned in the audit but I don't know how I feel about it. We could imagine some people doing a token + whitelist strategy, and having like 2^100 tokens each. In which case, the whitelist would be too limited...

I guess this is an edge case scenario and 99% of the time, uint96 will do the work so it's fine

Copy link
Contributor Author

@Orland0x Orland0x Jun 15, 2023

Choose a reason for hiding this comment

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

yeah, always possible to just write a new strat too if you really need something specific

src/types.sol Show resolved Hide resolved
@Orland0x Orland0x merged commit c3c173f into main Jun 15, 2023
@Orland0x Orland0x deleted the audit_struct_packing branch October 11, 2023 10:37
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.

audit: Better Storage and Struct Packing Possible
2 participants