From 2f6763b21bcf99edb1f319170a72c838c45068a3 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Fri, 17 Jul 2020 14:32:34 -0700 Subject: [PATCH 1/9] WIP - contract functionality there, cleanup still pending --- eth-contracts/contracts/DelegateManager.sol | 127 +++++++++++++++++++- eth-contracts/test/delegateManager.test.js | 23 +++- 2 files changed, 146 insertions(+), 4 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 989ff19ee32..97c50167806 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -40,6 +40,18 @@ contract DelegateManager is InitializableV2 { // Minimum amount of delegation allowed uint256 private minDelegationAmount; + /** + * Lockup duration for a remove delegator request + * @notice must be >= Governance.votingPeriod + */ + uint256 private removeDelegatorLockupDuration; + + /** + * Evaluation period for a remove delegator request + * @notice added to expiry block calculated for removeDelegatorLockupDuration + */ + uint256 private removeDelegatorEvalDuration; + // Staking contract ref ERC20Mintable private audiusToken; @@ -67,6 +79,10 @@ contract DelegateManager is InitializableV2 { // Requester to pending undelegate request mapping (address => UndelegateStakeRequest) private undelegateRequests; + // Pending remove delegator requests + // service provider -> (delegator -> lockupExpiryBlock) + mapping (address => mapping (address => uint256)) private removeDelegatorRequests; + event IncreaseDelegatedStake( address indexed _delegator, address indexed _serviceProvider, @@ -120,6 +136,10 @@ contract DelegateManager is InitializableV2 { // Default minimum delegation amount set to 100AUD minDelegationAmount = 100 * 10**uint256(18); InitializableV2.initialize(); + + // TODO: Hard code legit values or take this in constructor + removeDelegatorLockupDuration = 100; + removeDelegatorEvalDuration = 10; } /** @@ -514,7 +534,57 @@ contract DelegateManager is InitializableV2 { } /** - * @notice Allow a service provider to forcibly remove a delegator + * @notice Initiate forcible removal of a delegator + * @param _serviceProvider - address of service provider + * @param _delegator - address of delegator + */ + function requestRemoveDelegator(address _serviceProvider, address _delegator) external { + _requireIsInitialized(); + + // TODO: Consolidate error message into const + require( + msg.sender == _serviceProvider || msg.sender == governanceAddress, + "DelegateManager: Only callable by target SP or governance" + ); + + require( + removeDelegatorRequests[_serviceProvider][_delegator] == 0, + "DelegateManager: Pending request" + ); + + // TODO: Consolidate error message to const + require( + _delegatorExistsForSP(_delegator, _serviceProvider), + "DelegateManager: Delegator must be staked for SP" + ); + + // Update lockup + removeDelegatorRequests[_serviceProvider][_delegator] = ( + block.number + removeDelegatorLockupDuration + ); + } + + /** + * @notice Cancel pending removeDelegator request + * @param _serviceProvider - address of service provider + * @param _delegator - address of delegator + */ + function cancelRemoveDelegator(address _serviceProvider, address _delegator) external { + require( + msg.sender == _serviceProvider || msg.sender == governanceAddress, + "DelegateManager: Only callable by target SP or governance" + ); + require( + removeDelegatorRequests[_serviceProvider][_delegator] != 0, + "DelegateManager: No pending request" + ); + + // Reset lockup expiry + removeDelegatorRequests[_serviceProvider][_delegator] = 0; + } + + /** + * @notice Evaluate removeDelegator request * @param _serviceProvider - address of service provider * @param _delegator - address of delegator * @return Updated total amount delegated to the service provider by delegator @@ -527,6 +597,25 @@ contract DelegateManager is InitializableV2 { msg.sender == _serviceProvider || msg.sender == governanceAddress, "DelegateManager: Only callable by target SP or governance" ); + + // TODO: Evaluate + require( + removeDelegatorRequests[_serviceProvider][_delegator] != 0, + "DelegateManager: No pending request" + ); + + // Enforce lockup expiry block + require( + block.number > removeDelegatorRequests[_serviceProvider][_delegator], + "DelegateManager: Lockup must be expired" + ); + + // Envorce evaluation window for request + require( + removeDelegatorRequests[_serviceProvider][_delegator] <= block.number + removeDelegatorEvalDuration, + "DelegateManager: RemoveDelegator evaluation window expired" + ); + uint256 unstakeAmount = delegateInfo[_delegator][_serviceProvider]; // Unstake on behalf of target service provider Staking(stakingAddress).undelegateStakeFor( @@ -706,6 +795,22 @@ contract DelegateManager is InitializableV2 { return (req.serviceProvider, req.amount, req.lockupExpiryBlock); } + /** + * @notice Get status of pending remove delegator request for a given address + * @param _serviceProvider - address of the service provider + * @param _delegator - address of the delegator + * @return - current lockup expiry block for remove delegator request + */ + function getPendingRemoveDelegatorRequest( + address _serviceProvider, + address _delegator + ) external view returns (uint256) + { + _requireIsInitialized(); + + return removeDelegatorRequests[_serviceProvider][_delegator]; + } + /// @notice Get current undelegate lockup duration function getUndelegateLockupDuration() external view returns (uint256) @@ -733,6 +838,26 @@ contract DelegateManager is InitializableV2 { return minDelegationAmount; } + /// @notice Get the duration for remove delegator request lockup + // TODO: Expose a setter + function getRemoveDelegatorLockupDuration() + external view returns (uint256) + { + _requireIsInitialized(); + + return removeDelegatorLockupDuration; + } + + /// @notice Get the duration for evaluation of remove delegator operations + // TODO: Expose a setter + function getRemoveDelegatorEvalDuration() + external view returns (uint256) + { + _requireIsInitialized(); + + return removeDelegatorEvalDuration; + } + /// @notice Get the Governance address function getGovernanceAddress() external view returns (address) { _requireIsInitialized(); diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index 8b11fb84054..a7508ace644 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -1712,14 +1712,27 @@ contract('DelegateManager', async (accounts) => { 'Expect pending request' ) - // fail to removeDelegator from not a SP or governance + // Fail to call removeDelegator from not a SP or governance await _lib.assertRevert( delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: delegatorAccount1 }), "Only callable by target SP or governance" ) - // Forcibly remove the delegator from service provider account + // Confirm failure without a pending request + await _lib.assertRevert( + delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }), + "No pending request" + ) + + // Remove delegator + await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + + let requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1) + + // Move to valid block and actually perform remove + await time.advanceBlockTo(requestTargetBlock) await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + let stakeAfterRemoval = await delegateManager.getDelegatorStakeForServiceProvider(delegatorAccount1, stakerAccount) let delegatorsList = await delegateManager.getDelegatorsList(stakerAccount) pendingUndelegateRequest = await delegateManager.getPendingUndelegateRequest(delegatorAccount1) @@ -1768,7 +1781,11 @@ contract('DelegateManager', async (accounts) => { ) }) - it('Deregister delegator', async () => { + // TODO: Validate behavior around removeDelegator + // - expiry block calculated correctly + // - cancelRemoveDelegator behavior resets request + // - evaluation window enforced + it.only('removeDelegator validation', async () => { const delegationAmount = _lib.toBN(100) // Transfer tokens to delegator await token.transfer(delegatorAccount1, delegationAmount, { from: proxyDeployerAddress }) From dbcbdacf84655e1bbd83e2a446cb0af1cc949aa3 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Fri, 17 Jul 2020 15:19:24 -0700 Subject: [PATCH 2/9] Further validation --- eth-contracts/test/delegateManager.test.js | 59 +++++++++++++++++++--- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index a7508ace644..1670b6fcb91 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -1782,17 +1782,18 @@ contract('DelegateManager', async (accounts) => { }) // TODO: Validate behavior around removeDelegator - // - expiry block calculated correctly + // - expiry block calculated correctly (done) // - cancelRemoveDelegator behavior resets request // - evaluation window enforced + // - Pending request before call to requestRemoveDelegator + // - Unstaked delegator for this sp it.only('removeDelegator validation', async () => { const delegationAmount = _lib.toBN(100) + const delegatorAccount2 = accounts[5] // Transfer tokens to delegator await token.transfer(delegatorAccount1, delegationAmount, { from: proxyDeployerAddress }) - await token.approve( - stakingAddress, - delegationAmount, - { from: delegatorAccount1 }) + await token.transfer(delegatorAccount2, delegationAmount, { from: proxyDeployerAddress }) + await token.approve(stakingAddress, delegationAmount, { from: delegatorAccount1 }) await delegateManager.delegateStake( stakerAccount, delegationAmount, @@ -1805,9 +1806,22 @@ contract('DelegateManager', async (accounts) => { delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: delegatorAccount1 }), "Only callable by target SP or governance" ) + + let removeReqDuration = await delegateManager.getRemoveDelegatorLockupDuration() - // Forcibly remove the delegator from service provider account + // Remove delegator + let tx = await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + let blocknumber = _lib.toBN(tx.receipt.blockNumber) + let expectedTarget = blocknumber.add(removeReqDuration) + + let requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1) + assert.isTrue(requestTargetBlock.eq(expectedTarget), 'Target unexpected') + + // Move to valid block and actually perform remove + await time.advanceBlockTo(requestTargetBlock) await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + + // Forcibly remove the delegator from service provider account let stakeAfterRemoval = await delegateManager.getDelegatorStakeForServiceProvider(delegatorAccount1, stakerAccount) let delegatorsList = await delegateManager.getDelegatorsList(stakerAccount) assert.isTrue(stakeAfterRemoval.eq(_lib.toBN(0)), 'Expect 0 delegated stake') @@ -1816,6 +1830,39 @@ contract('DelegateManager', async (accounts) => { let delegatorTokenBalance2 = await token.balanceOf(delegatorAccount1) let diff = delegatorTokenBalance2.sub(delegatorTokenBalance) assert.isTrue(diff.eq(delegationAmount), 'Expect full delegation amount to be refunded') + + // Try to remove a delegator that does not yet exist, confirm failure + await _lib.assertRevert( + delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }), + 'Delegator must be staked for SP' + ) + + // Delegate from a new account + await token.approve( + stakingAddress, + delegationAmount, + { from: delegatorAccount2 }) + await delegateManager.delegateStake( + stakerAccount, + delegationAmount, + { from: delegatorAccount2 }) + + tx = await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }) + blocknumber = _lib.toBN(tx.receipt.blockNumber) + expectedTarget = blocknumber.add(removeReqDuration) + + requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2) + assert.isTrue(requestTargetBlock.eq(expectedTarget), 'Target unexpected') + + // Call from wrong account + await _lib.assertRevert( + delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2), + 'Only callable by target SP' + ) + await delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }) + + let requestTargetBlockAfterCancel = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2) + assert.isTrue(requestTargetBlockAfterCancel.eq(_lib.toBN(0)), 'Expect reset') }) describe('Service provider decrease stake behavior', async () => { From 70a19d6d6b7e0ba3211ed8e80f2424907f09365f Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Fri, 17 Jul 2020 15:48:46 -0700 Subject: [PATCH 3/9] Pending correct values and set call validation --- eth-contracts/contracts/DelegateManager.sol | 52 ++++++++++++++++----- eth-contracts/test/delegateManager.test.js | 42 +++++++++++++---- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 97c50167806..bf06112b29a 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -22,6 +22,12 @@ contract DelegateManager is InitializableV2 { string private constant ERROR_MINIMUM_DELEGATION = ( "DelegateManager: Minimum delegation amount required" ); + string private constant ERROR_ONLY_SP_GOVERNANCE = ( + "DelegateManager: Only callable by target SP or governance" + ); + string private constant ERROR_DELEGATOR_STAKE = ( + "DelegateManager: Delegator must be staked for SP" + ); address private governanceAddress; address private stakingAddress; @@ -238,7 +244,7 @@ contract DelegateManager is InitializableV2 { address delegator = msg.sender; require( _delegatorExistsForSP(delegator, _target), - "DelegateManager: Delegator must be staked for SP" + ERROR_DELEGATOR_STAKE ); // Confirm no pending delegation request @@ -544,18 +550,17 @@ contract DelegateManager is InitializableV2 { // TODO: Consolidate error message into const require( msg.sender == _serviceProvider || msg.sender == governanceAddress, - "DelegateManager: Only callable by target SP or governance" + ERROR_ONLY_SP_GOVERNANCE ); require( removeDelegatorRequests[_serviceProvider][_delegator] == 0, - "DelegateManager: Pending request" + "DelegateManager: Pending remove delegator request" ); - // TODO: Consolidate error message to const require( _delegatorExistsForSP(_delegator, _serviceProvider), - "DelegateManager: Delegator must be staked for SP" + ERROR_DELEGATOR_STAKE ); // Update lockup @@ -572,7 +577,7 @@ contract DelegateManager is InitializableV2 { function cancelRemoveDelegator(address _serviceProvider, address _delegator) external { require( msg.sender == _serviceProvider || msg.sender == governanceAddress, - "DelegateManager: Only callable by target SP or governance" + ERROR_ONLY_SP_GOVERNANCE ); require( removeDelegatorRequests[_serviceProvider][_delegator] != 0, @@ -595,7 +600,7 @@ contract DelegateManager is InitializableV2 { require( msg.sender == _serviceProvider || msg.sender == governanceAddress, - "DelegateManager: Only callable by target SP or governance" + ERROR_ONLY_SP_GOVERNANCE ); // TODO: Evaluate @@ -606,13 +611,13 @@ contract DelegateManager is InitializableV2 { // Enforce lockup expiry block require( - block.number > removeDelegatorRequests[_serviceProvider][_delegator], + block.number >= removeDelegatorRequests[_serviceProvider][_delegator], "DelegateManager: Lockup must be expired" ); - // Envorce evaluation window for request + // Enforce evaluation window for request require( - removeDelegatorRequests[_serviceProvider][_delegator] <= block.number + removeDelegatorEvalDuration, + block.number < removeDelegatorRequests[_serviceProvider][_delegator] + removeDelegatorEvalDuration, "DelegateManager: RemoveDelegator evaluation window expired" ); @@ -647,6 +652,9 @@ contract DelegateManager is InitializableV2 { // Remove from list of delegators _removeFromDelegatorsList(_serviceProvider, _delegator); + + // Reset lockup expiry + removeDelegatorRequests[_serviceProvider][_delegator] = 0; } /** @@ -688,6 +696,28 @@ contract DelegateManager is InitializableV2 { emit MinDelegationUpdated(_minDelegationAmount); } + /** + * @notice Update remove delegator lockup duration + * @param _duration - new lockup duration + */ + function updateRemoveDelegatorLockupDuration(uint256 _duration) external { + _requireIsInitialized(); + + require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); + removeDelegatorLockupDuration = _duration; + } + + /** + * @notice Update remove delegator evaluation window duration + * @param _duration - new window duration + */ + function updateRemoveDelegatorEvalDuration(uint256 _duration) external { + _requireIsInitialized(); + + require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); + removeDelegatorEvalDuration = _duration; + } + /** * @notice Set the Governance address * @dev Only callable by Governance address @@ -839,7 +869,6 @@ contract DelegateManager is InitializableV2 { } /// @notice Get the duration for remove delegator request lockup - // TODO: Expose a setter function getRemoveDelegatorLockupDuration() external view returns (uint256) { @@ -849,7 +878,6 @@ contract DelegateManager is InitializableV2 { } /// @notice Get the duration for evaluation of remove delegator operations - // TODO: Expose a setter function getRemoveDelegatorEvalDuration() external view returns (uint256) { diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index 1670b6fcb91..8d3ef912835 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -1781,13 +1781,13 @@ contract('DelegateManager', async (accounts) => { ) }) - // TODO: Validate behavior around removeDelegator + // Validate behavior around removeDelegator // - expiry block calculated correctly (done) - // - cancelRemoveDelegator behavior resets request - // - evaluation window enforced - // - Pending request before call to requestRemoveDelegator - // - Unstaked delegator for this sp - it.only('removeDelegator validation', async () => { + // - cancelRemoveDelegator behavior resets request (done) + // - evaluation window enforced (done) + // - invalid delegator for this sp during call to removeDelegator (done) + // - Pending request before call to requestRemoveDelegator + it('removeDelegator validation', async () => { const delegationAmount = _lib.toBN(100) const delegatorAccount2 = accounts[5] // Transfer tokens to delegator @@ -1808,7 +1808,8 @@ contract('DelegateManager', async (accounts) => { ) let removeReqDuration = await delegateManager.getRemoveDelegatorLockupDuration() - + let removeReqEvalDuration = await delegateManager.getRemoveDelegatorEvalDuration() + // Remove delegator let tx = await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) let blocknumber = _lib.toBN(tx.receipt.blockNumber) @@ -1821,6 +1822,9 @@ contract('DelegateManager', async (accounts) => { await time.advanceBlockTo(requestTargetBlock) await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1) + assert.isTrue(requestTargetBlock.eq(_lib.toBN(0)), 'Reset expected') + // Forcibly remove the delegator from service provider account let stakeAfterRemoval = await delegateManager.getDelegatorStakeForServiceProvider(delegatorAccount1, stakerAccount) let delegatorsList = await delegateManager.getDelegatorsList(stakerAccount) @@ -1847,6 +1851,7 @@ contract('DelegateManager', async (accounts) => { delegationAmount, { from: delegatorAccount2 }) + // Request removal tx = await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }) blocknumber = _lib.toBN(tx.receipt.blockNumber) expectedTarget = blocknumber.add(removeReqDuration) @@ -1859,10 +1864,31 @@ contract('DelegateManager', async (accounts) => { delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2), 'Only callable by target SP' ) - await delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }) + // Cancel and validate request + await delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }) let requestTargetBlockAfterCancel = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2) assert.isTrue(requestTargetBlockAfterCancel.eq(_lib.toBN(0)), 'Expect reset') + + // Reissue request + await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }) + requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2) + let evalBlock = requestTargetBlock.add(removeReqEvalDuration) + + // Progress to the evaluation block + await time.advanceBlockTo(evalBlock) + + // Confirm rejection after window + await _lib.assertRevert( + delegateManager.removeDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }), + 'RemoveDelegator evaluation window expired' + ) + + // Retry should fail here as the request has not been cancelled yet, but the window has expired + await _lib.assertRevert( + delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }), + 'Pending remove delegator request' + ) }) describe('Service provider decrease stake behavior', async () => { From d16bd9dc618fd9e7bf5c6dc387b71e262a467243 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Fri, 17 Jul 2020 17:38:43 -0700 Subject: [PATCH 4/9] Enforce relationship between removeDelegator lockup and votingPeriod RM TODO --- eth-contracts/contracts/DelegateManager.sol | 63 ++++++++++++----- eth-contracts/test/delegateManager.test.js | 78 ++++++++++++++++++--- 2 files changed, 113 insertions(+), 28 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index bf06112b29a..e4139ec4142 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -90,27 +90,33 @@ contract DelegateManager is InitializableV2 { mapping (address => mapping (address => uint256)) private removeDelegatorRequests; event IncreaseDelegatedStake( - address indexed _delegator, - address indexed _serviceProvider, - uint256 indexed _increaseAmount + address indexed _delegator, + address indexed _serviceProvider, + uint256 indexed _increaseAmount ); event DecreaseDelegatedStake( - address indexed _delegator, - address indexed _serviceProvider, - uint256 indexed _decreaseAmount + address indexed _delegator, + address indexed _serviceProvider, + uint256 indexed _decreaseAmount ); event Claim( - address indexed _claimer, - uint256 indexed _rewards, - uint256 indexed _newTotal + address indexed _claimer, + uint256 indexed _rewards, + uint256 indexed _newTotal ); event Slash( - address indexed _target, - uint256 indexed _amount, - uint256 indexed _newTotal + address indexed _target, + uint256 indexed _amount, + uint256 indexed _newTotal + ); + + event DelegatorRemoved( + address indexed _serviceProvider, + address indexed _delegator, + uint256 indexed _unstakedAmount ); event MaxDelegatorsUpdated(uint256 indexed _maxDelegators); @@ -120,6 +126,8 @@ contract DelegateManager is InitializableV2 { event StakingAddressUpdated(address indexed _newStakingAddress); event ServiceProviderFactoryAddressUpdated(address indexed _newServiceProviderFactoryAddress); event ClaimsManagerAddressUpdated(address indexed _newClaimsManagerAddress); + event RemoveDelegatorLockupDurationUpdated(uint256 indexed _removeDelegatorLockupDuration); + event RemoveDelegatorEvalDurationUpdated(uint256 indexed _removeDelegatorEvalDuration); /** * @notice Function to initialize the contract @@ -143,9 +151,11 @@ contract DelegateManager is InitializableV2 { minDelegationAmount = 100 * 10**uint256(18); InitializableV2.initialize(); - // TODO: Hard code legit values or take this in constructor - removeDelegatorLockupDuration = 100; - removeDelegatorEvalDuration = 10; + // 72hr * 60 min/hr * 60 sec/min / ~13 sec/block = 199938 blocks + _updateRemoveDelegatorLockupDuration(199938); + + // 24hr * 60min/hr * 60sec/min / ~13 sec/block = 6646 blocks + removeDelegatorEvalDuration = 6646; } /** @@ -547,7 +557,6 @@ contract DelegateManager is InitializableV2 { function requestRemoveDelegator(address _serviceProvider, address _delegator) external { _requireIsInitialized(); - // TODO: Consolidate error message into const require( msg.sender == _serviceProvider || msg.sender == governanceAddress, ERROR_ONLY_SP_GOVERNANCE @@ -603,7 +612,6 @@ contract DelegateManager is InitializableV2 { ERROR_ONLY_SP_GOVERNANCE ); - // TODO: Evaluate require( removeDelegatorRequests[_serviceProvider][_delegator] != 0, "DelegateManager: No pending request" @@ -655,6 +663,7 @@ contract DelegateManager is InitializableV2 { // Reset lockup expiry removeDelegatorRequests[_serviceProvider][_delegator] = 0; + emit DelegatorRemoved(_serviceProvider, _delegator, unstakeAmount); } /** @@ -704,7 +713,9 @@ contract DelegateManager is InitializableV2 { _requireIsInitialized(); require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); - removeDelegatorLockupDuration = _duration; + + _updateRemoveDelegatorLockupDuration(_duration); + emit RemoveDelegatorLockupDurationUpdated(_duration); } /** @@ -715,7 +726,9 @@ contract DelegateManager is InitializableV2 { _requireIsInitialized(); require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); + removeDelegatorEvalDuration = _duration; + emit RemoveDelegatorEvalDurationUpdated(_duration); } /** @@ -727,6 +740,7 @@ contract DelegateManager is InitializableV2 { _requireIsInitialized(); require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); + _updateGovernanceAddress(_governanceAddress); governanceAddress = _governanceAddress; emit GovernanceAddressUpdated(_governanceAddress); @@ -1128,6 +1142,19 @@ contract DelegateManager is InitializableV2 { governanceAddress = _governanceAddress; } + /** + * @notice Set the remove delegator lockup duration after validating against governance + * @param _duration - Incoming duration value + */ + function _updateRemoveDelegatorLockupDuration(uint256 _duration) internal { + Governance governance = Governance(governanceAddress); + require( + _duration > governance.getVotingPeriod() + governance.getExecutionDelay(), + "DelegateManager: removeDelegatorLockupDuration duration must be greater than governance votingPeriod + executionDelay" + ); + removeDelegatorLockupDuration = _duration; + } + /** * @notice Returns if delegator has delegated to a service provider * @param _delegator - address of delegator diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index 8d3ef912835..949ec354f9f 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -174,14 +174,6 @@ contract('DelegateManager', async (accounts) => { delegateManager = await DelegateManager.at(delegateManagerProxy.address) await registry.addContract(delegateManagerKey, delegateManagerProxy.address, { from: proxyDeployerAddress }) - // Clear min delegation amount for testing - await governance.guardianExecuteTransaction( - delegateManagerKey, - _lib.toBN(0), - 'updateMinDelegationAmount(uint256)', - _lib.abiEncode(['uint256'], [0]), - { from: guardianAddress } - ) // ---- Configuring addresses await _lib.configureGovernanceStakingAddress( governance, @@ -259,6 +251,58 @@ contract('DelegateManager', async (accounts) => { claimsManagerProxy.address, delegateManagerProxy.address ) + + // Clear min delegation amount for testing + let updateTx = await governance.guardianExecuteTransaction( + delegateManagerKey, + _lib.toBN(0), + 'updateMinDelegationAmount(uint256)', + _lib.abiEncode(['uint256'], [0]), + { from: guardianAddress } + ) + await expectEvent.inTransaction( + updateTx.tx, + DelegateManager, + 'MinDelegationUpdated', + { _minDelegationAmount: '0' } + ) + // Expect revert for 8 since it is below votingPeriod + votingDelay + await _lib.assertRevert( + governance.guardianExecuteTransaction( + delegateManagerKey, + _lib.toBN(0), + 'updateRemoveDelegatorLockupDuration(uint256)', + _lib.abiEncode(['uint256'], [8]), + { from: guardianAddress } + ) + ) + // Reset lockup and eval duration for testing + updateTx = await governance.guardianExecuteTransaction( + delegateManagerKey, + _lib.toBN(0), + 'updateRemoveDelegatorLockupDuration(uint256)', + _lib.abiEncode(['uint256'], [100]), + { from: guardianAddress } + ) + await expectEvent.inTransaction( + updateTx.tx, + DelegateManager, + 'RemoveDelegatorLockupDurationUpdated', + { _removeDelegatorLockupDuration: '100' } + ) + updateTx = await governance.guardianExecuteTransaction( + delegateManagerKey, + _lib.toBN(0), + 'updateRemoveDelegatorEvalDuration(uint256)', + _lib.abiEncode(['uint256'], [10]), + { from: guardianAddress } + ) + await expectEvent.inTransaction( + updateTx.tx, + DelegateManager, + 'RemoveDelegatorEvalDurationUpdated', + { _removeDelegatorEvalDuration: '10' } + ) }) /* Helper functions */ @@ -1629,6 +1673,14 @@ contract('DelegateManager', async (accounts) => { delegateManager.slash(10, slasherAccount), "Only callable by Governance contract" ) + await _lib.assertRevert( + delegateManager.updateRemoveDelegatorLockupDuration(10, { from: accounts[3] }), + "Only callable by Governance contract" + ) + await _lib.assertRevert( + delegateManager.updateRemoveDelegatorEvalDuration(10, { from: accounts[3] }), + "Only callable by Governance contract" + ) }) it('Fail to set service addresses from non-governance contract', async () => { @@ -1820,7 +1872,13 @@ contract('DelegateManager', async (accounts) => { // Move to valid block and actually perform remove await time.advanceBlockTo(requestTargetBlock) - await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + tx = await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }) + await expectEvent.inTransaction( + tx.tx, + DelegateManager, + 'DelegatorRemoved', + { _serviceProvider: stakerAccount, _delegator: delegatorAccount1 } + ) requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1) assert.isTrue(requestTargetBlock.eq(_lib.toBN(0)), 'Reset expected') @@ -1857,7 +1915,7 @@ contract('DelegateManager', async (accounts) => { expectedTarget = blocknumber.add(removeReqDuration) requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2) - assert.isTrue(requestTargetBlock.eq(expectedTarget), 'Target unexpected') + assert.isTrue(requestTargetBlock.eq(expectedTarget), 'Target block unexpected') // Call from wrong account await _lib.assertRevert( From e48c13e8ad5b38f6b2703be35f1ffb12483c6900 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Fri, 17 Jul 2020 17:45:21 -0700 Subject: [PATCH 5/9] Missing event validation --- eth-contracts/test/delegateManager.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index 949ec354f9f..0beae26a2cc 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -1877,7 +1877,7 @@ contract('DelegateManager', async (accounts) => { tx.tx, DelegateManager, 'DelegatorRemoved', - { _serviceProvider: stakerAccount, _delegator: delegatorAccount1 } + { _serviceProvider: stakerAccount, _delegator: delegatorAccount1, _unstakedAmount: delegationAmount } ) requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1) From a7279610b31c3ae1baedaa8deb3248644149036b Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Mon, 20 Jul 2020 12:41:37 -0700 Subject: [PATCH 6/9] Update default to 1week in blocks --- eth-contracts/contracts/DelegateManager.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index e4139ec4142..165de35dd8f 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -151,8 +151,8 @@ contract DelegateManager is InitializableV2 { minDelegationAmount = 100 * 10**uint256(18); InitializableV2.initialize(); - // 72hr * 60 min/hr * 60 sec/min / ~13 sec/block = 199938 blocks - _updateRemoveDelegatorLockupDuration(199938); + // 1 week = 168hrs * 60 min/hr * 60 sec/min / ~13 sec/block = 46523 blocks + _updateRemoveDelegatorLockupDuration(46523); // 24hr * 60min/hr * 60sec/min / ~13 sec/block = 6646 blocks removeDelegatorEvalDuration = 6646; From 86a85c5d67783140c30f2f0ff1feffa6a5414465 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Tue, 21 Jul 2020 15:35:58 -0700 Subject: [PATCH 7/9] Updates --- eth-contracts/contracts/DelegateManager.sol | 6 +++++- eth-contracts/migrations/8_delegate_manager_migration.js | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 165de35dd8f..54d047db229 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -36,6 +36,8 @@ contract DelegateManager is InitializableV2 { /** * Number of blocks an undelegate operation has to wait + * The undelegate operation speed bump is to prevent a delegator from + * attempting to remove their delegation in anticipation of a slash. * @notice must be >= Governance.votingPeriod */ uint256 private undelegateLockupDuration; @@ -47,7 +49,9 @@ contract DelegateManager is InitializableV2 { uint256 private minDelegationAmount; /** - * Lockup duration for a remove delegator request + * Lockup duration for a remove delegator request. + * The remove delegator speed bump is to prevent a service provider from maliciously + * removing a delegator prior to the evaluation of a proposal. * @notice must be >= Governance.votingPeriod */ uint256 private removeDelegatorLockupDuration; diff --git a/eth-contracts/migrations/8_delegate_manager_migration.js b/eth-contracts/migrations/8_delegate_manager_migration.js index eec2106f3aa..a2a0ac04876 100644 --- a/eth-contracts/migrations/8_delegate_manager_migration.js +++ b/eth-contracts/migrations/8_delegate_manager_migration.js @@ -16,9 +16,9 @@ const serviceProviderFactoryKey = web3.utils.utf8ToHex('ServiceProviderFactory') const governanceKey = web3.utils.utf8ToHex('Governance') const delegateManagerKey = web3.utils.utf8ToHex('DelegateManager') -// stake lockup duration = 1 wk in blocks +// undelegate lockup duration = 1 wk in blocks // - 1/13 block/s * 604800 s/wk ~= 46523 block/wk -const decreaseStakeLockupDuration = 46523 +const undelegateLockupDuration = 46523 module.exports = (deployer, network, accounts) => { deployer.then(async () => { @@ -43,7 +43,7 @@ module.exports = (deployer, network, accounts) => { const initializeCallData = _lib.encodeCall( 'initialize', ['address', 'address', 'uint256'], - [token.address, governanceAddress, decreaseStakeLockupDuration] + [token.address, governanceAddress, undelegateLockupDuration] ) const delegateManagerProxy = await deployer.deploy( AudiusAdminUpgradeabilityProxy, From 80ae6580989ad40cb9c1ce16c92285ae0a566f1e Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Tue, 21 Jul 2020 15:43:23 -0700 Subject: [PATCH 8/9] Enforce undelegate lockup relationship to voting period as well --- eth-contracts/contracts/DelegateManager.sol | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 54d047db229..ce9d22c2623 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -149,12 +149,13 @@ contract DelegateManager is InitializableV2 { { _updateGovernanceAddress(_governanceAddress); audiusToken = ERC20Mintable(_tokenAddress); - undelegateLockupDuration = _undelegateLockupDuration; maxDelegators = 175; // Default minimum delegation amount set to 100AUD minDelegationAmount = 100 * 10**uint256(18); InitializableV2.initialize(); + _updateUndelegateLockupDuration(_undelegateLockupDuration); + // 1 week = 168hrs * 60 min/hr * 60 sec/min / ~13 sec/block = 46523 blocks _updateRemoveDelegatorLockupDuration(46523); @@ -679,7 +680,7 @@ contract DelegateManager is InitializableV2 { require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); - undelegateLockupDuration = _duration; + _updateUndelegateLockupDuration(_duration); emit UndelegateLockupDurationUpdated(_duration); } @@ -1148,7 +1149,7 @@ contract DelegateManager is InitializableV2 { /** * @notice Set the remove delegator lockup duration after validating against governance - * @param _duration - Incoming duration value + * @param _duration - Incoming remove delegator duration value */ function _updateRemoveDelegatorLockupDuration(uint256 _duration) internal { Governance governance = Governance(governanceAddress); @@ -1159,6 +1160,19 @@ contract DelegateManager is InitializableV2 { removeDelegatorLockupDuration = _duration; } + /** + * @notice Set the undelegate lockup duration after validating against governance + * @param _duration - Incoming undelegate lockup duration value + */ + function _updateUndelegateLockupDuration(uint256 _duration) internal { + Governance governance = Governance(governanceAddress); + require( + _duration > governance.getVotingPeriod() + governance.getExecutionDelay(), + "DelegateManager: undelegateLockupDuration duration must be greater than governance votingPeriod + executionDelay" + ); + undelegateLockupDuration = _duration; + } + /** * @notice Returns if delegator has delegated to a service provider * @param _delegator - address of delegator From 0b65cde377e83f99d360bf790af2e4c2fe4c5b55 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Tue, 21 Jul 2020 16:05:30 -0700 Subject: [PATCH 9/9] Test fixes --- eth-contracts/test/delegateManager.test.js | 3 ++- eth-contracts/test/governance.test.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index 0beae26a2cc..fe2cfefba72 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -30,6 +30,7 @@ const VOTING_PERIOD = 10 const EXECUTION_DELAY = VOTING_PERIOD const VOTING_QUORUM_PERCENT = 10 const DECREASE_STAKE_LOCKUP_DURATION = 10 +const UNDELEGATE_LOCKUP_DURATION = 21 const callValue0 = _lib.toBN(0) @@ -161,7 +162,7 @@ contract('DelegateManager', async (accounts) => { const delegateManagerInitializeData = _lib.encodeCall( 'initialize', ['address', 'address', 'uint256'], - [token.address, governance.address, 10] + [token.address, governance.address, UNDELEGATE_LOCKUP_DURATION] ) let delegateManager0 = await DelegateManager.new({ from: proxyDeployerAddress }) let delegateManagerProxy = await AudiusAdminUpgradeabilityProxy.new( diff --git a/eth-contracts/test/governance.test.js b/eth-contracts/test/governance.test.js index c5908dbe101..700a4977f14 100644 --- a/eth-contracts/test/governance.test.js +++ b/eth-contracts/test/governance.test.js @@ -53,6 +53,7 @@ contract('Governance.sol', async (accounts) => { const maxInProgressProposals = 20 const maxDescriptionLength = 250 const executionDelay = votingPeriod + const undelegateLockupDuration = 21 // intentionally not using acct0 to make sure no TX accidentally succeeds without specifying sender const [, proxyAdminAddress, proxyDeployerAddress, newUpdateAddress] = accounts @@ -205,7 +206,7 @@ contract('Governance.sol', async (accounts) => { const delegateManagerInitializeData = _lib.encodeCall( 'initialize', ['address', 'address', 'uint256'], - [token.address, governance.address, 10] + [token.address, governance.address, undelegateLockupDuration] ) let delegateManager0 = await DelegateManager.new({ from: proxyDeployerAddress }) let delegateManagerProxy = await AudiusAdminUpgradeabilityProxy.new(