Code review via comments for YieldDistributor.sol #65
Conversation
| - Even better IMO would be a `mapping(address => uint256)` to track distributions per project. | ||
| That would allow a voting interface that passes votes by address rather than array index, which | ||
| is pretty hard to debug after the fact (e.g. through a chain explorer). The current setup is more | ||
| vulnerable to either of an evil frontend or MEV. Neither of those is a proximate issue, but still. |
There was a problem hiding this comment.
It occurs to me that more probable than an evil frontend is a bug in the real frontend that causes the displayed project order to mismatch with the contract project order. Protecting against that seems worthwhile.
| - This returns the voting power based on the last period, which is intended but may be | ||
| surprising. It would be good to comment this behavior. |
There was a problem hiding this comment.
Voting power at the time of distribution required us to iterate over all the voters, introducing an attack vector. This prompted us to record voting power at the time of voting and not distribution. This would, however, introduce an advantage to "late voters", so we opted to calculate it at a set delay.
There was a problem hiding this comment.
Makes sense, just think it deserves a comment.
| choice it should be explicitly called out, but I think it might make more sense to track the | ||
| last two distribution blocks, rather than just `lastClaimedBlockNumber` here. |
There was a problem hiding this comment.
I do not understand, the suggestion is to define an interval as a tuple of two blocks, rather than using lastClaimedBlockNumber? What are the benefits in this approach?
There was a problem hiding this comment.
The only benefit is that it will measure the power accumulated over the previous period between distributions, rather than the cycleLength before the previous distribution. I believe that in normal operation, these are expected to be very close. There is only a difference when distributions are not occurring on a cycleLength candence.
Consider the following scenario:
cycleLength == 100
Block 100: distribution
Block 200: no distribution (because the mechanism is broken or for some other reason)
Block 300: distribution
Block 400: distribution
IIUC, the distribution in block 400 will use the voting power accumulated between blocks 200 and 300. I'm proposing that it may be more fair to use the voting power accumulated between blocks 100 and 300 in this scenario.
There was a problem hiding this comment.
I agree with @ShaNagbaImuru on this. cycleLength would be more accurately considered the "minimum cycle length" before which a distribution can't happen. Assuming the suggested changes don't have any major tradeoffs, I think it makes for a better algorithm.
| @@ -236,6 +259,8 @@ contract YieldDistributor is OwnableUpgradeable { | |||
| /** | |||
| * @notice Internal function for casting votes for a specified user | |||
| * @param _account Address of user to cast votes for | |||
| * /* Unless I'm missing something, calling these basis points is pretty confusing, since that term | |||
| * usually refers to 1/100th of a percent, which these are not. | |||
There was a problem hiding this comment.
these points can be up to 10000, does that make sense?
There was a problem hiding this comment.
That's param-controlled and changeable, right? So they happen to be basis points currently but that is contingent. That does explain it though.
| /* It really feels like L170-190 could be unified into a single loop rather than a separate | ||
| step for calculating just the first chunk of voting power. | ||
| */ |
There was a problem hiding this comment.
Suggestion re: this
function getVotingPowerForPeriod(uint256 _start, uint256 _end, address _account) external view returns (uint256) {
if (_start >= _end) revert StartMustBeBeforeEnd();
if (_end > block.number) revert EndAfterCurrentBlock();
uint32 checkpointCount = BREAD.numCheckpoints(_account);
if (checkpointCount == 0) return 0;
Checkpoints.Checkpoint208 memory checkpoint = BREAD.checkpoints(_account, 0);
if (checkpoint._key > _end) return 0;
uint256 totalVotingPower = 0;
for (uint32 i = checkpointCount; i > 0;) {
checkpoint = BREAD.checkpoints(_account, --i);
if (checkpoint._key <= _end) {
uint48 effectiveStart = checkpoint._key < _start ? uint48(_start) : checkpoint._key;
totalVotingPower += checkpoint._value * (_end - effectiveStart);
if (checkpoint._key <= _start) {
totalVotingPower -= checkpoint._value * (_start - checkpoint._key);
break;
}
_end = checkpoint._key;
}
}
return totalVotingPower;
}
Thoughts @ShaNagbaImuru ?
There was a problem hiding this comment.
- It might be cleaner to use a
continuewhen thecheckpoint._key > end, to reduce the indent required. - I don't think I understand why you need both of calculating and using
effectiveStartand subtracting voting power whencheckpoint._key <= _start. That seems redundant unless I'm missing something. (If it is redundant, I prefer the first approach for clarity.) - I think there's probably already unit testing for this code, but if not there should be, to ensure that this change in logic doesn't change the results.
There was a problem hiding this comment.
Ok, opened #84 for this, will get back in touch regarding this
No description provided.