Add batch delegatee score update functionality to BinaryEligibilityOracleEarningPowerCalculator #153
Conversation
funkyenough
changed the title
There was a problem hiding this comment.
Thanks @funkyenough. Overall this is looking great. I did request some changes to avoid duplication and to clarify one of the requirements.
src/calculators/BinaryEligibilityOracleEarningPowerCalculator.sol
Outdated
Show resolved
Hide resolved
src/calculators/BinaryEligibilityOracleEarningPowerCalculator.sol
Outdated
Show resolved
Hide resolved
src/calculators/BinaryEligibilityOracleEarningPowerCalculator.sol
Outdated
Show resolved
Hide resolved
| } | ||
| _updateDelegateeScore(_delegatee, _newScore); | ||
| } | ||
| if (_delegateeScoreUpdates.length > 0) lastOracleUpdateTime = block.timestamp; |
There was a problem hiding this comment.
Hmm how to handle this is actually an interesting question. It might actually be worthwhile to allow the oracle to call this method with 0 new scores yet have that actually update the timestamp. It could be useful as a way to "touch" the oracle and keep it fresh even if no scores need to change.
Let's switch to that: calling this method with a zero length array should update the lastOracleUpdateTime
There was a problem hiding this comment.
Makes sense!
I was initially cautious about the security implications of allowing empty arrays to update the timestamp. I took some time to think it through and realized that for any meaningful attack to succeed here, the oracle itself would need to be compromised, and at that point the entire system's security model is already broken anyways.
Change implemented in 47020f0
src/calculators/BinaryEligibilityOracleEarningPowerCalculator.sol
Outdated
Show resolved
Hide resolved
| } | ||
| for (uint256 _i = 0; _i < _delegateeScoreUpdates.length; _i++) { | ||
| address _delegatee = _delegateeScoreUpdates[_i].delegatee; | ||
| uint256 _newScore = _delegateeScoreUpdates[_i].newScore; |
There was a problem hiding this comment.
Might be worth using memory reference for calldata array instead of accesing the array twice
DelegateeScoreUpdate calldata update = _delegateeScoreUpdates[_i];
address _delegatee = update.delegatee;
uint256 _newScore = update.newScore;There was a problem hiding this comment.
Good call! Fixed in d318e9b
src/calculators/BinaryEligibilityOracleEarningPowerCalculator.sol
Outdated
Show resolved
Hide resolved
| } | ||
| _revertIfNotScoreOracle(); | ||
| _revertIfPaused(); | ||
| _revertIfDelegateeScoreLocked(_delegatee); |
There was a problem hiding this comment.
This is soo much cleaner. ❤️ nice job
src/calculators/BinaryEligibilityOracleEarningPowerCalculator.sol
Outdated
Show resolved
Hide resolved
| _revertIfPaused(); | ||
| uint256 _delegateesLength = _delegateeScoreUpdates.length; | ||
| for (uint256 _i = 0; _i < _delegateesLength; _i++) { | ||
| DelegateeScoreUpdate calldata update = _delegateeScoreUpdates[_i]; |
There was a problem hiding this comment.
nit: should this be renamed to _update / _delagateScoreUpdate to be consistent (cause convention is we add _ to local vars )
There was a problem hiding this comment.
Indeed. Fixed in 9b13507
| ); | ||
| } | ||
|
|
||
| function _boundToRealisticDelegateeScoreUpdateLength(uint256 _length) |
There was a problem hiding this comment.
Ah i like this naming. Will borrow this onto the project I'm working on
|
LGTM |
There was a problem hiding this comment.
Left one small nit on test naming @funkyenough, if you don't mind fixing that up. Otherwise this is good to go, from my perspective, but let's avoid merging it for now, because I want to share the PR with the auditors who will be doing a security review as-is.
|
Coverage after merging feature/beoepc-batch-update-delegatee-score into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
4 participants
Implementation
updateDelegateeScoresaccepts an array ofDelegateeScoreUpdatestructs for efficient batch processingupdateDelegateeScoretest patterns to cover the batch methoda. Empty array handling
b. Single delegatee, single update
c. Single delegatee, multiple updates
d. Multiple delegatees, single update each
e. Multiple delegatees, multiple updates each
Design Decisions