fix: Prevent NOPs on allacation and resetting #82
Conversation
|
|
||
| // Explicit >= 0 checks for all values since we reset values below | ||
| _requireNoNegatives(_absoluteLQTYVotes); | ||
| _requireNoNegatives(_absoluteLQTYVetos); | ||
| // If the goal is to remove all votes from an initiative, including in _initiativesToReset is enough | ||
| _requireNoNOP(_absoluteLQTYVotes, _absoluteLQTYVetos); |
There was a problem hiding this comment.
Technically, this renders the previous _requireNoNegatives() checks redundant, although we might want to revise the name and revert message of _requireNoNOP() if we eliminate them.
There was a problem hiding this comment.
Why? _requireNoNOP doesn’t prevent a negative value, only that for each pair at least one is positive.
Anyway, I was thinking we would remove _requireNoNegatives when we change the signature int88 to unsigned. It’s weird to call them absolute but allow them to be negative to then check they are not.
There was a problem hiding this comment.
Ah yes, I forgot it's ||.
Agree the interface should be unsigned.
| @@ -5,6 +5,8 @@ pragma solidity ^0.8.24; | |||
| /// @param arr - List to check for dups | |||
| function _requireNoDuplicates(address[] calldata arr) pure { | |||
| uint256 arrLength = arr.length; | |||
| if (arrLength == 0) return; | |||
test/BribeInitiative.t.sol
Outdated
| @@ -929,6 +936,30 @@ contract BribeInitiativeTest is Test, MockStakingV1Deployer { | |||
| vm.stopPrank(); | |||
| } | |||
|
|
|||
| function _allocateNothing(address staker) internal { | |||
There was a problem hiding this comment.
Maybe _tryAllocateNothing() to emphasize that it's expected to fail? Or something along those lines.
Just reading the test cases, my first thought was: "how is _allocateNothing() different from _resetAllocation()?"
No description provided.