refactor and test: service staking optimization #135
Conversation
kupermind
commented
Oct 24, 2023
kupermind
commented
- Optimizing service staking contract logic;
- Adding tests.
| // Get the service Ids set length | ||
| uint256 size = setServiceIds.length; | ||
| serviceIds = new uint256[](size); | ||
|
|
||
| // Record service Ids | ||
| for (uint256 i = 0; i < size; ++i) { | ||
| // Get current service Id | ||
| serviceIds[i] = setServiceIds[i]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't allocate serviceIds if the rewards calculation is not going to be successful.
| lastAvailableRewards = availableRewards; | ||
| if (block.timestamp - tsCheckpointLast >= livenessPeriod && lastAvailableRewards > 0) { |
There was a problem hiding this comment.
Merged that check in one: for the liveness period and available rewards.
| (uint256[] memory serviceIds, , , , , ) = checkpoint(); | ||
| (uint256[] memory serviceIds, , , , , bool success) = checkpoint(); | ||
|
|
||
| // If the checkpoint was not successful, the serviceIds set is not returned and needs to be allocated | ||
| if (!success) { | ||
| serviceIds = getServiceIds(); | ||
| } |
There was a problem hiding this comment.
Checkpoint now might not return serviceIds in memory if it was not successful. So in that case it must be obtained separately.
| if (numServices > 0) { | ||
| for (uint256 i = 0; i < numServices; ++i) { |
There was a problem hiding this comment.
No need to check for the numServices, just loop with them. Also, the eligibleServiceIds.length is not correct as the length is equal to the full length of serviceIds, and it must have the maximum of numServices elements.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
=======================================
Coverage 99.73% 99.73%
=======================================
Files 22 22
Lines 1530 1536 +6
Branches 287 286 -1
=======================================
+ Hits 1526 1532 +6
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
| } | ||
|
|
||
| /// @dev Gets staked service Ids. | ||
| /// @return serviceIds Staked service Ids. | ||
| function getServiceIds() public view returns (uint256[] memory serviceIds) { |
There was a problem hiding this comment.
Adding the function that returns current staking serviceIds. The default ABI getter would not be enough as it does not return all of the Ids and the length, but just a single element with the provided index.
| /// @dev Gets the next reward checkpoint timestamp. | ||
| /// @return tsNext Next reward checkpoint timestamp. | ||
| function getNextRewardCheckpointTimestamp() external view returns (uint256 tsNext) { | ||
| // Last checkpoint timestamp plus the liveness period | ||
| tsNext = tsCheckpoint + livenessPeriod; |
There was a problem hiding this comment.
Getter of the next reward checkpoint timestamp - the agent can always compare with the block.timestamp and decide whether to call a checkpoint() or skip for now.
