refactor: evict inactive services in the checkpoint #141
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,10 +70,6 @@ error LowerThan(uint256 provided, uint256 expected); | |
| /// @param serviceId Service Id. | ||
| error WrongServiceConfiguration(uint256 serviceId); | ||
|
|
||
| /// @dev Service is not unstaked. | ||
| /// @param serviceId Service Id. | ||
| error ServiceNotUnstaked(uint256 serviceId); | ||
|
|
@@ -109,6 +105,12 @@ struct ServiceInfo { | |
| /// @author Andrey Lebedev - <[email protected]> | ||
| /// @author Mariapia Moscatiello - <[email protected]> | ||
| abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | ||
| enum ServiceStakingState { | ||
| Unstaked, | ||
| Staked, | ||
| Evicted | ||
| } | ||
|
|
||
| // Input staking parameters | ||
| struct StakingParams { | ||
| // Maximum number of staking services | ||
|
|
@@ -131,11 +133,13 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| bytes32 configHash; | ||
| } | ||
|
|
||
| event ServiceStaked(uint256 indexed epoch, uint256 indexed serviceId, address indexed multisig, address owner, | ||
|
There was a problem hiding this comment. why is owner not indexed? There was a problem hiding this comment. It's max of 3 indexed fields. I've changed that now, not indexing the epoch. |
||
| uint256[] nonces); | ||
| event Checkpoint(uint256 indexed epoch, uint256 availableRewards, uint256[] serviceIds, uint256[] rewards); | ||
| event ServiceUnstaked(uint256 indexed epoch, uint256 indexed serviceId, address indexed multisig, address owner, | ||
|
There was a problem hiding this comment. see above why tsStart? What's the point of emitting this here? |
||
| uint256[] nonces, uint256 reward, uint256 tsStart); | ||
| event ServicesEvicted(uint256 indexed epoch, uint256[] serviceIds, address[] owners, address[] multisigs, | ||
|
There was a problem hiding this comment. order of multisigs and owners need to be reverted to be symmetric with the other events |
||
| uint256[] serviceInactivity); | ||
| event Deposit(address indexed sender, uint256 amount, uint256 balance, uint256 availableRewards); | ||
| event Withdraw(address indexed to, uint256 amount); | ||
|
|
||
|
|
@@ -167,6 +171,8 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| // Max allowed inactivity | ||
| uint256 public immutable maxAllowedInactivity; | ||
|
|
||
| // Epoch counter | ||
| uint256 public epochCounter; | ||
| // Token / ETH balance | ||
| uint256 public balance; | ||
| // Token / ETH available rewards | ||
|
|
@@ -329,7 +335,7 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| // Transfer the service for staking | ||
| IService(serviceRegistry).safeTransferFrom(msg.sender, address(this), serviceId); | ||
|
|
||
| emit ServiceStaked(epochCounter, serviceId, service.multisig, msg.sender, nonces); | ||
| } | ||
|
|
||
| /// @dev Gets service multisig nonces. | ||
|
|
@@ -371,6 +377,7 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| /// @param eligibleServiceRewards Corresponding rewards for eligible service Ids. | ||
| /// @param serviceIds All the staking service Ids. | ||
| /// @param serviceNonces Current service nonces. | ||
| /// @param serviceInactivity Service inactivity records. | ||
| function _calculateStakingRewards() internal view returns ( | ||
| uint256 lastAvailableRewards, | ||
| uint256 numServices, | ||
|
|
@@ -435,20 +442,80 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| } | ||
| } | ||
|
|
||
| /// @dev Evicts services due to their extended inactivity. | ||
| /// @param evictServiceIds Service Ids to be evicted. | ||
| /// @param serviceInactivity Corresponding service inactivity records. | ||
| /// @param numEvictServices Number of services to evict. | ||
| function _evict( | ||
| uint256[] memory evictServiceIds, | ||
| uint256[] memory serviceInactivity, | ||
| uint256 numEvictServices | ||
| ) internal | ||
| { | ||
| // Get the total number of staked services | ||
| // All the passed arrays have the length of the number of staked services | ||
| uint256 totalNumServices = evictServiceIds.length; | ||
|
|
||
| // Get arrays of exact sizes | ||
| uint256[] memory serviceIds = new uint256[](numEvictServices); | ||
| address[] memory owners = new address[](numEvictServices); | ||
| address[] memory multisigs = new address[](numEvictServices); | ||
| uint256[] memory inactivity = new uint256[](numEvictServices); | ||
| uint256[] memory serviceIndexes = new uint256[](numEvictServices); | ||
|
|
||
| // Fill in arrays | ||
| uint256 sCounter; | ||
| uint256 serviceId; | ||
| for (uint256 i = 0; i < totalNumServices; ++i) { | ||
| if (evictServiceIds[i] > 0) { | ||
| serviceId = evictServiceIds[i]; | ||
| serviceIds[sCounter] = serviceId; | ||
|
|
||
| ServiceInfo storage sInfo = mapServiceInfo[serviceId]; | ||
| owners[sCounter] = sInfo.owner; | ||
| multisigs[sCounter] = sInfo.multisig; | ||
| inactivity[sCounter] = serviceInactivity[i]; | ||
| serviceIndexes[sCounter] = i; | ||
| sCounter++; | ||
| } | ||
| } | ||
|
|
||
| // Evict services from the global set of staked services | ||
| uint256 idx; | ||
| for (uint256 i = numEvictServices - 1; i > 0; --i) { | ||
| // Decrease the number of services | ||
| totalNumServices--; | ||
| // Get the evicted service index | ||
| idx = serviceIndexes[i]; | ||
| // Assign last service Id to the index that points to the evicted service Id | ||
| setServiceIds[idx] = setServiceIds[totalNumServices]; | ||
| // Pop the last element | ||
| setServiceIds.pop(); | ||
| } | ||
|
|
||
| // Deal with the very first element | ||
| // Get the evicted service index | ||
| idx = serviceIndexes[0]; | ||
| // Assign last service Id to the index that points to the evicted service Id | ||
| setServiceIds[idx] = setServiceIds[totalNumServices - 1]; | ||
| // Pop the last element | ||
| setServiceIds.pop(); | ||
|
|
||
| emit ServicesEvicted(epochCounter, serviceIds, owners, multisigs, inactivity); | ||
| } | ||
|
|
||
| /// @dev Checkpoint to allocate rewards up until a current time. | ||
| /// @return All staking service Ids (including evicted ones during within a current epoch). | ||
| /// @return All staking updated nonces (including evicted ones during within a current epoch). | ||
|
|
||
| /// @return Set of eligible service Ids. | ||
|
There was a problem hiding this comment. why do we no longer return the nonces? There was a problem hiding this comment. We return nonces, we don't return the success flag. No need to return that, just can check for the |
||
| /// @return Corresponding set of eligible service rewards. | ||
| /// @return evictServiceIds Evicted service Ids. | ||
| function checkpoint() public returns ( | ||
| uint256[] memory, | ||
| uint256[][] memory, | ||
| uint256[] memory, | ||
| uint256[] memory, | ||
| uint256[] memory evictServiceIds | ||
| ) | ||
| { | ||
| // Calculate staking rewards | ||
|
|
@@ -457,9 +524,16 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| uint256[] memory serviceIds, uint256[][] memory serviceNonces, | ||
| uint256[] memory serviceInactivity) = _calculateStakingRewards(); | ||
|
|
||
| // Get arrays for eligible service Ids and rewards of exact size | ||
| uint256[] memory finalEligibleServiceIds; | ||
| uint256[] memory finalEligibleServiceRewards; | ||
| evictServiceIds = new uint256[](serviceIds.length); | ||
| uint256 curServiceId; | ||
|
Comment on lines
+534
to
+537
There was a problem hiding this comment. For the event, we need a precise number of eligible services. Also, evict services are allocated to the size of the all services in the set of staked services. |
||
|
|
||
| // If there are eligible services, proceed with staking calculation and update rewards | ||
| if (numServices > 0) { | ||
| finalEligibleServiceIds = new uint256[](numServices); | ||
| finalEligibleServiceRewards = new uint256[](numServices); | ||
| // If total allocated rewards are not enough, adjust the reward value | ||
| if (totalRewards > lastAvailableRewards) { | ||
| // Traverse all the eligible services and adjust their rewards proportional to leftovers | ||
|
|
@@ -472,17 +546,21 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| updatedTotalRewards += updatedReward; | ||
| // Add reward to the overall service reward | ||
| curServiceId = eligibleServiceIds[i]; | ||
| finalEligibleServiceIds[i] = eligibleServiceIds[i]; | ||
| finalEligibleServiceRewards[i] = updatedReward; | ||
| mapServiceInfo[curServiceId].reward += updatedReward; | ||
| } | ||
|
|
||
| // Process the first service in the set | ||
| updatedReward = (eligibleServiceRewards[0] * lastAvailableRewards) / totalRewards; | ||
| updatedTotalRewards += updatedReward; | ||
| curServiceId = eligibleServiceIds[0]; | ||
| finalEligibleServiceIds[0] = eligibleServiceIds[0]; | ||
| // If the reward adjustment happened to have small leftovers, add it to the first service | ||
| if (lastAvailableRewards > updatedTotalRewards) { | ||
| updatedReward += lastAvailableRewards - updatedTotalRewards; | ||
| } | ||
| finalEligibleServiceRewards[0] = updatedReward; | ||
| // Add reward to the overall service reward | ||
| mapServiceInfo[curServiceId].reward += updatedReward; | ||
| // Set available rewards to zero | ||
|
|
@@ -492,6 +570,8 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| for (uint256 i = 0; i < numServices; ++i) { | ||
| // Add reward to the service overall reward | ||
| curServiceId = eligibleServiceIds[i]; | ||
| finalEligibleServiceIds[i] = eligibleServiceIds[i]; | ||
| finalEligibleServiceRewards[i] = eligibleServiceRewards[i]; | ||
| mapServiceInfo[curServiceId].reward += eligibleServiceRewards[i]; | ||
| } | ||
|
|
||
|
|
@@ -505,30 +585,48 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
|
|
||
| // If service Ids are returned, then the checkpoint takes place | ||
| if (serviceIds.length > 0) { | ||
| numServices = 0; | ||
| // Record service inactivities and updated current service nonces | ||
| for (uint256 i = 0; i < serviceIds.length; ++i) { | ||
| // Get the current service Id | ||
| curServiceId = serviceIds[i]; | ||
| // Record service nonces | ||
| mapServiceInfo[curServiceId].nonces = serviceNonces[i]; | ||
|
|
||
| // Increase service inactivity if it is greater than zero | ||
| if (serviceInactivity[i] > 0) { | ||
| // Get the overall continuous service inactivity | ||
| serviceInactivity[i] = mapServiceInfo[curServiceId].inactivity + serviceInactivity[i]; | ||
| mapServiceInfo[curServiceId].inactivity = serviceInactivity[i]; | ||
| // Check for the maximum allowed inactivity time | ||
| if (serviceInactivity[i] > maxAllowedInactivity) { | ||
| // Evict a service if it has been inactive for more than a maximum allowed inactivity time | ||
| evictServiceIds[i] = curServiceId; | ||
| // Increase number of evicted services | ||
| numServices++; | ||
| } | ||
| } else { | ||
| // Otherwise, set it back to zero | ||
| mapServiceInfo[curServiceId].inactivity = 0; | ||
| } | ||
| } | ||
|
|
||
| // Evict inactive services | ||
| if (numServices > 0) { | ||
| _evict(evictServiceIds, serviceInactivity, numServices); | ||
| } | ||
|
|
||
| // Record the current timestamp such that next calculations start from this point of time | ||
| tsCheckpoint = block.timestamp; | ||
|
|
||
| // Increase the epoch counter | ||
| uint256 eCounter = epochCounter; | ||
| epochCounter = eCounter + 1; | ||
|
|
||
| emit Checkpoint(eCounter, lastAvailableRewards, finalEligibleServiceIds, finalEligibleServiceRewards); | ||
| } | ||
|
|
||
| return (serviceIds, serviceNonces, finalEligibleServiceIds, finalEligibleServiceRewards, evictServiceIds); | ||
| } | ||
|
|
||
| /// @dev Unstakes the service. | ||
|
|
@@ -540,26 +638,36 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| revert OwnerOnly(msg.sender, sInfo.owner); | ||
| } | ||
|
|
||
| // Get the staking start time | ||
| // Note that if the service info exists, the service is staked or evicted, and thus start time is always valid | ||
| uint256 tsStart = sInfo.tsStart; | ||
|
|
||
| // Check that the service has staked long enough, or if there are no rewards left | ||
mariapiamo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| uint256 ts = block.timestamp - tsStart; | ||
| if (ts <= maxAllowedInactivity && availableRewards > 0) { | ||
| revert NotEnoughTimeStaked(serviceId, ts, maxAllowedInactivity); | ||
| } | ||
|
|
||
| // Call the checkpoint | ||
| (uint256[] memory serviceIds, , , , uint256[] memory evictServiceIds) = checkpoint(); | ||
|
|
||
| // If the checkpoint was not successful, the serviceIds set is not returned and needs to be allocated | ||
| if (serviceIds.length == 0) { | ||
| serviceIds = getServiceIds(); | ||
| evictServiceIds = new uint256[](serviceIds.length); | ||
| } | ||
|
|
||
| // Get the service index in the set of services | ||
| // The index must always exist as the service is currently staked, otherwise it has no record in the map | ||
| uint256 idx; | ||
| bool inSet; | ||
| for (; idx < serviceIds.length; ++idx) { | ||
| // Service is still in a global staking set if it is found in the services set, | ||
| // and is not present in the evicted set | ||
| if (evictServiceIds[idx] == serviceId) { | ||
| break; | ||
| } else if (serviceIds[idx] == serviceId) { | ||
| inSet = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
@@ -575,7 +683,7 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
|
|
||
| // Update the set of staked service Ids | ||
| // If the index was not found, the service was evicted and is not part of staked services set | ||
| if (inSet) { | ||
| setServiceIds[idx] = setServiceIds[setServiceIds.length - 1]; | ||
| setServiceIds.pop(); | ||
| } | ||
|
|
@@ -588,40 +696,7 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| _withdraw(multisig, reward); | ||
| } | ||
|
|
||
| emit ServiceUnstaked(epochCounter, serviceId, multisig, msg.sender, nonces, reward, tsStart); | ||
| } | ||
|
|
||
| /// @dev Calculates service staking reward during the last checkpoint period. | ||
|
|
@@ -655,19 +730,20 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries { | |
| ServiceInfo memory sInfo = mapServiceInfo[serviceId]; | ||
| reward = sInfo.reward; | ||
|
|
||
| // Add pending reward | ||
| reward += calculateServiceStakingLastReward(serviceId); | ||
| } | ||
|
|
||
| /// @dev Gets the service staking state. | ||
| /// @param serviceId. | ||
| /// @return stakingState Staking state of the service. | ||
| function getServiceStakingState(uint256 serviceId) external view returns (ServiceStakingState stakingState) { | ||
| ServiceInfo memory sInfo = mapServiceInfo[serviceId]; | ||
| if (sInfo.inactivity > maxAllowedInactivity) { | ||
| stakingState = ServiceStakingState.Evicted; | ||
| } else if (sInfo.tsStart > 0) { | ||
| stakingState = ServiceStakingState.Staked; | ||
| } | ||
| } | ||
|
|
||
| /// @dev Gets the next reward checkpoint timestamp. | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving a bit more information about service staking statuses