From 54ad03fe644cdd82d6c44ea971e50499c9ca9431 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Tue, 23 Jun 2020 15:34:15 -0700 Subject: [PATCH 1/5] Fix rewards distribution --- eth-contracts/contracts/ClaimsManager.sol | 6 ++- eth-contracts/contracts/DelegateManager.sol | 53 +++++++++++++------ .../contracts/test/MockDelegateManager.sol | 2 +- 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/eth-contracts/contracts/ClaimsManager.sol b/eth-contracts/contracts/ClaimsManager.sol index 4165aa09358..689aca78747 100644 --- a/eth-contracts/contracts/ClaimsManager.sol +++ b/eth-contracts/contracts/ClaimsManager.sol @@ -227,7 +227,7 @@ contract ClaimsManager is InitializableV2 { function processClaim( address _claimer, uint _totalLockedForSP - ) external + ) external returns (uint mintedRewards) { _requireIsInitialized(); require( @@ -262,7 +262,7 @@ contract ClaimsManager is InitializableV2 { // Total rewards can be zero if all stake is currently locked up if (!withinBounds || rewardsForClaimer == 0) { stakingContract.updateClaimHistory(0, _claimer); - return; + return 0; } // ERC20Mintable always returns true @@ -286,6 +286,8 @@ contract ClaimsManager is InitializableV2 { totalStakedAtFundBlockForClaimer, newTotal ); + + return rewardsForClaimer; } /** diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 69e910692b2..33ffb396342 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -338,18 +338,20 @@ contract DelegateManager is InitializableV2 { ( uint totalBalanceInStaking, uint totalBalanceInSPFactory, - uint totalBalanceOutsideStaking + uint totalBalanceOutsideStaking, + uint spLockedStake, + uint totalRewards ) = _validateClaimRewards(spFactory); // No-op if balance is already equivalent // This case can occur if no rewards due to bound violation or all stake is locked - if (totalBalanceInStaking == totalBalanceOutsideStaking) { + if (totalRewards == 0) { return; } // Total rewards // Equal to (balance in staking) - ((balance in sp factory) + (balance in delegate manager)) - uint totalRewards = totalBalanceInStaking.sub(totalBalanceOutsideStaking); + // uint totalRewards = totalBalanceInStaking.sub(totalBalanceOutsideStaking); // Emit claim event emit Claim(msg.sender, totalRewards, totalBalanceInStaking); @@ -361,9 +363,10 @@ contract DelegateManager is InitializableV2 { // Total valid funds used to calculate rewards distribution uint totalActiveFunds = ( - totalBalanceOutsideStaking.sub(spDelegateInfo[msg.sender].totalLockedUpStake) + totalBalanceOutsideStaking.sub( + spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake) + ) ); - // Traverse all delegates and calculate their rewards // As each delegate reward is calculated, increment SP cut reward accordingly for (uint i = 0; i < spDelegateInfo[msg.sender].delegators.length; i++) { @@ -412,8 +415,10 @@ contract DelegateManager is InitializableV2 { ); // Rewards directly allocated to service provider for their stake + // Total active funds for direct deployer reward share + /// totalActiveDeployerFunds = totalBalanceInSPFactory.sub(spLockedStake); uint spRewardShare = ( - totalBalanceInSPFactory.mul(totalRewards) + (totalBalanceInSPFactory.sub(spLockedStake)).mul(totalRewards) ).div(totalActiveFunds); spFactory.updateServiceProviderStake( @@ -700,20 +705,27 @@ contract DelegateManager is InitializableV2 { * @notice Helper function for claimRewards to get balances from Staking contract and do validation * @param spFactory - reference to ServiceProviderFactory contract - * @return (totalBalanceInStaking, totalBalanceInSPFactory, totalBalanceOutsideStaking) + * @return (totalBalanceInStaking, totalBalanceInSPFactory, totalBalanceOutsideStaking, spLockedStake, totalRewards) */ function _validateClaimRewards(ServiceProviderFactory spFactory) - internal returns (uint totalBalanceInStaking, uint totalBalanceInSPFactory, uint totalBalanceOutsideStaking) - { + internal returns ( + uint totalBalanceInStaking, + uint totalBalanceInSPFactory, + uint totalBalanceOutsideStaking, + uint spLockedStake, + uint totalRewards + ) + { // Account for any pending locked up stake for the service provider - (uint spLockedStake,) = spFactory.getPendingDecreaseStakeRequest(msg.sender); + (spLockedStake,) = spFactory.getPendingDecreaseStakeRequest(msg.sender); + uint totalLockedUpStake = spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake); // Process claim for msg.sender // Total locked parameter is equal to delegate locked up stake + service provider locked up stake - ClaimsManager(claimsManagerAddress).processClaim( + uint mintedRewards = ClaimsManager(claimsManagerAddress).processClaim( msg.sender, - (spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake)) + totalLockedUpStake ); // Amount stored in staking contract for owner @@ -722,10 +734,6 @@ contract DelegateManager is InitializableV2 { // Amount in sp factory for claimer (uint _totalBalanceInSPFactory,,,,,) = spFactory.getServiceProviderDetails(msg.sender); - - // Decrease total balance by any locked up stake - _totalBalanceInSPFactory = _totalBalanceInSPFactory.sub(spLockedStake); - // Require active stake to claim any rewards require(_totalBalanceInSPFactory > 0, "Service Provider stake required"); @@ -734,7 +742,18 @@ contract DelegateManager is InitializableV2 { _totalBalanceInSPFactory.add(spDelegateInfo[msg.sender].totalDelegatedStake) ); - return (_totalBalanceInStaking, _totalBalanceInSPFactory, _totalBalanceOutsideStaking); + require( + mintedRewards == _totalBalanceInStaking.sub(_totalBalanceOutsideStaking), + "Reward amount mismatch" + ); + + return ( + _totalBalanceInStaking, + _totalBalanceInSPFactory, + _totalBalanceOutsideStaking, + spLockedStake, + mintedRewards + ); } /** diff --git a/eth-contracts/contracts/test/MockDelegateManager.sol b/eth-contracts/contracts/test/MockDelegateManager.sol index 5c6b51842de..55b15936e4a 100644 --- a/eth-contracts/contracts/test/MockDelegateManager.sol +++ b/eth-contracts/contracts/test/MockDelegateManager.sol @@ -21,7 +21,7 @@ contract MockDelegateManager is InitializableV2 { function testProcessClaim( address _claimer, uint _totalLockedForSP - ) external { + ) external returns (uint) { ClaimsManager claimsManager = ClaimsManager( claimsManagerAddress ); From d5b540027881b4c562f03c9badeb5e7367fcf69c Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Tue, 23 Jun 2020 15:34:49 -0700 Subject: [PATCH 2/5] Updated test --- eth-contracts/test/delegateManager.test.js | 25 ++++++++++++---------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/eth-contracts/test/delegateManager.test.js b/eth-contracts/test/delegateManager.test.js index 31b20ece39b..9790a25ded5 100644 --- a/eth-contracts/test/delegateManager.test.js +++ b/eth-contracts/test/delegateManager.test.js @@ -270,9 +270,10 @@ contract('DelegateManager', async (accounts) => { let spDetails = await serviceProviderFactory.getServiceProviderDetails(account) spFactoryStake = spDetails.deployerStake totalInStakingContract = await staking.totalStakedFor(account) + let spDecreaseRequest = await serviceProviderFactory.getPendingDecreaseStakeRequest(account) let delegatedStake = await delegateManager.getTotalDelegatedToServiceProvider(account) - let lockedUpStake = await delegateManager.getTotalLockedDelegationForServiceProvider(account) + let lockedUpDelegatorStake = await delegateManager.getTotalLockedDelegationForServiceProvider(account) let delegatorInfo = {} let delegators = await delegateManager.getDelegatorsList(account) for (var i = 0; i < delegators.length; i++) { @@ -286,7 +287,7 @@ contract('DelegateManager', async (accounts) => { } } let outsideStake = spFactoryStake.add(delegatedStake) - let totalActiveStake = outsideStake.sub(lockedUpStake) + let totalActiveStake = outsideStake.sub(lockedUpDelegatorStake) let stakeDiscrepancy = totalInStakingContract.sub(outsideStake) let accountSummary = { totalInStakingContract, @@ -294,8 +295,9 @@ contract('DelegateManager', async (accounts) => { spFactoryStake, delegatorInfo, outsideStake, - lockedUpStake, - totalActiveStake + lockedUpDelegatorStake, + totalActiveStake, + spDecreaseRequest } if (print) { @@ -914,7 +916,7 @@ contract('DelegateManager', async (accounts) => { ) let preSlashInfo = await getAccountStakeInfo(stakerAccount, false) - let preSlashLockupStake = preSlashInfo.lockedUpStake + let preSlashLockupStake = preSlashInfo.lockedUpDelegatorStake assert.isTrue( preSlashLockupStake.eq(initialDelegateAmount), 'Initial delegate amount not found') @@ -924,7 +926,7 @@ contract('DelegateManager', async (accounts) => { let postRewardInfo = await getAccountStakeInfo(stakerAccount, false) - let postSlashLockupStake = postRewardInfo.lockedUpStake + let postSlashLockupStake = postRewardInfo.lockedUpDelegatorStake assert.equal( postSlashLockupStake, 0, @@ -1483,11 +1485,12 @@ contract('DelegateManager', async (accounts) => { let fundsPerRound = await claimsManager.getFundsPerRound() let expectedIncrease = fundsPerRound.div(_lib.toBN(4)) // Request decrease in stake corresponding to 1/2 of DEFAULT_AMOUNT - await serviceProviderFactory.requestDecreaseStake(DEFAULT_AMOUNT.div(_lib.toBN(2)), { from: stakerAccount }) - let info = await getAccountStakeInfo(stakerAccount) + let decreaseStakeAmount = DEFAULT_AMOUNT.div(_lib.toBN(2)) + await serviceProviderFactory.requestDecreaseStake(decreaseStakeAmount, { from: stakerAccount }) + let info = await getAccountStakeInfo(stakerAccount, true) await claimsManager.initiateRound({ from: stakerAccount }) await delegateManager.claimRewards({ from: stakerAccount }) - let info2 = await getAccountStakeInfo(stakerAccount) + let info2 = await getAccountStakeInfo(stakerAccount, true) let stakingDiff = (info2.totalInStakingContract).sub(info.totalInStakingContract) let spFactoryDiff = (info2.spFactoryStake).sub(info.spFactoryStake) assert.isTrue(stakingDiff.eq(expectedIncrease), 'Expected increase not found in Staking.sol') @@ -1499,7 +1502,7 @@ contract('DelegateManager', async (accounts) => { let requestInfo = await serviceProviderFactory.getPendingDecreaseStakeRequest(stakerAccount) assert.isTrue((requestInfo.lockupExpiryBlock).gt(_lib.toBN(0)), 'Expected lockup expiry block to be set') assert.isTrue((requestInfo.amount).gt(_lib.toBN(0)), 'Expected amount to be set') - + // Slash await _lib.slash(_lib.audToWei(5), slasherAccount, governance, delegateManagerKey, guardianAddress) @@ -1512,7 +1515,7 @@ contract('DelegateManager', async (accounts) => { let duration = await serviceProviderFactory.getDecreaseStakeLockupDuration() // Double decrease stake duration let newDuration = duration.add(duration) - + await _lib.assertRevert( serviceProviderFactory.updateDecreaseStakeLockupDuration(newDuration), "Only callable by Governance contract" From 22a48b54a0488b7488407e6b09f9dc92a938b0d6 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Tue, 23 Jun 2020 17:40:15 -0700 Subject: [PATCH 3/5] Evaluate active stake on rewards --- eth-contracts/contracts/DelegateManager.sol | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 33ffb396342..7dc22670066 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -716,7 +716,6 @@ contract DelegateManager is InitializableV2 { uint totalRewards ) { - // Account for any pending locked up stake for the service provider (spLockedStake,) = spFactory.getPendingDecreaseStakeRequest(msg.sender); uint totalLockedUpStake = spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake); @@ -729,28 +728,28 @@ contract DelegateManager is InitializableV2 { ); // Amount stored in staking contract for owner - uint _totalBalanceInStaking = Staking(stakingAddress).totalStakedFor(msg.sender); - require(_totalBalanceInStaking > 0, "Stake required for claim"); + totalBalanceInStaking = Staking(stakingAddress).totalStakedFor(msg.sender); + require(totalBalanceInStaking > 0, "Stake required for claim"); // Amount in sp factory for claimer - (uint _totalBalanceInSPFactory,,,,,) = spFactory.getServiceProviderDetails(msg.sender); + (totalBalanceInSPFactory,,,,,) = spFactory.getServiceProviderDetails(msg.sender); // Require active stake to claim any rewards - require(_totalBalanceInSPFactory > 0, "Service Provider stake required"); + require(totalBalanceInSPFactory.sub(spLockedStake) > 0, "Service Provider stake required"); // Amount in delegate manager staked to service provider - uint _totalBalanceOutsideStaking = ( - _totalBalanceInSPFactory.add(spDelegateInfo[msg.sender].totalDelegatedStake) + totalBalanceOutsideStaking = ( + totalBalanceInSPFactory.add(spDelegateInfo[msg.sender].totalDelegatedStake) ); require( - mintedRewards == _totalBalanceInStaking.sub(_totalBalanceOutsideStaking), + mintedRewards == totalBalanceInStaking.sub(totalBalanceOutsideStaking), "Reward amount mismatch" ); return ( - _totalBalanceInStaking, - _totalBalanceInSPFactory, - _totalBalanceOutsideStaking, + totalBalanceInStaking, + totalBalanceInSPFactory, + totalBalanceOutsideStaking, spLockedStake, mintedRewards ); From 34c6b22f44fac5b9c1be9eb9fd003826b5101987 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Wed, 24 Jun 2020 10:25:44 -0700 Subject: [PATCH 4/5] Removing unused field --- eth-contracts/contracts/DelegateManager.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index 7dc22670066..ce950cee787 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -351,7 +351,6 @@ contract DelegateManager is InitializableV2 { // Total rewards // Equal to (balance in staking) - ((balance in sp factory) + (balance in delegate manager)) - // uint totalRewards = totalBalanceInStaking.sub(totalBalanceOutsideStaking); // Emit claim event emit Claim(msg.sender, totalRewards, totalBalanceInStaking); From 896cf99cb11c802e12ffe60b2f8f169f4202ab85 Mon Sep 17 00:00:00 2001 From: Hareesh Nagaraj Date: Thu, 25 Jun 2020 11:00:34 -0700 Subject: [PATCH 5/5] Remove dupe op --- eth-contracts/contracts/DelegateManager.sol | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/eth-contracts/contracts/DelegateManager.sol b/eth-contracts/contracts/DelegateManager.sol index ce950cee787..b52df5af7a0 100644 --- a/eth-contracts/contracts/DelegateManager.sol +++ b/eth-contracts/contracts/DelegateManager.sol @@ -338,7 +338,7 @@ contract DelegateManager is InitializableV2 { ( uint totalBalanceInStaking, uint totalBalanceInSPFactory, - uint totalBalanceOutsideStaking, + uint totalActiveFunds, uint spLockedStake, uint totalRewards ) = _validateClaimRewards(spFactory); @@ -360,12 +360,6 @@ contract DelegateManager is InitializableV2 { uint spDeployerCutRewards = 0; uint totalDelegatedStakeIncrease = 0; - // Total valid funds used to calculate rewards distribution - uint totalActiveFunds = ( - totalBalanceOutsideStaking.sub( - spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake) - ) - ); // Traverse all delegates and calculate their rewards // As each delegate reward is calculated, increment SP cut reward accordingly for (uint i = 0; i < spDelegateInfo[msg.sender].delegators.length; i++) { @@ -704,13 +698,13 @@ contract DelegateManager is InitializableV2 { * @notice Helper function for claimRewards to get balances from Staking contract and do validation * @param spFactory - reference to ServiceProviderFactory contract - * @return (totalBalanceInStaking, totalBalanceInSPFactory, totalBalanceOutsideStaking, spLockedStake, totalRewards) + * @return (totalBalanceInStaking, totalBalanceInSPFactory, totalActiveFunds, spLockedStake, totalRewards) */ function _validateClaimRewards(ServiceProviderFactory spFactory) internal returns ( uint totalBalanceInStaking, uint totalBalanceInSPFactory, - uint totalBalanceOutsideStaking, + uint totalActiveFunds, uint spLockedStake, uint totalRewards ) @@ -736,10 +730,12 @@ contract DelegateManager is InitializableV2 { require(totalBalanceInSPFactory.sub(spLockedStake) > 0, "Service Provider stake required"); // Amount in delegate manager staked to service provider - totalBalanceOutsideStaking = ( + uint totalBalanceOutsideStaking = ( totalBalanceInSPFactory.add(spDelegateInfo[msg.sender].totalDelegatedStake) ); + totalActiveFunds = totalBalanceOutsideStaking.sub(totalLockedUpStake); + require( mintedRewards == totalBalanceInStaking.sub(totalBalanceOutsideStaking), "Reward amount mismatch" @@ -748,7 +744,7 @@ contract DelegateManager is InitializableV2 { return ( totalBalanceInStaking, totalBalanceInSPFactory, - totalBalanceOutsideStaking, + totalActiveFunds, spLockedStake, mintedRewards );