From 0f21f88c449fe6006b2cd076effa42fa44a8f988 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Fri, 4 Oct 2024 16:55:57 -0300 Subject: [PATCH] chore(Horizon): add redelegate option --- .../internal/IHorizonStakingMain.sol | 40 ++++- .../contracts/staking/HorizonStaking.sol | 37 ++++- .../HorizonStakingShared.t.sol | 55 +++++-- .../test/staking/delegation/redelegate.t.sol | 146 ++++++++++++++++++ .../test/staking/delegation/withdraw.t.sol | 85 ++-------- 5 files changed, 269 insertions(+), 94 deletions(-) create mode 100644 packages/horizon/test/staking/delegation/redelegate.t.sol diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol index 8d047ac1d..b144b0ce6 100644 --- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol +++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol @@ -420,6 +420,16 @@ interface IHorizonStakingMain { */ error HorizonStakingInvalidBeneficiaryZeroAddress(); + /** + * @notice Thrown when attempting to redelegate with a serivce provider that is the zero address. + */ + error HorizonStakingInvalidServiceProviderZeroAddress(); + + /** + * @notice Thrown when attempting to redelegate with a verifier that is the zero address. + */ + error HorizonStakingInvalidVerifierZeroAddress(); + // -- Errors: thaw requests -- error HorizonStakingNothingThawing(); @@ -746,7 +756,6 @@ interface IHorizonStakingMain { /** * @notice Withdraw undelegated tokens from a provision after thawing. - * Tokens can be automatically re-delegated to another provision by setting `newServiceProvider`. * @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw * requests in the event that fulfilling all of them results in a gas limit error. * @dev If the delegation pool was completely slashed before withdrawing, calling this function will fulfill @@ -754,20 +763,39 @@ interface IHorizonStakingMain { * * Requirements: * - Must have previously initiated a thaw request using {undelegate}. - * - `newServiceProvider` must either be zero address or have previously provisioned stake to `verifier`. * * Emits {ThawRequestFulfilled}, {ThawRequestsFulfilled} and {DelegatedTokensWithdrawn} events. * * @param serviceProvider The service provider address * @param verifier The verifier address - * @param newServiceProvider The address of a new service provider, if the delegator wants to re-delegate + * @param nThawRequests The number of thaw requests to fulfill. Set to 0 to fulfill all thaw requests. + */ + function withdrawDelegated(address serviceProvider, address verifier, uint256 nThawRequests) external; + + /** + * @notice Re-delegate undelegated tokens from a provision after thawing to a `newServiceProvider` and `newVerifier`. + * @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw + * requests in the event that fulfilling all of them results in a gas limit error. + * + * Requirements: + * - Must have previously initiated a thaw request using {undelegate}. + * - `newServiceProvider` and `newVerifier` must not be the zero address. + * - `newServiceProvider` must have previously provisioned stake to `newVerifier`. + * + * Emits {ThawRequestFulfilled}, {ThawRequestsFulfilled} and {DelegatedTokensWithdrawn} events. + * + * @param oldServiceProvider The old service provider address + * @param oldVerifier The old verifier address + * @param newServiceProvider The address of a new service provider + * @param newVerifier The address of a new verifier * @param minSharesForNewProvider The minimum amount of shares to accept for the new service provider * @param nThawRequests The number of thaw requests to fulfill. Set to 0 to fulfill all thaw requests. */ - function withdrawDelegated( - address serviceProvider, - address verifier, + function redelegate( + address oldServiceProvider, + address oldVerifier, address newServiceProvider, + address newVerifier, uint256 minSharesForNewProvider, uint256 nThawRequests ) external; diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol index 476437ffb..c46670dcb 100644 --- a/packages/horizon/contracts/staking/HorizonStaking.sol +++ b/packages/horizon/contracts/staking/HorizonStaking.sol @@ -319,11 +319,32 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { function withdrawDelegated( address serviceProvider, address verifier, + uint256 nThawRequests + ) external override notPaused { + _withdrawDelegated(serviceProvider, verifier, address(0), address(0), 0, nThawRequests); + } + + /** + * @notice See {IHorizonStakingMain-redelegate}. + */ + function redelegate( + address oldServiceProvider, + address oldVerifier, address newServiceProvider, + address newVerifier, uint256 minSharesForNewProvider, uint256 nThawRequests ) external override notPaused { - _withdrawDelegated(serviceProvider, verifier, newServiceProvider, minSharesForNewProvider, nThawRequests); + require(newServiceProvider != address(0), HorizonStakingInvalidServiceProviderZeroAddress()); + require(newVerifier != address(0), HorizonStakingInvalidVerifierZeroAddress()); + _withdrawDelegated( + oldServiceProvider, + oldVerifier, + newServiceProvider, + newVerifier, + minSharesForNewProvider, + nThawRequests + ); } /** @@ -360,7 +381,14 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { * @notice See {IHorizonStakingMain-withdrawDelegated}. */ function withdrawDelegated(address serviceProvider, address newServiceProvider) external override notPaused { - _withdrawDelegated(serviceProvider, SUBGRAPH_DATA_SERVICE_ADDRESS, newServiceProvider, 0, 0); + _withdrawDelegated( + serviceProvider, + SUBGRAPH_DATA_SERVICE_ADDRESS, + newServiceProvider, + SUBGRAPH_DATA_SERVICE_ADDRESS, + 0, + 0 + ); } /* @@ -851,6 +879,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { address _serviceProvider, address _verifier, address _newServiceProvider, + address _newVerifier, uint256 _minSharesForNewProvider, uint256 _nThawRequests ) private { @@ -882,8 +911,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { pool.tokensThawing = tokensThawing; if (tokensThawed != 0) { - if (_newServiceProvider != address(0)) { - _delegate(_newServiceProvider, _verifier, tokensThawed, _minSharesForNewProvider); + if (_newServiceProvider != address(0) && _newVerifier != address(0)) { + _delegate(_newServiceProvider, _newVerifier, tokensThawed, _minSharesForNewProvider); } else { _graphToken().pushTokens(msg.sender, tokensThawed); } diff --git a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol index d6d88283f..1fe4ceba3 100644 --- a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol +++ b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol @@ -1004,9 +1004,26 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest { } function _withdrawDelegated( + address serviceProvider, + address verifier, + uint256 nThawRequests + ) internal { + __withdrawDelegated( + serviceProvider, + verifier, + address(0), + address(0), + 0, + nThawRequests, + false + ); + } + + function _redelegate( address serviceProvider, address verifier, address newServiceProvider, + address newVerifier, uint256 minSharesForNewProvider, uint256 nThawRequests ) internal { @@ -1014,6 +1031,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest { serviceProvider, verifier, newServiceProvider, + newVerifier, minSharesForNewProvider, nThawRequests, false @@ -1021,7 +1039,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest { } function _withdrawDelegated(address serviceProvider, address newServiceProvider) internal { - __withdrawDelegated(serviceProvider, subgraphDataServiceLegacyAddress, newServiceProvider, 0, 0, true); + __withdrawDelegated(serviceProvider, subgraphDataServiceLegacyAddress, newServiceProvider, subgraphDataServiceLegacyAddress, 0, 0, true); } struct BeforeValues_WithdrawDelegated { @@ -1045,19 +1063,20 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest { address _serviceProvider, address _verifier, address _newServiceProvider, + address _newVerifier, uint256 _minSharesForNewProvider, uint256 _nThawRequests, bool legacy ) private { (, address msgSender, ) = vm.readCallers(); - bool reDelegate = _newServiceProvider != address(0); + bool reDelegate = _newServiceProvider != address(0) && _newVerifier != address(0); // before BeforeValues_WithdrawDelegated memory beforeValues; beforeValues.pool = _getStorage_DelegationPoolInternal(_serviceProvider, _verifier, legacy); - beforeValues.newPool = _getStorage_DelegationPoolInternal(_newServiceProvider, _verifier, legacy); - beforeValues.newDelegation = _getStorage_Delegation(_serviceProvider, _verifier, msgSender, legacy); + beforeValues.newPool = _getStorage_DelegationPoolInternal(_newServiceProvider, _newVerifier, legacy); + beforeValues.newDelegation = _getStorage_Delegation(_newServiceProvider, _newVerifier, msgSender, legacy); beforeValues.thawRequestList = staking.getThawRequestList(_serviceProvider, _verifier, msgSender); beforeValues.senderBalance = token.balanceOf(msgSender); beforeValues.stakingBalance = token.balanceOf(address(staking)); @@ -1095,7 +1114,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest { if (reDelegate) { emit IHorizonStakingMain.TokensDelegated( _newServiceProvider, - _verifier, + _newVerifier, msgSender, calcValues.tokensThawed ); @@ -1104,25 +1123,33 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest { } } vm.expectEmit(); + emit IHorizonStakingMain.DelegatedTokensWithdrawn( _serviceProvider, _verifier, msgSender, calcValues.tokensThawed ); - staking.withdrawDelegated( - _serviceProvider, - _verifier, - _newServiceProvider, - _minSharesForNewProvider, - _nThawRequests - ); + if (legacy) { + staking.withdrawDelegated(_serviceProvider, _newServiceProvider); + } else if (reDelegate) { + staking.redelegate( + _serviceProvider, + _verifier, + _newServiceProvider, + _newVerifier, + _minSharesForNewProvider, + _nThawRequests + ); + } else { + staking.withdrawDelegated(_serviceProvider, _verifier, _nThawRequests); + } // after AfterValues_WithdrawDelegated memory afterValues; afterValues.pool = _getStorage_DelegationPoolInternal(_serviceProvider, _verifier, legacy); - afterValues.newPool = _getStorage_DelegationPoolInternal(_newServiceProvider, _verifier, legacy); - afterValues.newDelegation = _getStorage_Delegation(_newServiceProvider, _verifier, msgSender, legacy); + afterValues.newPool = _getStorage_DelegationPoolInternal(_newServiceProvider, _newVerifier, legacy); + afterValues.newDelegation = _getStorage_Delegation(_newServiceProvider, _newVerifier, msgSender, legacy); afterValues.thawRequestList = staking.getThawRequestList(_serviceProvider, _verifier, msgSender); afterValues.senderBalance = token.balanceOf(msgSender); afterValues.stakingBalance = token.balanceOf(address(staking)); diff --git a/packages/horizon/test/staking/delegation/redelegate.t.sol b/packages/horizon/test/staking/delegation/redelegate.t.sol new file mode 100644 index 000000000..605e6601f --- /dev/null +++ b/packages/horizon/test/staking/delegation/redelegate.t.sol @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.27; + +import "forge-std/Test.sol"; + +import { IHorizonStakingMain } from "../../../contracts/interfaces/internal/IHorizonStakingMain.sol"; + +import { HorizonStakingTest } from "../HorizonStaking.t.sol"; + +contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { + + /* + * MODIFIERS + */ + + modifier useUndelegate(uint256 shares) { + resetPrank(users.delegator); + DelegationInternal memory delegation = _getStorage_Delegation(users.indexer, subgraphDataServiceAddress, users.delegator, false); + shares = bound(shares, 1, delegation.shares); + + _undelegate(users.indexer, subgraphDataServiceAddress, shares); + _; + } + + /* + * HELPERS + */ + + function _setupNewIndexer(uint256 tokens) private returns(address) { + (, address msgSender, ) = vm.readCallers(); + + address newIndexer = createUser("newIndexer"); + vm.startPrank(newIndexer); + _createProvision(newIndexer, subgraphDataServiceAddress, tokens, 0, MAX_THAWING_PERIOD); + + vm.startPrank(msgSender); + return newIndexer; + } + + function _setupNewIndexerAndVerifier(uint256 tokens) private returns(address, address) { + (, address msgSender,) = vm.readCallers(); + + address newIndexer = createUser("newIndexer"); + address newVerifier = makeAddr("newVerifier"); + vm.startPrank(newIndexer); + _createProvision(newIndexer, newVerifier, tokens, 0, MAX_THAWING_PERIOD); + + vm.startPrank(msgSender); + return (newIndexer, newVerifier); + } + + /* + * TESTS + */ + + function testRedelegate_MoveToNewServiceProvider( + uint256 delegationAmount, + uint256 withdrawShares + ) + public + useIndexer + useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) + useDelegation(delegationAmount) + useUndelegate(withdrawShares) + { + skip(MAX_THAWING_PERIOD + 1); + + // Setup new service provider + address newIndexer = _setupNewIndexer(10_000_000 ether); + _redelegate(users.indexer, subgraphDataServiceAddress, newIndexer, subgraphDataServiceAddress, 0, 0); + } + + function testRedelegate_MoveToNewServiceProviderAndNewVerifier( + uint256 delegationAmount, + uint256 withdrawShares + ) + public + useIndexer + useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) + useDelegation(delegationAmount) + useUndelegate(withdrawShares) + { + skip(MAX_THAWING_PERIOD + 1); + + // Setup new service provider + (address newIndexer, address newVerifier) = _setupNewIndexerAndVerifier(10_000_000 ether); + _redelegate(users.indexer, subgraphDataServiceAddress, newIndexer, newVerifier, 0, 0); + } + + function testRedelegate_RevertWhen_VerifierZeroAddress( + uint256 delegationAmount + ) + public + useIndexer + useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) + useDelegation(delegationAmount) + useUndelegate(delegationAmount) + { + skip(MAX_THAWING_PERIOD + 1); + + // Setup new service provider + address newIndexer = _setupNewIndexer(10_000_000 ether); + vm.expectRevert(abi.encodeWithSelector(IHorizonStakingMain.HorizonStakingInvalidVerifierZeroAddress.selector)); + staking.redelegate(users.indexer, subgraphDataServiceAddress, newIndexer, address(0), 0, 0); + } + + function testRedelegate_RevertWhen_ServiceProviderZeroAddress( + uint256 delegationAmount + ) + public + useIndexer + useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) + useDelegation(delegationAmount) + useUndelegate(delegationAmount) + { + skip(MAX_THAWING_PERIOD + 1); + + // Setup new verifier + address newVerifier = makeAddr("newVerifier"); + vm.expectRevert(abi.encodeWithSelector(IHorizonStakingMain.HorizonStakingInvalidServiceProviderZeroAddress.selector)); + staking.redelegate(users.indexer, subgraphDataServiceAddress, address(0), newVerifier, 0, 0); + } + + function testRedelegate_MoveZeroTokensToNewServiceProviderAndVerifier( + uint256 delegationAmount, + uint256 withdrawShares + ) + public + useIndexer + useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) + useDelegation(delegationAmount) + useUndelegate(withdrawShares) + { + // Setup new service provider + (address newIndexer, address newVerifier) = _setupNewIndexerAndVerifier(10_000_000 ether); + + uint256 previousBalance = token.balanceOf(users.delegator); + _redelegate(users.indexer, subgraphDataServiceAddress, newIndexer, newVerifier, 0, 0); + + uint256 newBalance = token.balanceOf(users.delegator); + assertEq(newBalance, previousBalance); + + uint256 delegatedTokens = staking.getDelegatedTokensAvailable(newIndexer, newVerifier); + assertEq(delegatedTokens, 0); + } +} \ No newline at end of file diff --git a/packages/horizon/test/staking/delegation/withdraw.t.sol b/packages/horizon/test/staking/delegation/withdraw.t.sol index c66126e1c..2b6c7a76d 100644 --- a/packages/horizon/test/staking/delegation/withdraw.t.sol +++ b/packages/horizon/test/staking/delegation/withdraw.t.sol @@ -14,8 +14,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { */ modifier useUndelegate(uint256 shares) { - vm.stopPrank(); - vm.startPrank(users.delegator); + resetPrank(users.delegator); DelegationInternal memory delegation = _getStorage_Delegation( users.indexer, subgraphDataServiceAddress, @@ -28,20 +27,6 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { _; } - /* - * HELPERS - */ - function _setupNewIndexer(uint256 tokens) private returns (address) { - (, address msgSender, ) = vm.readCallers(); - - address newIndexer = createUser("newIndexer"); - vm.startPrank(newIndexer); - _createProvision(newIndexer, subgraphDataServiceAddress, tokens, 0, MAX_THAWING_PERIOD); - - vm.startPrank(msgSender); - return newIndexer; - } - /* * TESTS */ @@ -65,7 +50,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { skip(thawRequest.thawingUntil + 1); - _withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 0); + _withdrawDelegated(users.indexer, subgraphDataServiceAddress, 0); } function testWithdrawDelegation_RevertWhen_NotThawing( @@ -73,23 +58,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { ) public useIndexer useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) useDelegation(delegationAmount) { bytes memory expectedError = abi.encodeWithSignature("HorizonStakingNothingThawing()"); vm.expectRevert(expectedError); - staking.withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 0); - } - - function testWithdrawDelegation_MoveToNewServiceProvider( - uint256 delegationAmount - ) - public - useIndexer - useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) - useDelegation(delegationAmount) - useUndelegate(delegationAmount) - { - skip(MAX_THAWING_PERIOD + 1); - - // Setup new service provider - address newIndexer = _setupNewIndexer(10_000_000 ether); - _withdrawDelegated(users.indexer, subgraphDataServiceAddress, newIndexer, 0, 0); + staking.withdrawDelegated(users.indexer, subgraphDataServiceAddress, 0); } function testWithdrawDelegation_ZeroTokens( @@ -102,33 +71,11 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { useUndelegate(delegationAmount) { uint256 previousBalance = token.balanceOf(users.delegator); - _withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 0); - - // Nothing changed since thawing period haven't finished - uint256 newBalance = token.balanceOf(users.delegator); - assertEq(newBalance, previousBalance); - } - - function testWithdrawDelegation_MoveZeroTokensToNewServiceProvider( - uint256 delegationAmount - ) - public - useIndexer - useProvision(10_000_000 ether, 0, MAX_THAWING_PERIOD) - useDelegation(delegationAmount) - useUndelegate(delegationAmount) - { - // Setup new service provider - address newIndexer = _setupNewIndexer(10_000_000 ether); - - uint256 previousBalance = token.balanceOf(users.delegator); - _withdrawDelegated(users.indexer, subgraphDataServiceAddress, newIndexer, 0, 0); - + _withdrawDelegated(users.indexer, subgraphDataServiceAddress, 0); + + // Nothing changed since thawing period hasn't finished uint256 newBalance = token.balanceOf(users.delegator); assertEq(newBalance, previousBalance); - - uint256 delegatedTokens = staking.getDelegatedTokensAvailable(newIndexer, subgraphDataServiceAddress); - assertEq(delegatedTokens, 0); } function testWithdrawDelegation_LegacySubgraphService(uint256 delegationAmount) public useIndexer { @@ -182,14 +129,12 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { // fast forward in time and attempt to withdraw skip(MAX_THAWING_PERIOD + 1); resetPrank(users.delegator); - vm.expectRevert( - abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidDelegationPoolState.selector, - users.indexer, - subgraphDataServiceAddress - ) - ); - staking.withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 0); + vm.expectRevert(abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPoolState.selector, + users.indexer, + subgraphDataServiceAddress + )); + staking.withdrawDelegated(users.indexer, subgraphDataServiceAddress, 0); } function testWithdrawDelegation_AfterRecoveringPool( @@ -222,7 +167,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { // fast forward in time and withdraw - this withdraw will net 0 tokens skip(MAX_THAWING_PERIOD + 1); resetPrank(users.delegator); - _withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 0); + _withdrawDelegated(users.indexer, subgraphDataServiceAddress, 0); } function testWithdrawDelegation_WithBeneficiary( @@ -250,7 +195,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { // Beneficiary withdraws delegated tokens resetPrank(beneficiary); - _withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 1); + _withdrawDelegated(users.indexer, subgraphDataServiceAddress, 1); } function testWithdrawDelegation_RevertWhen_PreviousOwnerAttemptsToWithdraw( @@ -277,6 +222,6 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest { // Delegator attempts to withdraw delegated tokens, should revert since beneficiary is the thaw request owner bytes memory expectedError = abi.encodeWithSelector(IHorizonStakingMain.HorizonStakingNothingThawing.selector); vm.expectRevert(expectedError); - staking.withdrawDelegated(users.indexer, subgraphDataServiceAddress, address(0), 0, 1); + staking.withdrawDelegated(users.indexer, subgraphDataServiceAddress, 1); } }