feat: Rewards v2 #837

0xrajath · 2024-10-16T23:16:00Z

Motivation

As part of the Rewards v2 release, we need to support the following features:

Performance-based Rewards submission by AVSs.
Batching claiming by users to be more gas efficient.
Variable Operator commission per AVS (Settable by the operator with a 7-day delay. Default of 10%)

Modifications

IRewardsCoordinator :
- PerformanceRewardsSubmission struct added.
- OperatorReward struct added.
- AVSPerformanceRewardsSubmissionCreated event added.
- createAVSPerformanceRewardsSubmission external function added.
- processClaims external function added.
- OperatorAVSCommission struct added.
- OperatorAVSCommissionBipsSet event added.
- setOperatorAVSCommission external function added.
RewardsCoordinator :
- PAUSED_AVS_PERFORMANCE_REWARDS_SUBMISSION constant added,
- createAVSPerformanceRewardsSubmission external function added.
- _validatePerformanceRewardsSubmission internal function added.
- processClaims external function added.
- _processClaim internal function added.
- processClaim external function was refactored to use _processClaim
- setOperatorAVSCommission external function added.
RewardsCoordinatorStorage :
- isAVSPerformanceRewardsSubmissionHash nested mapping added.
- operatorAVSCommissionBips nested mapping added.
- Updated storage gap from 40 to 38 slots to accomodate isAVSPerformanceRewardsSubmissionHash and operatorAVSCommissionBips

TODO

Test Cases.
Update operatorCommissionBips view function to have logic for operatorAVSCommission too.
7 day delay in setOperatorAVSCommission.
Add a check to ensure that the operator is registered to the AVS in setOperatorAVSCommission.
Update createAVSPerformanceRewardsSubmission and related stuff to be delegation-aware and future-proofed by taking avs as a param.

Open Questions

Do we need delegation-aware interfaces for these new setters in this release?
Do we need variable Operator commission for all Programmatic Incentives (Settable by the operator with a 7-day delay. Default of 10%)?
Do we need to support Operator-set commissions in this release or be forward compatible for it (What does that entail for the setters and getters. Eg: Operator set ID, etc.)?
Do we need Operator key rotation in this release (To give time for Operators to rotate before slashing)?

Result

Rewards v2 release.

0xrajath · 2024-10-16T23:44:21Z

lib/forge-std

Will fix this. Something happened while rebasing.

0xrajath · 2024-10-16T23:46:07Z

script/deploy/holesky/upgrade_preprod_rewardsCoordinator.s.sol

This file was touched since dev wasn't compiling and I manually made it _verifyContractsInitialized(false);. But now I need to figure out what's up with my linting. Either these existing files haven't been linted using prettier or we're currently not using forge fmt (My IDE is auto linting them on save).

0xrajath · 2024-10-16T23:46:29Z

src/contracts/core/RewardsCoordinator.sol

+    function createAVSRewardsSubmission(
+        RewardsSubmission[] calldata rewardsSubmissions
+    ) external onlyWhenNotPaused(PAUSED_AVS_REWARDS_SUBMISSION) nonReentrant {


This was changed due to auto linting.

Multiple other auto-linting instances in this PR.

0xrajath · 2024-10-16T23:49:16Z

src/contracts/core/RewardsCoordinator.sol

+     * @param performanceRewardsSubmission PerformanceRewardsSubmission to validate.
+     * @return total amount to be transferred from the avs to the contract.
+     */
+    function _validatePerformanceRewardsSubmission(


Not checking for MAX_FUTURE_LENGTH (Since Performance based reward submissions are retroactive) or MAX_REWARDS_AMOUNT (Since we no longer have the 1e38 - 1 limitation in the offchain calculation.

could u add this in code comments?

Done in 179aa1f as a @dev natspec.

0xrajath · 2024-10-16T23:49:58Z

src/contracts/core/RewardsCoordinatorStorage.sol

+    /// @notice Mapping: avs => avsPerformanceRewardsSubmissionHash => bool to check if performance rewards submission hash has been submitted
+    mapping(address => mapping(bytes32 => bool)) public isAVSPerformanceRewardsSubmissionHash;


Was there a reason we needed separate mappings for each reward type? I just copied the current setup but questioning the need for it here.

let's combine into one, agreed

I'm assuming this is used by the rewards pipeline/sidecar in some form. I don't want to break backwards compatibility if that's the case.

@ypatil12 @8sunyuan may have opinions

This should be fine since we include the nonce in the rewards submission hash. cc @8sunyuan to work

Internally we're not using this mapping offchain

Oh if it's not being used offchain, what was the purpose of this mapping? I don't see it being used anywhere else besides just setting it during reward submission.

Is it meant to be used as a view function by some other contract in the future?

@8sunyuan do you remember? I can't recall off the top of my head

It appears it may actually have no use... potentially we wanted it for some sort of future pipeline integration

0xrajath · 2024-10-16T23:50:41Z

src/contracts/core/RewardsCoordinatorStorage.sol

@@ -120,5 +123,5 @@ abstract contract RewardsCoordinatorStorage is IRewardsCoordinator {
     * variables without shifting down storage in the inheritance chain.
     * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
     */
-    uint256[40] private __gap;
+    uint256[39] private __gap;


I've updated this since we added the mapping above. Just wanted to double confirm that this looks right. I remember seeing a PR a while back changing this to 40.

Yup, this is correct

0xrajath · 2024-10-17T00:15:19Z

src/contracts/core/RewardsCoordinator.sol

+    function processClaims(
+        RewardsMerkleClaim[] calldata claims,
+        address recipient


We could also claim for multiple recipients instead of claiming to a single recipient. Thoughts?

single recipient seems fine

0xrajath · 2024-10-17T00:17:04Z

src/contracts/core/RewardsCoordinator.sol

+     * @param performanceRewardsSubmission PerformanceRewardsSubmission to validate.
+     * @return total amount to be transferred from the avs to the contract.
+     */
+    function _validatePerformanceRewardsSubmission(


Thoughts on using revert with custom errors? Or just custom errors in general instead of error strings? I kept it similar to the current setup but using custom errors will be more gas efficient.

let's do that with slashing like we are on #727

Alright holding off on custom errors for now.

gpsanant · 2024-10-17T02:35:07Z

src/contracts/core/RewardsCoordinator.sol

+
+            uint256 totalAmount = _validatePerformanceRewardsSubmission(performanceRewardsSubmission);
+
+            isAVSPerformanceRewardsSubmissionHash[msg.sender][performanceRewardsSubmissionHash] = true;


do we need to check that it already exists

The nonce keeps monotonically increasing. So it can theoretically never exist already. That was the point of the nonce here.

ah, sounds good

scotthconner · 2024-10-17T14:26:49Z

src/contracts/core/RewardsCoordinator.sol

+                currOperatorAddress < operatorReward.operator,
+                "RewardsCoordinator._validatePerformanceRewardsSubmission: operators must be in ascending order to handle duplicates"


Is this a more gas efficient way of not having a temporary storage slot? Seems rather restrictive on the client but not seemingly impossible I suppose.

Also, who cares if there are duplicates? Is it up to us to infer their intention? Are there any side effects of allowing duplicates?

Is your question: why are we doing this check? If that's the case we're doing it so that the AVS can't pass in duplicate operator addresses into the reward submission, and this is the cheapest way we can check that without doing any searching of the array.

The cli/sdk can easily sort it into this order and warn if there are duplicates before the transaction is submitted onchain.

We do this duplicate check for the Rewards v1 submission. So I'm assuming it's some constraint on the pipeline and to make some calculation easier (I'm just speculating, but I could be wrong - Need to check this assumption).

But besides that why would an AVS need to pay the same operator 2 different amounts in the same reward submission? This seems like a nice constraint to have.

scotthconner · 2024-10-17T14:28:25Z

src/contracts/core/RewardsCoordinator.sol

+            require(
+                strategyManager.strategyIsWhitelistedForDeposit(strategy) || strategy == beaconChainETHStrategy,
+                "RewardsCoordinator._validatePerformanceRewardsSubmission: invalid strategy considered"
+            );


does this propertly support longtail assets?

Good question. It was something I had noted down myself to check.

Rewards works out of the box for longtail assets without overflow since we limit the number of shares that can be minted for any strategy

ypatil12 · 2024-10-17T17:19:32Z

src/contracts/core/RewardsCoordinator.sol

+            require(
+                operatorReward.amount > 0,
+                "RewardsCoordinator._validatePerformanceRewardsSubmission: operator reward amount cannot be 0"
+            );


What are the gas costs for 200,500,1000 operators?

Is that question wrt to this check or in general how much it would cost if 200,500,1000 operators were passed in calldata?

ypatil12 · 2024-10-17T17:28:20Z

src/contracts/interfaces/IRewardsCoordinator.sol

+     *         <--------MAX_RETROACTIVE_LENGTH--------> t (block.timestamp)
+     *         <----valid range for startTimestamp----
+     *     ^
+     * GENESIS_REWARDS_TIMESTAMP


GENESIS_REWARDS_TIMESTAMP is never in range?

What do you mean?

For the performance based rewards it's not possible to submit a reward that starts at GENESIS_REWARDS_TIMESTAMP (ie. scenario B)

ypatil12 · 2024-10-17T17:30:48Z

src/contracts/core/RewardsCoordinator.sol

+            require(
+                strategyManager.strategyIsWhitelistedForDeposit(strategy) || strategy == beaconChainETHStrategy,
+                "RewardsCoordinator._validatePerformanceRewardsSubmission: invalid strategy considered"
+            );


Rewards works out of the box for longtail assets without overflow since we limit the number of shares that can be minted for any strategy

0xrajath · 2024-10-18T00:14:07Z

src/contracts/core/RewardsCoordinatorStorage.sol

@@ -57,7 +57,7 @@ abstract contract RewardsCoordinatorStorage is IRewardsCoordinator {
     */
    DistributionRoot[] internal _distributionRoots;

-    /// Slot 3
+    /// Slot 2


Updated the comment here. It is in fact Slot 2 since we start from Slot 0 (rather than Slot 1)

0xrajath added the enhancement New feature or request label Oct 16, 2024

0xrajath self-assigned this Oct 16, 2024

0xrajath added 5 commits October 16, 2024 19:23

feat: avs performance rewards pauser index

95da83f

feat: IRewardsCoordinator

72d118b

feat: isAVSPerformanceRewardsSubmissionHash

9fa7737

feat: OperatorReward struct

de66c70

feat: _validatePerformanceRewardsSubmission

72cf05e

0xrajath force-pushed the feat/rewards-v2 branch from 3832b9d to 72cf05e Compare October 16, 2024 23:26

0xrajath commented Oct 16, 2024

View reviewed changes

lib/forge-std Outdated

Copy link

Author

0xrajath Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix this. Something happened while rebasing.

0xrajath commented Oct 16, 2024

View reviewed changes

feat: processClaims

3975548

0xrajath commented Oct 17, 2024

View reviewed changes

ypatil12 self-requested a review October 17, 2024 02:34

gpsanant reviewed Oct 17, 2024

View reviewed changes

0xrajath changed the title ~~feat: Rewards V2~~ feat: Rewards v2 Oct 17, 2024

scotthconner reviewed Oct 17, 2024

View reviewed changes

ypatil12 reviewed Oct 17, 2024

View reviewed changes

feat: operatorAVSCommissionBips

179aa1f

0xrajath commented Oct 18, 2024

View reviewed changes

0xrajath added 2 commits October 17, 2024 20:58

feat: setOperatorAVSCommission

d4db04d

feat: delegation aware rewards submission

acd51b3

		/// @notice Mapping: avs => avsPerformanceRewardsSubmissionHash => bool to check if performance rewards submission hash has been submitted
		mapping(address => mapping(bytes32 => bool)) public isAVSPerformanceRewardsSubmissionHash;


		uint256 totalAmount = _validatePerformanceRewardsSubmission(performanceRewardsSubmission);

		isAVSPerformanceRewardsSubmissionHash[msg.sender][performanceRewardsSubmissionHash] = true;

		currOperatorAddress < operatorReward.operator,
		"RewardsCoordinator._validatePerformanceRewardsSubmission: operators must be in ascending order to handle duplicates"

feat: Rewards v2 #837

Are you sure you want to change the base?

feat: Rewards v2 #837

Conversation

0xrajath commented Oct 16, 2024 • edited Loading

Motivation

Modifications

TODO

Open Questions

Result

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

0xrajath Oct 17, 2024 • 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

0xrajath Oct 17, 2024 • 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

Choose a reason for hiding this comment

0xrajath commented Oct 16, 2024 •

edited

Loading

0xrajath Oct 17, 2024 •

edited

Loading

0xrajath Oct 17, 2024 •

edited

Loading