Service staking refactor #120
Conversation
kupermind
commented
Oct 3, 2023
kupermind
commented
- Service staking implementation accounting for latest suggestions;
- Adding more tests.
| // Optional service multisig threshold requirement | ||
| uint256 public immutable threshold; | ||
| // Optional service number of agent instances requirement | ||
| uint256 public immutable numAgentInstances; |
There was a problem hiding this comment.
According to the specifications, the agent instance is not an optional parameter.
| revert WrongServiceConfiguration(serviceId); | ||
| } | ||
| // Check the threshold, if applicable | ||
| if (threshold > 0 && threshold != minNumAgents) { |
There was a problem hiding this comment.
We might consider replacing minNumAgents with agentThreshold to improve the clarity of this check
| @@ -191,7 +281,6 @@ abstract contract ServiceStakingBase is IErrorsRegistries { | |||
| /// @param eligibleServiceIds Service Ids eligible for rewards. | |||
| /// @param eligibleServiceRewards Corresponding rewards for eligible service Ids. | |||
| /// @param serviceIds All the staking service Ids. | |||
| /// @param serviceNonces Current service nonces. | |||
There was a problem hiding this comment.
why this comment was removed?
There was a problem hiding this comment.
It was from the previous PR when this was considered obsolete, I've changed that now.
| uint256 serviceCheckpoint = tsCheckpointLast; | ||
| uint256 tsStart = curInfo.tsStart; | ||
| // Adjust the service checkpoint time if the service was staking less than the current staking period | ||
| if (tsStart > serviceCheckpoint) { |
There was a problem hiding this comment.
With this approach, we are not forcing a service to be staked for the entire liveness period. As we did previously, service multisigs that pass the liveness check will be considered for incentives, even if they have been staked for less than the liveness period. Is that correct?
There was a problem hiding this comment.
That is correct, as the checkpoint is a centralization point and everybody eligible must account for their rewards.
| } | ||
| // Calculate the liveness ratio in 1e18 value | ||
| uint256 ratio; | ||
| // If the checkpoint was called in the exactly same block, the ratio is zero |
There was a problem hiding this comment.
If the checkpoint was called in the exactly same block, we do not reach this part. No?
There was a problem hiding this comment.
I think it is possible if someone calls staking and checkpoint in the same block. There will be a test for it.
| // Assign agent Ids, if applicable | ||
| uint256 size = _stakingParams.agentIds.length; | ||
| uint256 agentId; | ||
| if (size > 0) { | ||
| for (uint256 i = 0; i < size; ++i) { | ||
| // Agent Ids must be unique and in ascending order | ||
| if (_stakingParams.agentIds[i] <= agentId) { | ||
| revert WrongAgentId(_stakingParams.agentIds[i]); | ||
| } | ||
| agentId = _stakingParams.agentIds[i]; | ||
| agentIds.push(agentId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this guaranteed by the registry already?
There was a problem hiding this comment.
Staking service agent Ids must match the staking contract configuration. We want to ensure at least the minimum order requirement.
There was a problem hiding this comment.
When staking, the service agents are extracted from the service and must match the provided agent Ids configuration.
Update Docs: Autonolas_tokenomics_audit
