feat: first implementation of the service staking #116

kupermind · 2023-09-21T18:19:36Z

First implementation of the service staking.

contracts/staking/ServiceStakingToken.sol

contracts/staking/ServiceStakingBase.sol

DavidMinarsch · 2023-09-24T21:52:45Z

contracts/staking/ServiceStakingBase.sol

+
+        // Transfer accumulated rewards to the service owner
+        if (amount > 0) {
+            _withdraw(msg.sender, amount);


Needs to be sent to multisig, not service owner!

Just to make sure: the agents of the multisig then have to transfer rewards somewhere later or utilize rewards for the service's needs? Also it means that the service owner is not getting anything directly, although they were staking the service.

I understand correctly that receiving a reward is not a mirror (asymmetrical) operation to "staking"

Yes the service logic is responsible for deciding where rewards go.

… times of staking

DavidMinarsch · 2023-09-25T22:09:05Z

contracts/staking/ServiceStakingBase.sol

+    uint256[] public setServiceIds;
+
+    /// @dev ServiceStakingBase constructor.
+    /// @param _apy Staking APY (in single digits).


apy - not sure that's appropriate terminology

DavidMinarsch · 2023-09-25T22:09:56Z

contracts/staking/ServiceStakingBase.sol

+    /// @dev ServiceStakingBase constructor.
+    /// @param _apy Staking APY (in single digits).
+    /// @param _minStakingDeposit Minimum security deposit for a service to be eligible to stake.
+    /// @param _stakingRatio Staking ratio: number of seconds per nonce (in 18 digits).


Suggested change

/// @param _stakingRatio Staking ratio: number of seconds per nonce (in 18 digits).

/// @param _livenessRatio Liveness ratio: number of seconds per nonce (in 18 digits).

seems more appropriate

DavidMinarsch · 2023-09-25T22:15:31Z

contracts/staking/ServiceStakingBase.sol

+                    serviceCheckpoint = curInfo.tsStart;
+                }
+                // Calculate the nonce ratio in 1e18 value
+                uint256 ratio = ((block.timestamp - serviceCheckpoint) * 1e18) / (curNonce - curInfo.nonce);


this is the wrong way round. More tx is more liveness!

Also prevent division by zero

I can see an issue here with closeness of checkpoints leading to rounding issues

This can be defined in either direction. If we believe there will be more tx-s than seconds, then this must be reversed. We are still checking on a specific period of time (from last checkpoint till now) and how many tx-s were performed by every staking service within that period.

Agree, incorrect.
It should be tps with dimension: nonce/s (tx/s). In the code we got the inverse value.

yeah, tps is the inverse. Plus denominator cannot be zero

DavidMinarsch · 2023-09-25T22:18:53Z

contracts/staking/ServiceStakingToken.sol

+        uint256 newAvailableRewards = availableRewards + amount;
+
+        // Update rewards per second
+        uint256 newRewardsPerSecond = (newAvailableRewards * apy) / (100 * 365 days);


I'd prefer if rewards per second was an arg rather than apy

So as I understand what we want:
A given constant RewardsPerSecond (RPS) with dimension [T/s] T - token (including ETH)
RewardsPerSecond : immutable, and accordingly does not depend on the current AvailableRewards

Formula reward = RPS * (t2 - t1) (1*) A simple example of how this works without boundary conditions ( AvailableRewards >> sum(rewards)): 0. Start staking with AvailableRewards = 1M Tokens, no staking services, RPS = 1T/s 1. Staking 3 services: s1, s2, s3 2. +24h. Unstaking 3 services s1 = pass tps_ratio => reward1 (yes) s2 = pass tps_ratio => reward2 (yes) s3 = fail tps_ratio => 0 (no) reward1 = RPS * 24*60*60s = 1T/s * 86400s = 86400 T reward2 = RPS * 24*60*60s = 1T/s * 86400s = 86400 T AvailableRewards = AvailableRewards - sum(rewards) = 1M - 2*86400 = 827,200 Let's assume we still have AvailableRewards = 100k 0. No staking atm 1. Staking 3 services: s1, s2, s3 2. +24h. Unstaking 3 services s1 = pass tps_ratio => reward1 (yes) s2 = pass tps_ratio => reward2 (yes) s3 = fail tps_ratio => 0 (no) reward1 = RPS * 24*60*60s = 1T/s * 86400s = 86400 T (incorrect) reward2 = RPS * 24*60*60s = 1T/s * 86400s = 86400 T (incorrect) AvailableRewards = AvailableRewards - sum(rewards) = 100k - 2*86400 < 0 If AvailableRewards < sum(rewards) by (1*): We need to distribute rewards based on the current limit in proportion to the "ideal" reward (1*) reward1fix = RPS * 24*60*60s /sum(rewards) * AvailableRewards = 1/2 * 100k = 50k T reward2fix = RPS * 24*60*60s /sum(rewards) * AvailableRewards = 1/2 * 100k = 50k T AvailableRewards = AvailableRewards - 100k = 0

With your suggestion @DavidMinarsch and the formula of @77ph the contract becomes significantly simpler. However, I have concerns because staking rewards are entirely unrelated to the service security deposit amount. This could potentially lead to a situation where staking rewards greatly exceed the service security deposits.

"I have concerns because staking rewards are entirely unrelated to the service security deposit amount." -> that's up to the contract deployer. They set both after all.

DavidMinarsch · 2023-09-25T22:21:11Z

contracts/staking/ServiceStakingBase.sol

+
+                // Adjust the available rewards value
+                if (lastAvailableRewards >= reward) {
+                    lastAvailableRewards -= reward;


use memory var in loop when optimising later

contracts/staking/ServiceStakingBase.sol

mariapiamo · 2023-09-26T08:23:02Z

contracts/staking/ServiceStakingBase.sol

+                    serviceCheckpoint = curInfo.tsStart;
+                }
+                // Calculate the nonce ratio in 1e18 value
+                uint256 ratio = ((block.timestamp - serviceCheckpoint) * 1e18) / (curNonce - curInfo.nonce);


yeah, tps is the inverse. Plus denominator cannot be zero

mariapiamo · 2023-09-26T08:28:57Z

contracts/staking/ServiceStakingBase.sol

+
+            // Process each eligible service Id reward
+            // Calculate the maximum possible reward per service during the last deposit period
+            uint256 maxRewardsPerService = (rewardsPerSecond * (block.timestamp - tsCheckpointLast)) / numServices;


How large can be rewardsPerSecond? Worried when the numSevices is small

This is not applicable anymore

mariapiamo · 2023-09-26T09:14:43Z

contracts/staking/ServiceStakingBase.sol

+                    serviceCheckpoint = curInfo.tsStart;
+                }
+
+                // If the staking was longer than the deposited period, the service's timestamp is adjusted such that


not sure about the following

mariapiamo · 2023-09-26T10:24:16Z

contracts/staking/ServiceStakingToken.sol

+        uint256 newAvailableRewards = availableRewards + amount;
+
+        // Update rewards per second
+        uint256 newRewardsPerSecond = (newAvailableRewards * apy) / (100 * 365 days);


With your suggestion @DavidMinarsch and the formula of @77ph the contract becomes significantly simpler. However, I have concerns because staking rewards are entirely unrelated to the service security deposit amount. This could potentially lead to a situation where staking rewards greatly exceed the service security deposits.

contracts/staking/ServiceStakingBase.sol

mariapiamo · 2023-09-27T08:56:34Z

contracts/staking/ServiceStakingBase.sol

+                }
+                // Calculate the liveness ratio in 1e18 value
+                uint256 ratio;
+                // If the checkpoint was called in the exactly same block, the ratio is zero


what if there is a checkpoint at the beginning of the block, one at the end, and in the middle one service multisig has done some transactions? We just update the nonce info at the end on this checkpoint, but all the transaction in between are not accounted for incentives

I don't see the issue here, as the last nonce will be registered at the moment of the checkpoint, so if more nonces are going to be in that block, they will be picked up the next time.

I guess @mariapiamo's point requires >= rather than >?

mariapiamo · 2023-09-27T09:18:45Z

contracts/staking/ServiceStakingBase.sol

+                    mapServiceInfo[curServiceId].reward += updatedReward;
+                }
+
+                // If the reward adjustment happened to have small leftovers, add it to the last traversed service


So last service in the list can receive a bit more?

Is reverse loop more expensive? Makes sense to have the first staked service have this privilige.

contracts/staking/ServiceStakingBase.sol

mariapiamo · 2023-09-27T09:31:59Z

contracts/staking/ServiceStakingBase.sol

+        setServiceIds[idx] = setServiceIds[setServiceIds.length - 1];
+        setServiceIds.pop();
+
+        emit ServiceUnstaked(serviceId, msg.sender, amount, tsStart);


can we emit also service multisig address and nonce?

contracts/staking/ServiceStakingToken.sol

contracts/staking/ServiceStakingBase.sol

contracts/staking/ServiceStaking.sol

contracts/staking/ServiceStakingBase.sol

DavidMinarsch

Nice progress!

My biggest concern is now what I mentioned this morning: with this implementation it can happen that frequent staking/unstaking/checkpoint calls leads to "unjust" reward allocations simply because the liveness check is missed due to non-equally spaced transactions.

There's different ways to solve this:

we can say checkpoints can only happen every x time period. If people unstake before they get by default nothing for the rolling period.
we can somehow smoothen the liveness calculation across checkpoints. @mariapiamo might have ideas there?
other ideas...

DavidMinarsch · 2023-09-29T04:05:16Z

contracts/staking/ServiceStakingBase.sol

+            revert MaxNumServicesReached(maxNumServices);
+        }
+
+        // Check the service conditions for staking


I notice a nice optional feature which I'd like to support from the beginning: a staking contract can optionally specify an exact agentID and hash for the service that it requires to be staked. Then here, we check the agentId and service hash as well if and only if the hash is provided in the staking contract.

This would allow staking contracts to express exactly what service they want at which version.

This also raises the question if we should allow the staking contract to specify the number of agent instances and threshold.

These four things together tie down the exact configuration.

ok that's nice

DavidMinarsch · 2023-09-29T04:07:54Z

contracts/staking/ServiceStakingBase.sol

+                }
+                // Calculate the liveness ratio in 1e18 value
+                uint256 ratio;
+                // If the checkpoint was called in the exactly same block, the ratio is zero


I guess @mariapiamo's point requires >= rather than >?

DavidMinarsch · 2023-09-29T04:09:13Z

contracts/staking/ServiceStakingBase.sol

+                    mapServiceInfo[curServiceId].reward += updatedReward;
+                }
+
+                // If the reward adjustment happened to have small leftovers, add it to the last traversed service


Is reverse loop more expensive? Makes sense to have the first staked service have this privilige.

DavidMinarsch · 2023-09-29T04:10:50Z

contracts/staking/ServiceStaking.sol

+/// @author Aleksandr Kuperman - <[email protected]>
+/// @author Andrey Lebedev - <[email protected]>
+/// @author Mariapia Moscatiello - <[email protected]>
+contract ServiceStaking is ServiceStakingBase {


Maybe call it ServiceStakingNativeToken?

DavidMinarsch · 2023-09-29T04:13:35Z

contracts/staking/ServiceStakingToken.sol

+        if (_stakingToken == address(0) || _serviceRegistryTokenUtility == address(0)) {
+            revert ZeroAddress();
+        }
+
+        stakingToken = _stakingToken;
+        serviceRegistryTokenUtility = _serviceRegistryTokenUtility;


Should also check that the staking token matches the one used for security deposit? I mean in principle one could break this symmetry. But I think we should then have a different contract. Symmetry is more "expected"

We check that for each service itself. ServiceRegistryTokenUtility is not bound to a specific custom token.

DavidMinarsch · 2023-09-29T04:14:05Z

contracts/staking/ServiceStakingToken.sol

+        // The staking token must match the contract token
+        if (stakingToken != token) {
+            revert WrongStakingToken(stakingToken, token);
+        }


Also in constructor

kupermind · 2023-09-29T12:46:14Z

TPS average = (block.timestamp - time.start) / (nonce.current - nonce.start)

chore: adding contracts audit scripts

kupermind requested review from 77ph and mariapiamo September 21, 2023 18:19

kupermind added 5 commits September 21, 2023 19:21

feat: first implementation of the service staking

2487fb6

refactor: continue working on the service staking implementation

54abdab

Merge branch 'main' into service_staking

5dd6c20

refactor: advancing service staking contracts

d863092

refactor: advancing service staking contracts

8cc3855

DavidMinarsch reviewed Sep 24, 2023

View reviewed changes

kupermind added 2 commits September 25, 2023 14:29

refactor: separate interfaces, separate balance accounting and record…

4d2112c

… times of staking

chore: change variable names

5f44079

DavidMinarsch reviewed Sep 25, 2023

View reviewed changes

refactor: more service stakng optimizations

d7801c9

mariapiamo reviewed Sep 26, 2023

View reviewed changes

kupermind added 2 commits September 26, 2023 17:14

refactor: more code optimization for service staking

9154cb9

refactor: more code optimization for service staking

a013cc5

DavidMinarsch reviewed Sep 26, 2023

View reviewed changes

contracts/staking/ServiceStakingBase.sol Outdated Show resolved Hide resolved

DavidMinarsch reviewed Sep 26, 2023

View reviewed changes

contracts/staking/ServiceStakingBase.sol Outdated Show resolved Hide resolved

contracts/staking/ServiceStakingBase.sol Outdated Show resolved Hide resolved

contracts/staking/ServiceStakingBase.sol Outdated Show resolved Hide resolved

kupermind added 6 commits September 26, 2023 22:54

refactor: more code optimization for service staking

c61db33

refactor: more code optimization for service staking

1d13365

refactor: more code optimization for service staking

e409d0c

refactor: more code optimization for service staking

1ca785c

refactor: more code optimization for service staking

9d6e990

refactor: more code optimization for service staking

78da510

mariapiamo reviewed Sep 27, 2023

View reviewed changes

77ph reviewed Sep 27, 2023

View reviewed changes

contracts/staking/ServiceStakingBase.sol Show resolved Hide resolved

refactor: adding max number of staking services

68af804

DavidMinarsch reviewed Sep 29, 2023

View reviewed changes

77ph approved these changes Oct 6, 2023

View reviewed changes

mariapiamo approved these changes Oct 6, 2023

View reviewed changes

kupermind merged commit db5b1fc into main Oct 6, 2023
2 checks passed

kupermind deleted the service_staking branch October 6, 2023 13:31

77ph pushed a commit that referenced this pull request Oct 12, 2023

Merge pull request #116 from valory-xyz/audit_contracts

7216fd3

chore: adding contracts audit scripts

	/// @param _stakingRatio Staking ratio: number of seconds per nonce (in 18 digits).
	/// @param _livenessRatio Liveness ratio: number of seconds per nonce (in 18 digits).

feat: first implementation of the service staking #116

feat: first implementation of the service staking #116

Conversation

kupermind commented Sep 21, 2023

Choose a reason for hiding this comment

kupermind Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

77ph Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidMinarsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kupermind commented Sep 29, 2023 • edited Loading

kupermind Sep 25, 2023 •

edited

Loading

77ph Sep 26, 2023 •

edited

Loading

kupermind commented Sep 29, 2023 •

edited

Loading