From 4b9491f230617a140361769b59ec01700a5added Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Sun, 5 Jun 2022 15:17:14 -0700 Subject: [PATCH 01/21] feat: keeper reward for reservoir drip through token issuance --- contracts/l2/reservoir/IL2Reservoir.sol | 11 +- contracts/l2/reservoir/L2Reservoir.sol | 15 +- contracts/l2/reservoir/L2ReservoirStorage.sol | 5 + contracts/reservoir/L1Reservoir.sol | 58 +++++- contracts/reservoir/L1ReservoirStorage.sol | 7 + test/l2/l2Reservoir.test.ts | 28 ++- test/reservoir/l1Reservoir.test.ts | 170 ++++++++++++++---- test/rewards/rewards.test.ts | 6 +- 8 files changed, 254 insertions(+), 46 deletions(-) diff --git a/contracts/l2/reservoir/IL2Reservoir.sol b/contracts/l2/reservoir/IL2Reservoir.sol index 90b089ac9..5fd2fc861 100644 --- a/contracts/l2/reservoir/IL2Reservoir.sol +++ b/contracts/l2/reservoir/IL2Reservoir.sol @@ -18,13 +18,22 @@ interface IL2Reservoir is IReservoir { * updates the issuanceBase and issuanceRate, * and snapshots the accumulated rewards. If issuanceRate changes, * it also triggers a snapshot of rewards per signal on the RewardsManager. + * Note that the transaction might revert if it's received out-of-order, + * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed + * again once the ticket for previous drip has been redeemed. + * A keeper reward will be sent to the keeper that dripped on L1, and part of it + * to whoever redeemed the current retryable ticket (tx.origin) * @param _issuanceBase Base value for token issuance (approximation for token supply times L2 rewards fraction) * @param _issuanceRate Rewards issuance rate, using fixed point at 1e18, and including a +1 * @param _nonce Incrementing nonce to ensure messages are received in order + * @param _keeperReward Keeper reward to distribute between keeper that called drip and keeper that redeemed the retryable tx + * @param _l1Keeper Address of the keeper that called drip in L1 */ function receiveDrip( uint256 _issuanceBase, uint256 _issuanceRate, - uint256 _nonce + uint256 _nonce, + uint256 _keeperReward, + address _l1Keeper ) external; } diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 1bc93c912..3b1dd131d 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -16,7 +16,7 @@ import "./L2ReservoirStorage.sol"; * It receives tokens for rewards from L1, and provides functions to compute accumulated and new * total rewards at a particular block number. */ -contract L2Reservoir is L2ReservoirV1Storage, Reservoir, IL2Reservoir { +contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { using SafeMath for uint256; event DripReceived(uint256 _issuanceBase); @@ -88,14 +88,20 @@ contract L2Reservoir is L2ReservoirV1Storage, Reservoir, IL2Reservoir { * Note that the transaction might revert if it's received out-of-order, * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed * again once the ticket for previous drip has been redeemed. + * A keeper reward will be sent to the keeper that dripped on L1, and part of it + * to whoever redeemed the current retryable ticket (tx.origin) * @param _issuanceBase Base value for token issuance (approximation for token supply times L2 rewards fraction) * @param _issuanceRate Rewards issuance rate, using fixed point at 1e18, and including a +1 * @param _nonce Incrementing nonce to ensure messages are received in order + * @param _keeperReward Keeper reward to distribute between keeper that called drip and keeper that redeemed the retryable tx + * @param _l1Keeper Address of the keeper that called drip in L1 */ function receiveDrip( uint256 _issuanceBase, uint256 _issuanceRate, - uint256 _nonce + uint256 _nonce, + uint256 _keeperReward, + address _l1Keeper ) external override onlyL2Gateway { require(_nonce == nextDripNonce, "INVALID_NONCE"); nextDripNonce = nextDripNonce.add(1); @@ -108,6 +114,11 @@ contract L2Reservoir is L2ReservoirV1Storage, Reservoir, IL2Reservoir { snapshotAccumulatedRewards(); } issuanceBase = _issuanceBase; + uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div(TOKEN_DECIMALS); + IGraphToken grt = graphToken(); + // solhint-disable-next-line avoid-tx-origin + grt.transfer(tx.origin, _l2KeeperReward); + grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); emit DripReceived(issuanceBase); } diff --git a/contracts/l2/reservoir/L2ReservoirStorage.sol b/contracts/l2/reservoir/L2ReservoirStorage.sol index ee7880343..4e8c6825d 100644 --- a/contracts/l2/reservoir/L2ReservoirStorage.sol +++ b/contracts/l2/reservoir/L2ReservoirStorage.sol @@ -9,3 +9,8 @@ contract L2ReservoirV1Storage { // Expected nonce value for the next drip hook uint256 public nextDripNonce; } + +contract L2ReservoirV2Storage is L2ReservoirV1Storage { + // Fraction of the keeper reward to send to the retryable tx redeemer in L2 (fixed point 1e18) + uint256 public l2KeeperRewardFraction; +} diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 674e127ef..0e9059afa 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -17,7 +17,7 @@ import "./L1ReservoirStorage.sol"; * It provides a function to periodically drip rewards, and functions to compute accumulated and new * total rewards at a particular block number. */ -contract L1Reservoir is L1ReservoirV1Storage, Reservoir { +contract L1Reservoir is L1ReservoirV2Storage, Reservoir { using SafeMath for uint256; // Emitted when the initial supply snapshot is taken after contract deployment @@ -38,6 +38,10 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { event RewardsDripped(uint256 totalMinted, uint256 sentToL2, uint256 nextDeadline); // Emitted when the address for the L2Reservoir is updated event L2ReservoirAddressUpdated(address l2ReservoirAddress); + // Emitted when drip reward per block is updated + event DripRewardPerBlockUpdated(uint256 dripRewardPerBlock); + // Emitted when minDripInterval is updated + event MinDripIntervalUpdated(uint256 minDripInterval); /** * @dev Initialize this contract. @@ -105,6 +109,26 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { emit L2RewardsFractionStaged(_l2RewardsFraction); } + /** + * @dev Sets the drip reward per block + * This is the reward in GRT provided to the keeper that calls drip() + * @param _dripRewardPerBlock GRT accrued for each block after the threshold + */ + function setDripRewardPerBlock(uint256 _dripRewardPerBlock) external onlyGovernor { + dripRewardPerBlock = _dripRewardPerBlock; + emit DripRewardPerBlockUpdated(_dripRewardPerBlock); + } + + /** + * @dev Sets the minimum drip interval + * This is the minimum number of blocks between two successful drips + * @param _minDripInterval Minimum number of blocks since last drip for drip to be allowed + */ + function setMinDripInterval(uint256 _minDripInterval) external onlyGovernor { + minDripInterval = _minDripInterval; + emit MinDripIntervalUpdated(_minDripInterval); + } + /** * @dev Sets the L2 Reservoir address * This is the address on L2 to which we send tokens for rewards. @@ -153,16 +177,23 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { * @param _l2MaxGas Max gas for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 * @param _l2GasPriceBid Gas price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 * @param _l2MaxSubmissionCost Max submission price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _keeperRewardBeneficiary Address to which to credit keeper reward (will be redeemed in L2 if l2RewardsFraction is nonzero) */ function drip( uint256 _l2MaxGas, uint256 _l2GasPriceBid, uint256 _l2MaxSubmissionCost + address _keeperRewardBeneficiary ) external payable notPaused { + require(block.number > lastRewardsUpdateBlock + minDripInterval, "WAIT_FOR_MIN_INTERVAL"); + uint256 mintedRewardsTotal = getNewGlobalRewards(rewardsMintedUntilBlock); uint256 mintedRewardsActual = getNewGlobalRewards(block.number); // eps = (signed int) mintedRewardsTotal - mintedRewardsActual + uint256 keeperReward = dripRewardPerBlock.mul( + block.number.sub(lastRewardsUpdateBlock).sub(minDripInterval) + ); if (nextIssuanceRate != issuanceRate) { rewardsManager().updateAccRewardsPerSignal(); snapshotAccumulatedRewards(mintedRewardsActual); // This updates lastRewardsUpdateBlock @@ -178,7 +209,7 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { // N = n - eps uint256 tokensToMint; { - uint256 newRewardsPlusMintedActual = newRewardsToDistribute.add(mintedRewardsActual); + uint256 newRewardsPlusMintedActual = newRewardsToDistribute.add(mintedRewardsActual).add(keeperReward); require( newRewardsPlusMintedActual >= mintedRewardsTotal, "Would mint negative tokens, wait before calling again" @@ -186,8 +217,9 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { tokensToMint = newRewardsPlusMintedActual.sub(mintedRewardsTotal); } + IGraphToken grt = graphToken(); if (tokensToMint > 0) { - graphToken().mint(address(this), tokensToMint); + grt.mint(address(this), tokensToMint); } uint256 tokensToSendToL2 = 0; @@ -223,7 +255,9 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { tokensToSendToL2, _l2MaxGas, _l2GasPriceBid, - _l2MaxSubmissionCost + _l2MaxSubmissionCost, + keeperReward, + _keeperRewardBeneficiary ); } else if (l2RewardsFraction > 0) { tokensToSendToL2 = tokensToMint.mul(l2RewardsFraction).div(FIXED_POINT_SCALING_FACTOR); @@ -231,12 +265,16 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { tokensToSendToL2, _l2MaxGas, _l2GasPriceBid, - _l2MaxSubmissionCost + _l2MaxSubmissionCost, + keeperReward, + _keeperRewardBeneficiary ); } else { // Avoid locking funds in this contract if we don't need to // send a message to L2. require(msg.value == 0, "No eth value needed"); + // If we don't send rewards to L2, pay the keeper reward in L1 + grt.transfer(_keeperRewardBeneficiary, keeperReward); } emit RewardsDripped(tokensToMint, tokensToSendToL2, rewardsMintedUntilBlock); } @@ -317,12 +355,16 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { * @param _maxGas Max gas for the L2 retryable ticket execution * @param _gasPriceBid Gas price for the L2 retryable ticket execution * @param _maxSubmissionCost Max submission price for the L2 retryable ticket + * @param _keeperReward Tokens to assign as keeper reward for calling drip + * @param _keeper Address of the keeper that will be rewarded */ function _sendNewTokensAndStateToL2( uint256 _nTokens, uint256 _maxGas, uint256 _gasPriceBid, - uint256 _maxSubmissionCost + uint256 _maxSubmissionCost, + uint256 _keeperReward, + address _keeper ) internal { uint256 l2IssuanceBase = l2RewardsFraction.mul(issuanceBase).div( FIXED_POINT_SCALING_FACTOR @@ -331,7 +373,9 @@ contract L1Reservoir is L1ReservoirV1Storage, Reservoir { IL2Reservoir.receiveDrip.selector, l2IssuanceBase, issuanceRate, - nextDripNonce + nextDripNonce, + _keeperReward, + _keeper ); nextDripNonce = nextDripNonce.add(1); bytes memory data = abi.encode(_maxSubmissionCost, extraData); diff --git a/contracts/reservoir/L1ReservoirStorage.sol b/contracts/reservoir/L1ReservoirStorage.sol index 90821c809..04bb2aa31 100644 --- a/contracts/reservoir/L1ReservoirStorage.sol +++ b/contracts/reservoir/L1ReservoirStorage.sol @@ -21,3 +21,10 @@ contract L1ReservoirV1Storage { // Auto-incrementing nonce that will be used when sending rewards to L2, to ensure ordering uint256 public nextDripNonce; } + +contract L1ReservoirV2Storage is L1ReservoirV1Storage { + // Minimum number of blocks since last drip for a new drip to be allowed + uint256 public minDripInterval; + // Drip reward in GRT for each block since lastRewardsUpdateBlock + dripRewardThreshold + uint256 public dripRewardPerBlock; +} diff --git a/test/l2/l2Reservoir.test.ts b/test/l2/l2Reservoir.test.ts index ebfe3f52e..55c339c60 100644 --- a/test/l2/l2Reservoir.test.ts +++ b/test/l2/l2Reservoir.test.ts @@ -160,7 +160,13 @@ describe('L2Reservoir', () => { it('rejects the call when not called by the gateway', async function () { const tx = l2Reservoir .connect(governor.signer) - .receiveDrip(dripNormalizedSupply, dripIssuanceRate, toBN('0')) + .receiveDrip( + dripNormalizedSupply, + dripIssuanceRate, + toBN('0'), + toBN('0'), + testAccount1.address, + ) await expect(tx).revertedWith('ONLY_GATEWAY') }) it('rejects the call when received out of order', async function () { @@ -169,6 +175,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply, dripIssuanceRate, toBN('0'), + toBN('0'), + testAccount1.address, ) const tx = await validGatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -181,6 +189,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply.add(1), dripIssuanceRate.add(1), toBN('2'), + toBN('0'), + testAccount1.address, ) const tx2 = gatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -192,6 +202,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply, dripIssuanceRate, toBN('0'), + toBN('0'), + testAccount1.address, ) const tx = await validGatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -205,6 +217,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply, dripIssuanceRate, toBN('0'), + toBN('0'), + testAccount1.address, ) let tx = await validGatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -216,6 +230,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply.add(1), dripIssuanceRate.add(1), toBN('1'), + toBN('0'), + testAccount1.address, ) tx = await gatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -230,6 +246,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply, dripIssuanceRate, toBN('0'), + toBN('0'), + testAccount1.address, ) let tx = await validGatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -241,6 +259,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply.add(1), dripIssuanceRate, toBN('1'), + toBN('0'), + testAccount1.address, ) tx = await gatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -255,6 +275,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply, dripIssuanceRate, toBN('0'), + toBN('0'), + testAccount1.address, ) let tx = await validGatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -267,6 +289,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply.add(1), dripIssuanceRate, toBN('2'), + toBN('0'), + testAccount1.address, ) tx = await gatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() @@ -285,6 +309,8 @@ describe('L2Reservoir', () => { dripNormalizedSupply, ISSUANCE_RATE_PER_BLOCK, toBN('0'), + toBN('0'), + testAccount1.address, ) await validGatewayFinalizeTransfer(receiveDripTx.data) dripBlock = await latestBlock() diff --git a/test/reservoir/l1Reservoir.test.ts b/test/reservoir/l1Reservoir.test.ts index afc6233ca..86801149b 100644 --- a/test/reservoir/l1Reservoir.test.ts +++ b/test/reservoir/l1Reservoir.test.ts @@ -35,7 +35,7 @@ const l2ReservoirAbi = artifacts.readArtifactSync('L2Reservoir').abi const l2ReservoirIface = new Interface(l2ReservoirAbi) const { AddressZero } = constants -const toRound = (n: BigNumber) => formatGRT(n).split('.')[0] +const toRound = (n: BigNumber) => formatGRT(n.add(toGRT('0.5'))).split('.')[0] const maxGas = toBN('1000000') const maxSubmissionCost = toBN('7') @@ -49,6 +49,7 @@ describe('L1Reservoir', () => { let mockL2GRT: Account let mockL2Gateway: Account let mockL2Reservoir: Account + let keeper: Account let fixture: NetworkFixture let grt: GraphToken @@ -120,7 +121,9 @@ describe('L1Reservoir', () => { expect(await tracker.accRewards(dripBlock)).to.eq(0) let expectedNextDeadline = dripBlock.add(dripInterval) let expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) - const tx1 = await l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) + const tx1 = await l1Reservoir + .connect(keeper.signer) + .drip(toBN(0), toBN(0), toBN(0), keeper.address) const actualAmount = await grt.balanceOf(l1Reservoir.address) expect(await latestBlock()).eq(dripBlock) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount)) @@ -133,7 +136,9 @@ describe('L1Reservoir', () => { await advanceBlocks(blocksToAdvance) - const tx2 = await l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) + const tx2 = await l1Reservoir + .connect(keeper.signer) + .drip(toBN(0), toBN(0), toBN(0), keeper.address) const newAmount = (await grt.balanceOf(l1Reservoir.address)).sub(actualAmount) expectedNextDeadline = (await latestBlock()).add(dripInterval) const expectedSnapshottedSupply = supplyBeforeDrip.add(await tracker.accRewards()) @@ -147,7 +152,7 @@ describe('L1Reservoir', () => { } before(async function () { - ;[governor, testAccount1, mockRouter, mockL2GRT, mockL2Gateway, mockL2Reservoir] = + ;[governor, testAccount1, mockRouter, mockL2GRT, mockL2Gateway, mockL2Reservoir, keeper] = await getAccounts() fixture = new NetworkFixture() @@ -248,7 +253,7 @@ describe('L1Reservoir', () => { await expect(tx).emit(l1Reservoir, 'IssuanceRateStaged').withArgs(newIssuanceRate) expect(await l1Reservoir.issuanceRate()).eq(0) expect(await l1Reservoir.nextIssuanceRate()).eq(newIssuanceRate) - tx = l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) + tx = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) await expect(tx).emit(l1Reservoir, 'IssuanceRateUpdated').withArgs(newIssuanceRate) expect(await l1Reservoir.issuanceRate()).eq(newIssuanceRate) }) @@ -313,12 +318,25 @@ describe('L1Reservoir', () => { await expect(tx).emit(l1Reservoir, 'L2RewardsFractionStaged').withArgs(newValue) expect(await l1Reservoir.nextL2RewardsFraction()).eq(newValue) tx = l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) await expect(tx).emit(l1Reservoir, 'L2RewardsFractionUpdated').withArgs(newValue) expect(await l1Reservoir.l2RewardsFraction()).eq(newValue) }) }) + describe('minimum drip interval update', function () { + it('rejects setting minimum drip interval if unauthorized', async function () { + const tx = l1Reservoir.connect(testAccount1.signer).setMinDripInterval(toBN('200')) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + + it('sets the minimum drip interval', async function () { + const newValue = toBN('200') + const tx = l1Reservoir.connect(governor.signer).setMinDripInterval(newValue) + await expect(tx).emit(l1Reservoir, 'MinDripIntervalUpdated').withArgs(newValue) + expect(await l1Reservoir.minDripInterval()).eq(newValue) + }) + }) }) // TODO test that rewardsManager.updateAccRewardsPerSignal is called when @@ -337,7 +355,9 @@ describe('L1Reservoir', () => { expect(await tracker.accRewards(dripBlock)).to.eq(0) const expectedNextDeadline = dripBlock.add(defaults.rewards.dripInterval) const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) - const tx = await l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) + const tx = await l1Reservoir + .connect(keeper.signer) + .drip(toBN(0), toBN(0), toBN(0), keeper.address) const actualAmount = await grt.balanceOf(l1Reservoir.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount)) expect(await l1Reservoir.issuanceBase()).to.eq(supplyBeforeDrip) @@ -345,7 +365,7 @@ describe('L1Reservoir', () => { .emit(l1Reservoir, 'RewardsDripped') .withArgs(actualAmount, toBN(0), expectedNextDeadline) }) - it('has no effect if called a second time in the same block', async function () { + it('cannot be called more than once per minDripInterval', async function () { supplyBeforeDrip = await grt.totalSupply() const startAccrued = await l1Reservoir.getAccumulatedRewards(await latestBlock()) expect(startAccrued).to.eq(0) @@ -358,14 +378,16 @@ describe('L1Reservoir', () => { expect(await tracker.accRewards(dripBlock)).to.eq(0) const expectedNextDeadline = dripBlock.add(defaults.rewards.dripInterval) const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) - await provider().send('evm_setAutomine', [false]) - const tx1 = await l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) - const tx2 = await l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) - await provider().send('evm_mine', []) - await provider().send('evm_setAutomine', [true]) + + const tx1 = await l1Reservoir + .connect(keeper.signer) + .drip(toBN(0), toBN(0), toBN(0), keeper.address) + + const minInterval = toBN('200') + await l1Reservoir.connect(governor.signer).setMinDripInterval(minInterval) const actualAmount = await grt.balanceOf(l1Reservoir.address) - expect(await latestBlock()).eq(dripBlock) // Just in case disabling automine stops working + expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount)) await expect(tx1) .emit(l1Reservoir, 'RewardsDripped') @@ -373,15 +395,24 @@ describe('L1Reservoir', () => { await expect(tx1) .emit(grt, 'Transfer') .withArgs(AddressZero, l1Reservoir.address, actualAmount) - await expect(tx2) - .emit(l1Reservoir, 'RewardsDripped') - .withArgs(toBN(0), toBN(0), expectedNextDeadline) - await expect(tx2).not.emit(grt, 'Transfer') + + const tx2 = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + await expect(tx2).revertedWith('WAIT_FOR_MIN_INTERVAL') + + // We've had 1 block since the last drip so far, so we jump to one block before the interval is done + await advanceBlocks(minInterval.sub(2)) + const tx3 = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + await expect(tx3).revertedWith('WAIT_FOR_MIN_INTERVAL') + + await advanceBlocks(1) + // Now we're over the interval so we can drip again + const tx4 = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + await expect(tx4).emit(l1Reservoir, 'RewardsDripped') }) it('prevents locking eth in the contract if l2RewardsFraction is 0', async function () { const tx = l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) await expect(tx).revertedWith('No eth value needed') }) it('mints only a few more tokens if called on the next block', async function () { @@ -413,8 +444,8 @@ describe('L1Reservoir', () => { const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) const expectedSentToL2 = expectedMintedAmount.div(2) const tx = await l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) @@ -434,6 +465,69 @@ describe('L1Reservoir', () => { l2IssuanceBase, issuanceRate, toBN('0'), + toBN('0'), + keeper.address, + ]) + const expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( + grt.address, + l1Reservoir.address, + mockL2Reservoir.address, + escrowedAmount, + expectedCallhookData, + ) + await expect(tx) + .emit(l1GraphTokenGateway, 'TxToL2') + .withArgs(l1Reservoir.address, mockL2Gateway.address, toBN(1), expectedL2Data) + }) + it('sends the specified fraction of the rewards with a keeper reward to L2', async function () { + await l1Reservoir.connect(governor.signer).setL2RewardsFraction(toGRT('0.5')) + await l1Reservoir.connect(governor.signer).setDripRewardPerBlock(toGRT('3')) + await l1Reservoir.connect(governor.signer).setMinDripInterval(toBN('2')) + // lastRewardsUpdateBlock is set to block.number with initialSnapshot + await l1Reservoir.connect(governor.signer).initialSnapshot(toBN(0)) + await advanceBlocks(toBN('4')) + + // now we're at lastRewardsUpdateBlock + minDripInterval + 3, so keeper reward should be: + // dripRewardPerBlock * 3 + supplyBeforeDrip = await grt.totalSupply() + const startAccrued = await l1Reservoir.getAccumulatedRewards(await latestBlock()) + expect(startAccrued).to.eq(0) + const dripBlock = (await latestBlock()).add(1) // We're gonna drip in the next transaction + const tracker = await RewardsTracker.create( + supplyBeforeDrip, + defaults.rewards.issuanceRate, + dripBlock, + ) + expect(await tracker.accRewards(dripBlock)).to.eq(0) + const expectedNextDeadline = dripBlock.add(defaults.rewards.dripInterval) + const expectedMintedRewards = await tracker.accRewards(expectedNextDeadline) + const expectedMintedAmount = expectedMintedRewards.add(toGRT('9')) + const expectedSentToL2 = expectedMintedRewards.div(2) + const tx = await l1Reservoir + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + const actualAmount = await grt.balanceOf(l1Reservoir.address) + const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) + + expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) + expect(toRound((await grt.totalSupply()).sub(supplyBeforeDrip))).to.eq( + toRound(expectedMintedAmount), + ) + expect(toRound(escrowedAmount)).to.eq(toRound(expectedSentToL2)) + await expect(tx) + .emit(l1Reservoir, 'RewardsDripped') + .withArgs(actualAmount.add(escrowedAmount), escrowedAmount, expectedNextDeadline) + + const normalizedTokenSupply = (await l1Reservoir.tokenSupplyCache()) + .mul(await l1Reservoir.l2RewardsFraction()) + .div(toGRT('1')) + const issuanceRate = await l1Reservoir.issuanceRate() + const expectedCallhookData = l2ReservoirIface.encodeFunctionData('receiveDrip', [ + normalizedTokenSupply, + issuanceRate, + toBN('0'), + toGRT('9'), // keeper reward + keeper.address, ]) const expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( grt.address, @@ -462,8 +556,8 @@ describe('L1Reservoir', () => { const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) const expectedSentToL2 = expectedMintedAmount.div(2) const tx = await l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) @@ -483,6 +577,8 @@ describe('L1Reservoir', () => { l2IssuanceBase, issuanceRate, toBN('0'), + toBN('0'), + keeper.address, ]) let expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( grt.address, @@ -511,8 +607,8 @@ describe('L1Reservoir', () => { .add(expectedTotalRewards.sub(rewardsUntilSecondDripBlock).mul(8).div(10)) const tx2 = await l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) const newActualAmount = await grt.balanceOf(l1Reservoir.address) const newEscrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(newActualAmount)).to.eq( @@ -529,6 +625,8 @@ describe('L1Reservoir', () => { l2IssuanceBase, issuanceRate, toBN('1'), // Incremented nonce + toBN('0'), + keeper.address, ]) expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( grt.address, @@ -564,8 +662,8 @@ describe('L1Reservoir', () => { const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) const expectedSentToL2 = expectedMintedAmount.div(2) const tx = await l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) @@ -585,6 +683,8 @@ describe('L1Reservoir', () => { l2IssuanceBase, issuanceRate, toBN('0'), + toBN('0'), + keeper.address, ]) let expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( grt.address, @@ -610,8 +710,8 @@ describe('L1Reservoir', () => { const expectedNewTotalSentToL2 = expectedTotalRewards.div(2) const tx2 = await l1Reservoir - .connect(governor.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) const newActualAmount = await grt.balanceOf(l1Reservoir.address) const newEscrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(newActualAmount)).to.eq( @@ -628,6 +728,8 @@ describe('L1Reservoir', () => { l2IssuanceBase, issuanceRate, toBN('1'), // Incremented nonce + toBN('0'), + keeper.address, ]) expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( grt.address, @@ -654,7 +756,7 @@ describe('L1Reservoir', () => { // 5% minute rate (4 blocks) await l1Reservoir.connect(governor.signer).setIssuanceRate(ISSUANCE_RATE_PER_BLOCK) supplyBeforeDrip = await grt.totalSupply() - await l1Reservoir.drip(toBN(0), toBN(0), toBN(0)) + await l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) dripBlock = await latestBlock() }) @@ -724,7 +826,9 @@ describe('L1Reservoir', () => { it('computes the rewards delta considering the L2 rewards fraction', async function () { const lambda = toGRT('0.32') await l1Reservoir.connect(governor.signer).setL2RewardsFraction(lambda) - await l1Reservoir.drip(maxGas, gasPriceBid, maxSubmissionCost, { value: defaultEthValue }) + await l1Reservoir + .connect(keeper.signer) + .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) supplyBeforeDrip = await l1Reservoir.issuanceBase() // Has been updated accordingly dripBlock = await latestBlock() await advanceBlocks(20) diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index b95b0464e..f5e18b2f9 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -42,6 +42,7 @@ describe('Rewards', () => { let indexer1: Account let indexer2: Account let oracle: Account + let keeper: Account let fixture: NetworkFixture @@ -107,7 +108,8 @@ describe('Rewards', () => { } before(async function () { - ;[delegator, governor, curator1, curator2, indexer1, indexer2, oracle] = await getAccounts() + ;[delegator, governor, curator1, curator2, indexer1, indexer2, oracle, keeper] = + await getAccounts() fixture = new NetworkFixture() ;({ grt, curation, epochManager, staking, rewardsManager, l1Reservoir } = await fixture.load( @@ -276,7 +278,7 @@ describe('Rewards', () => { beforeEach(async function () { // 5% minute rate (4 blocks) await l1Reservoir.connect(governor.signer).setIssuanceRate(ISSUANCE_RATE_PER_BLOCK) - await l1Reservoir.connect(governor.signer).drip(toBN(0), toBN(0), toBN(0)) + await l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) dripBlock = await latestBlock() }) From 56de47a91784745d74e527e79791d9c86c861146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Tue, 7 Jun 2022 21:09:35 -0700 Subject: [PATCH 02/21] fix: don't use tx.origin as it will not work --- contracts/l2/reservoir/L2Reservoir.sol | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 3b1dd131d..96a344f67 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -114,11 +114,15 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { snapshotAccumulatedRewards(); } issuanceBase = _issuanceBase; - uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div(TOKEN_DECIMALS); IGraphToken grt = graphToken(); - // solhint-disable-next-line avoid-tx-origin - grt.transfer(tx.origin, _l2KeeperReward); - grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); + // We'd like to reward the keeper that redeemed the tx in L2 + // but this won't work right now as tx.origin will actually be the L1 sender. + // uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div(TOKEN_DECIMALS); + // // solhint-disable-next-line avoid-tx-origin + // grt.transfer(tx.origin, _l2KeeperReward); + // grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); + // So for now we just send all the rewards to teh L1 keeper: + grt.transfer(_l1Keeper, _keeperReward); emit DripReceived(issuanceBase); } From a33bd303d7648a00f67045a01165c05e06f16b76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Tue, 21 Jun 2022 15:37:33 +0300 Subject: [PATCH 03/21] fix: only allow indexers, their operators, or whitelisted addresses to call drip (needs tests) --- contracts/reservoir/L1Reservoir.sol | 122 ++++++++++++++++++++- contracts/reservoir/L1ReservoirStorage.sol | 2 + 2 files changed, 121 insertions(+), 3 deletions(-) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 0e9059afa..566181f43 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -42,6 +42,29 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { event DripRewardPerBlockUpdated(uint256 dripRewardPerBlock); // Emitted when minDripInterval is updated event MinDripIntervalUpdated(uint256 minDripInterval); + // Emitted when a new allowedDripper is added + event AllowedDripperAdded(address dripper); + // Emitted when an allowedDripper is revoked + event AllowedDripperRevoked(address dripper); + + /** + * @dev Checks that the sender is an indexer with stake on the Staking contract, + * or that the sender is an address whitelisted by governance to call. + */ + modifier onlyIndexerOrAllowedDripper() { + require(allowedDrippers[msg.sender] || _isIndexer(msg.sender), "UNAUTHORIZED"); + _; + } + + /** + * @dev Checks that the sender is an operator for the specified indexer + * (also checks that the specified indexer is, indeed, an indexer). + * @param _indexer Indexer for which the sender must be an operator + */ + modifier onlyIndexerOperator(address _indexer) { + require(_isIndexer(_indexer) && staking().isOperator(msg.sender, _indexer), "UNAUTHORIZED"); + _; + } /** * @dev Initialize this contract. @@ -140,6 +163,24 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { emit L2ReservoirAddressUpdated(_l2ReservoirAddress); } + /** + * @dev Grants an address permission to call drip() + * @param _dripper Address that will be an allowed dripper + */ + function grantDripPermission(address _dripper) external onlyGovernor { + allowedDrippers[_dripper] = true; + emit AllowedDripperAdded(_dripper); + } + + /** + * @dev Revokes an address' permission to call drip() + * @param _dripper Address that will not be an allowed dripper anymore + */ + function revokeDripPermission(address _dripper) external onlyGovernor { + allowedDrippers[_dripper] = false; + emit AllowedDripperRevoked(_dripper); + } + /** * @dev Computes the initial snapshot for token supply and mints any pending rewards * This will initialize the issuanceBase to the current GRT supply, after which @@ -164,7 +205,7 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * tokens and a callhook to the L2Reservoir, through the GRT Arbitrum bridge. * Any staged changes to issuanceRate or l2RewardsFraction will be applied when this function * is called. If issuanceRate changes, it also triggers a snapshot of rewards per signal on the RewardsManager. - * The call value must be equal to l2MaxSubmissionCost + (l2MaxGas * l2GasPriceBid), and must + * The call value must be greater than or equal to l2MaxSubmissionCost + (l2MaxGas * l2GasPriceBid), and must * only be nonzero if l2RewardsFraction is nonzero. * Calling this function can revert if the issuance rate has recently been reduced, and the existing * tokens are sufficient to cover the full pending period. In this case, it's necessary to wait @@ -174,17 +215,83 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * Note that the transaction on the L2 side might revert if it's received out-of-order by the L2Reservoir, * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed * again once the ticket for previous drip has been redeemed. + * This function with an additional parameter is only provided so that indexer operators can call it, + * specifying the indexer for which they are an operator. * @param _l2MaxGas Max gas for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 * @param _l2GasPriceBid Gas price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 * @param _l2MaxSubmissionCost Max submission price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 * @param _keeperRewardBeneficiary Address to which to credit keeper reward (will be redeemed in L2 if l2RewardsFraction is nonzero) + * @param _indexer Indexer for whom the sender must be an authorized Operator */ function drip( uint256 _l2MaxGas, uint256 _l2GasPriceBid, - uint256 _l2MaxSubmissionCost + uint256 _l2MaxSubmissionCost, + address _keeperRewardBeneficiary, + address _indexer + ) external payable notPaused onlyIndexerOperator(_indexer) { + _drip(_l2MaxGas, _l2GasPriceBid, _l2MaxSubmissionCost, _keeperRewardBeneficiary); + } + + /** + * @dev Drip indexer rewards for layers 1 and 2 + * This function will mint enough tokens to cover all indexer rewards for the next + * dripInterval number of blocks. If the l2RewardsFraction is > 0, it will also send + * tokens and a callhook to the L2Reservoir, through the GRT Arbitrum bridge. + * Any staged changes to issuanceRate or l2RewardsFraction will be applied when this function + * is called. If issuanceRate changes, it also triggers a snapshot of rewards per signal on the RewardsManager. + * The call value must be greater than or equal to l2MaxSubmissionCost + (l2MaxGas * l2GasPriceBid), and must + * only be nonzero if l2RewardsFraction is nonzero. + * Calling this function can revert if the issuance rate has recently been reduced, and the existing + * tokens are sufficient to cover the full pending period. In this case, it's necessary to wait + * until the drip amount becomes positive before calling the function again. It can also revert + * if the l2RewardsFraction has been updated and the amount already sent to L2 is more than what we + * should send now. + * Note that the transaction on the L2 side might revert if it's received out-of-order by the L2Reservoir, + * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed + * again once the ticket for previous drip has been redeemed. + * @param _l2MaxGas Max gas for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _l2GasPriceBid Gas price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _l2MaxSubmissionCost Max submission price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _keeperRewardBeneficiary Address to which to credit keeper reward (will be redeemed in L2 if l2RewardsFraction is nonzero) + */ + function drip( + uint256 _l2MaxGas, + uint256 _l2GasPriceBid, + uint256 _l2MaxSubmissionCost, address _keeperRewardBeneficiary - ) external payable notPaused { + ) external payable notPaused onlyIndexerOrAllowedDripper { + _drip(_l2MaxGas, _l2GasPriceBid, _l2MaxSubmissionCost, _keeperRewardBeneficiary); + } + + /** + * @dev Drip indexer rewards for layers 1 and 2, private implementation. + * This function will mint enough tokens to cover all indexer rewards for the next + * dripInterval number of blocks. If the l2RewardsFraction is > 0, it will also send + * tokens and a callhook to the L2Reservoir, through the GRT Arbitrum bridge. + * Any staged changes to issuanceRate or l2RewardsFraction will be applied when this function + * is called. If issuanceRate changes, it also triggers a snapshot of rewards per signal on the RewardsManager. + * The call value must be greater than or equal to l2MaxSubmissionCost + (l2MaxGas * l2GasPriceBid), and must + * only be nonzero if l2RewardsFraction is nonzero. + * Calling this function can revert if the issuance rate has recently been reduced, and the existing + * tokens are sufficient to cover the full pending period. In this case, it's necessary to wait + * until the drip amount becomes positive before calling the function again. It can also revert + * if the l2RewardsFraction has been updated and the amount already sent to L2 is more than what we + * should send now. + * Note that the transaction on the L2 side might revert if it's received out-of-order by the L2Reservoir, + * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed + * again once the ticket for previous drip has been redeemed. + * @param _l2MaxGas Max gas for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _l2GasPriceBid Gas price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _l2MaxSubmissionCost Max submission price for the L2 retryable ticket, only needed if l2RewardsFraction is > 0 + * @param _keeperRewardBeneficiary Address to which to credit keeper reward (will be redeemed in L2 if l2RewardsFraction is nonzero) + */ + function _drip( + uint256 _l2MaxGas, + uint256 _l2GasPriceBid, + uint256 _l2MaxSubmissionCost, + address _keeperRewardBeneficiary + ) private { require(block.number > lastRewardsUpdateBlock + minDripInterval, "WAIT_FOR_MIN_INTERVAL"); uint256 mintedRewardsTotal = getNewGlobalRewards(rewardsMintedUntilBlock); @@ -391,4 +498,13 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { data ); } + + /** + * @dev Checks if an address is an indexer with stake in the Staking contract + * @param _indexer Address that will be checked + */ + function _isIndexer(address _indexer) internal view returns (bool) { + IStaking staking = staking(); + return staking.hasStake(_indexer); + } } diff --git a/contracts/reservoir/L1ReservoirStorage.sol b/contracts/reservoir/L1ReservoirStorage.sol index 04bb2aa31..92b9d5107 100644 --- a/contracts/reservoir/L1ReservoirStorage.sol +++ b/contracts/reservoir/L1ReservoirStorage.sol @@ -27,4 +27,6 @@ contract L1ReservoirV2Storage is L1ReservoirV1Storage { uint256 public minDripInterval; // Drip reward in GRT for each block since lastRewardsUpdateBlock + dripRewardThreshold uint256 public dripRewardPerBlock; + // True for addresses that are allowed to call drip() + mapping(address => bool) public allowedDrippers; } From 435abc2ae8027d10e9584bd5bafc27877a1dddcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Tue, 21 Jun 2022 17:21:47 +0300 Subject: [PATCH 04/21] test: fix rewards and reservoir tests after restricting drip callers --- contracts/reservoir/L1Reservoir.sol | 13 +- test/reservoir/l1Reservoir.test.ts | 208 +++++++++++++++++++++++----- test/rewards/rewards.test.ts | 20 ++- 3 files changed, 195 insertions(+), 46 deletions(-) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 566181f43..30fa4d82a 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -316,7 +316,9 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { // N = n - eps uint256 tokensToMint; { - uint256 newRewardsPlusMintedActual = newRewardsToDistribute.add(mintedRewardsActual).add(keeperReward); + uint256 newRewardsPlusMintedActual = newRewardsToDistribute + .add(mintedRewardsActual) + .add(keeperReward); require( newRewardsPlusMintedActual >= mintedRewardsTotal, "Would mint negative tokens, wait before calling again" @@ -348,9 +350,9 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { tokensToSendToL2 > l2OffsetAmount, "Negative amount would be sent to L2, wait before calling again" ); - tokensToSendToL2 = tokensToSendToL2.sub(l2OffsetAmount); + tokensToSendToL2 = tokensToSendToL2.add(keeperReward).sub(l2OffsetAmount); } else { - tokensToSendToL2 = tokensToSendToL2.add( + tokensToSendToL2 = tokensToSendToL2.add(keeperReward).add( l2RewardsFraction.mul(mintedRewardsActual.sub(mintedRewardsTotal)).div( FIXED_POINT_SCALING_FACTOR ) @@ -367,7 +369,10 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { _keeperRewardBeneficiary ); } else if (l2RewardsFraction > 0) { - tokensToSendToL2 = tokensToMint.mul(l2RewardsFraction).div(FIXED_POINT_SCALING_FACTOR); + tokensToSendToL2 = tokensToMint + .mul(l2RewardsFraction) + .div(FIXED_POINT_SCALING_FACTOR) + .add(keeperReward); _sendNewTokensAndStateToL2( tokensToSendToL2, _l2MaxGas, diff --git a/test/reservoir/l1Reservoir.test.ts b/test/reservoir/l1Reservoir.test.ts index 86801149b..9c272c847 100644 --- a/test/reservoir/l1Reservoir.test.ts +++ b/test/reservoir/l1Reservoir.test.ts @@ -1,5 +1,5 @@ import { expect } from 'chai' -import { BigNumber, constants, utils } from 'ethers' +import { BigNumber, constants } from 'ethers' import { defaults, deployContract, deployL1Reservoir } from '../lib/deployment' import { ArbitrumL1Mocks, L1FixtureContracts, NetworkFixture } from '../lib/fixtures' @@ -17,7 +17,6 @@ import { formatGRT, Account, RewardsTracker, - provider, } from '../lib/testHelpers' import { L1Reservoir } from '../../build/types/L1Reservoir' import { BridgeEscrow } from '../../build/types/BridgeEscrow' @@ -26,9 +25,9 @@ import path from 'path' import { Artifacts } from 'hardhat/internal/artifacts' import { Interface } from 'ethers/lib/utils' import { L1GraphTokenGateway } from '../../build/types/L1GraphTokenGateway' -import { SubgraphDeploymentID } from '@graphprotocol/common-ts' import { Controller } from '../../build/types/Controller' import { GraphProxyAdmin } from '../../build/types/GraphProxyAdmin' +import { Staking } from '../../build/types/Staking' const ARTIFACTS_PATH = path.resolve('build/contracts') const artifacts = new Artifacts(ARTIFACTS_PATH) const l2ReservoirAbi = artifacts.readArtifactSync('L2Reservoir').abi @@ -45,6 +44,7 @@ const defaultEthValue = maxSubmissionCost.add(maxGas.mul(gasPriceBid)) describe('L1Reservoir', () => { let governor: Account let testAccount1: Account + let testAccount2: Account let mockRouter: Account let mockL2GRT: Account let mockL2Gateway: Account @@ -59,6 +59,7 @@ describe('L1Reservoir', () => { let l1GraphTokenGateway: L1GraphTokenGateway let controller: Controller let proxyAdmin: GraphProxyAdmin + let staking: Staking let supplyBeforeDrip: BigNumber let dripBlock: BigNumber @@ -123,7 +124,7 @@ describe('L1Reservoir', () => { let expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) const tx1 = await l1Reservoir .connect(keeper.signer) - .drip(toBN(0), toBN(0), toBN(0), keeper.address) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) const actualAmount = await grt.balanceOf(l1Reservoir.address) expect(await latestBlock()).eq(dripBlock) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount)) @@ -138,7 +139,7 @@ describe('L1Reservoir', () => { const tx2 = await l1Reservoir .connect(keeper.signer) - .drip(toBN(0), toBN(0), toBN(0), keeper.address) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) const newAmount = (await grt.balanceOf(l1Reservoir.address)).sub(actualAmount) expectedNextDeadline = (await latestBlock()).add(dripInterval) const expectedSnapshottedSupply = supplyBeforeDrip.add(await tracker.accRewards()) @@ -152,12 +153,20 @@ describe('L1Reservoir', () => { } before(async function () { - ;[governor, testAccount1, mockRouter, mockL2GRT, mockL2Gateway, mockL2Reservoir, keeper] = - await getAccounts() + ;[ + governor, + testAccount1, + mockRouter, + mockL2GRT, + mockL2Gateway, + mockL2Reservoir, + keeper, + testAccount2, + ] = await getAccounts() fixture = new NetworkFixture() fixtureContracts = await fixture.load(governor.signer) - ;({ grt, l1Reservoir, bridgeEscrow, l1GraphTokenGateway, controller, proxyAdmin } = + ;({ grt, l1Reservoir, bridgeEscrow, l1GraphTokenGateway, controller, proxyAdmin, staking } = fixtureContracts) await l1Reservoir.connect(governor.signer).initialSnapshot(toBN(0)) @@ -171,6 +180,7 @@ describe('L1Reservoir', () => { mockL2Gateway.address, mockL2Reservoir.address, ) + await l1Reservoir.connect(governor.signer).grantDripPermission(keeper.address) reservoirMock = (await deployContract( 'ReservoirMock', governor.signer, @@ -253,7 +263,9 @@ describe('L1Reservoir', () => { await expect(tx).emit(l1Reservoir, 'IssuanceRateStaged').withArgs(newIssuanceRate) expect(await l1Reservoir.issuanceRate()).eq(0) expect(await l1Reservoir.nextIssuanceRate()).eq(newIssuanceRate) - tx = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + tx = l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) await expect(tx).emit(l1Reservoir, 'IssuanceRateUpdated').withArgs(newIssuanceRate) expect(await l1Reservoir.issuanceRate()).eq(newIssuanceRate) }) @@ -319,7 +331,13 @@ describe('L1Reservoir', () => { expect(await l1Reservoir.nextL2RewardsFraction()).eq(newValue) tx = l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) await expect(tx).emit(l1Reservoir, 'L2RewardsFractionUpdated').withArgs(newValue) expect(await l1Reservoir.l2RewardsFraction()).eq(newValue) }) @@ -337,11 +355,74 @@ describe('L1Reservoir', () => { expect(await l1Reservoir.minDripInterval()).eq(newValue) }) }) + describe('allowed drippers whitelist', function () { + it('only allows the governor to add a dripper', async function () { + const tx = l1Reservoir + .connect(testAccount1.signer) + .grantDripPermission(testAccount1.address) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + it('only allows the governor to revoke a dripper', async function () { + const tx = l1Reservoir.connect(testAccount1.signer).revokeDripPermission(keeper.address) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + it('allows adding an address to the allowed drippers', async function () { + const tx = l1Reservoir.connect(governor.signer).grantDripPermission(testAccount1.address) + await expect(tx).emit(l1Reservoir, 'AllowedDripperAdded').withArgs(testAccount1.address) + expect(await l1Reservoir.allowedDrippers(testAccount1.address)).eq(true) + }) + it('allows removing an address from the allowed drippers', async function () { + await l1Reservoir.connect(governor.signer).grantDripPermission(testAccount1.address) + const tx = l1Reservoir.connect(governor.signer).revokeDripPermission(testAccount1.address) + await expect(tx).emit(l1Reservoir, 'AllowedDripperRevoked').withArgs(testAccount1.address) + expect(await l1Reservoir.allowedDrippers(testAccount1.address)).eq(false) + }) + }) }) // TODO test that rewardsManager.updateAccRewardsPerSignal is called when // issuanceRate or l2RewardsFraction is updated describe('drip', function () { + it('cannot be called by an unauthorized address', async function () { + const tx = l1Reservoir + .connect(testAccount1.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), testAccount1.address) + await expect(tx).revertedWith('UNAUTHORIZED') + }) + it('can be called by an indexer', async function () { + const stakedAmount = toGRT('100000') + await grt.connect(governor.signer).mint(testAccount1.address, stakedAmount) + await grt.connect(testAccount1.signer).approve(staking.address, stakedAmount) + await staking.connect(testAccount1.signer).stake(stakedAmount) + const tx = l1Reservoir + .connect(testAccount1.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), testAccount1.address) + await expect(tx).emit(l1Reservoir, 'RewardsDripped') + }) + it('can be called by a whitelisted address', async function () { + await l1Reservoir.connect(governor.signer).grantDripPermission(testAccount1.address) + const tx = l1Reservoir + .connect(testAccount1.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), testAccount1.address) + await expect(tx).emit(l1Reservoir, 'RewardsDripped') + }) + it('can be called by an indexer operator using an extra parameter', async function () { + const stakedAmount = toGRT('100000') + await grt.connect(governor.signer).mint(testAccount1.address, stakedAmount) + await grt.connect(testAccount1.signer).approve(staking.address, stakedAmount) + await staking.connect(testAccount1.signer).stake(stakedAmount) + await staking.connect(testAccount1.signer).setOperator(testAccount2.address, true) + const tx = l1Reservoir + .connect(testAccount2.signer) + ['drip(uint256,uint256,uint256,address,address)']( + toBN(0), + toBN(0), + toBN(0), + testAccount1.address, + testAccount1.address, + ) + await expect(tx).emit(l1Reservoir, 'RewardsDripped') + }) it('mints rewards for the next week', async function () { supplyBeforeDrip = await grt.totalSupply() const startAccrued = await l1Reservoir.getAccumulatedRewards(await latestBlock()) @@ -357,7 +438,7 @@ describe('L1Reservoir', () => { const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) const tx = await l1Reservoir .connect(keeper.signer) - .drip(toBN(0), toBN(0), toBN(0), keeper.address) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) const actualAmount = await grt.balanceOf(l1Reservoir.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount)) expect(await l1Reservoir.issuanceBase()).to.eq(supplyBeforeDrip) @@ -381,7 +462,7 @@ describe('L1Reservoir', () => { const tx1 = await l1Reservoir .connect(keeper.signer) - .drip(toBN(0), toBN(0), toBN(0), keeper.address) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) const minInterval = toBN('200') await l1Reservoir.connect(governor.signer).setMinDripInterval(minInterval) @@ -396,23 +477,35 @@ describe('L1Reservoir', () => { .emit(grt, 'Transfer') .withArgs(AddressZero, l1Reservoir.address, actualAmount) - const tx2 = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + const tx2 = l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) await expect(tx2).revertedWith('WAIT_FOR_MIN_INTERVAL') // We've had 1 block since the last drip so far, so we jump to one block before the interval is done await advanceBlocks(minInterval.sub(2)) - const tx3 = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + const tx3 = l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) await expect(tx3).revertedWith('WAIT_FOR_MIN_INTERVAL') await advanceBlocks(1) // Now we're over the interval so we can drip again - const tx4 = l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + const tx4 = l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) await expect(tx4).emit(l1Reservoir, 'RewardsDripped') }) it('prevents locking eth in the contract if l2RewardsFraction is 0', async function () { const tx = l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) await expect(tx).revertedWith('No eth value needed') }) it('mints only a few more tokens if called on the next block', async function () { @@ -445,7 +538,13 @@ describe('L1Reservoir', () => { const expectedSentToL2 = expectedMintedAmount.div(2) const tx = await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) @@ -483,29 +582,37 @@ describe('L1Reservoir', () => { await l1Reservoir.connect(governor.signer).setL2RewardsFraction(toGRT('0.5')) await l1Reservoir.connect(governor.signer).setDripRewardPerBlock(toGRT('3')) await l1Reservoir.connect(governor.signer).setMinDripInterval(toBN('2')) - // lastRewardsUpdateBlock is set to block.number with initialSnapshot - await l1Reservoir.connect(governor.signer).initialSnapshot(toBN(0)) + await advanceBlocks(toBN('4')) - // now we're at lastRewardsUpdateBlock + minDripInterval + 3, so keeper reward should be: - // dripRewardPerBlock * 3 supplyBeforeDrip = await grt.totalSupply() + const issuanceBase = await l1Reservoir.issuanceBase() const startAccrued = await l1Reservoir.getAccumulatedRewards(await latestBlock()) expect(startAccrued).to.eq(0) const dripBlock = (await latestBlock()).add(1) // We're gonna drip in the next transaction + const expectedKeeperReward = dripBlock + .sub(await l1Reservoir.lastRewardsUpdateBlock()) + .sub(toBN('2')) + .mul(toGRT('3')) const tracker = await RewardsTracker.create( - supplyBeforeDrip, + issuanceBase, defaults.rewards.issuanceRate, dripBlock, ) expect(await tracker.accRewards(dripBlock)).to.eq(0) const expectedNextDeadline = dripBlock.add(defaults.rewards.dripInterval) const expectedMintedRewards = await tracker.accRewards(expectedNextDeadline) - const expectedMintedAmount = expectedMintedRewards.add(toGRT('9')) - const expectedSentToL2 = expectedMintedRewards.div(2) + const expectedMintedAmount = expectedMintedRewards.add(expectedKeeperReward) + const expectedSentToL2 = expectedMintedRewards.div(2).add(expectedKeeperReward) const tx = await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) @@ -518,15 +625,15 @@ describe('L1Reservoir', () => { .emit(l1Reservoir, 'RewardsDripped') .withArgs(actualAmount.add(escrowedAmount), escrowedAmount, expectedNextDeadline) - const normalizedTokenSupply = (await l1Reservoir.tokenSupplyCache()) + const l2IssuanceBase = (await l1Reservoir.issuanceBase()) .mul(await l1Reservoir.l2RewardsFraction()) .div(toGRT('1')) const issuanceRate = await l1Reservoir.issuanceRate() const expectedCallhookData = l2ReservoirIface.encodeFunctionData('receiveDrip', [ - normalizedTokenSupply, + l2IssuanceBase, issuanceRate, toBN('0'), - toGRT('9'), // keeper reward + expectedKeeperReward, keeper.address, ]) const expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( @@ -557,7 +664,13 @@ describe('L1Reservoir', () => { const expectedSentToL2 = expectedMintedAmount.div(2) const tx = await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) @@ -608,7 +721,13 @@ describe('L1Reservoir', () => { const tx2 = await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) const newActualAmount = await grt.balanceOf(l1Reservoir.address) const newEscrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(newActualAmount)).to.eq( @@ -663,7 +782,13 @@ describe('L1Reservoir', () => { const expectedSentToL2 = expectedMintedAmount.div(2) const tx = await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) const actualAmount = await grt.balanceOf(l1Reservoir.address) const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) @@ -702,7 +827,6 @@ describe('L1Reservoir', () => { supplyBeforeDrip = await grt.totalSupply() const secondDripBlock = (await latestBlock()).add(1) const expectedNewNextDeadline = secondDripBlock.add(defaults.rewards.dripInterval) - const rewardsUntilSecondDripBlock = await tracker.accRewards(secondDripBlock) const expectedTotalRewards = await tracker.accRewards(expectedNewNextDeadline) const expectedNewMintedAmount = expectedTotalRewards.sub(expectedMintedAmount) // The amount sent to L2 should cover up to the new drip block with the old fraction, @@ -711,7 +835,13 @@ describe('L1Reservoir', () => { const tx2 = await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) const newActualAmount = await grt.balanceOf(l1Reservoir.address) const newEscrowedAmount = await grt.balanceOf(bridgeEscrow.address) expect(toRound(newActualAmount)).to.eq( @@ -756,7 +886,9 @@ describe('L1Reservoir', () => { // 5% minute rate (4 blocks) await l1Reservoir.connect(governor.signer).setIssuanceRate(ISSUANCE_RATE_PER_BLOCK) supplyBeforeDrip = await grt.totalSupply() - await l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + await l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) dripBlock = await latestBlock() }) @@ -828,7 +960,13 @@ describe('L1Reservoir', () => { await l1Reservoir.connect(governor.signer).setL2RewardsFraction(lambda) await l1Reservoir .connect(keeper.signer) - .drip(maxGas, gasPriceBid, maxSubmissionCost, keeper.address, { value: defaultEthValue }) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) supplyBeforeDrip = await l1Reservoir.issuanceBase() // Has been updated accordingly dripBlock = await latestBlock() await advanceBlocks(20) diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index f5e18b2f9..2bcae309f 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -122,6 +122,7 @@ describe('Rewards', () => { await grt.connect(wallet.signer).approve(staking.address, toGRT('1000000')) await grt.connect(wallet.signer).approve(curation.address, toGRT('1000000')) } + await l1Reservoir.connect(governor.signer).grantDripPermission(keeper.address) await l1Reservoir.connect(governor.signer).initialSnapshot(toBN(0)) supplyBeforeDrip = await grt.totalSupply() }) @@ -278,7 +279,9 @@ describe('Rewards', () => { beforeEach(async function () { // 5% minute rate (4 blocks) await l1Reservoir.connect(governor.signer).setIssuanceRate(ISSUANCE_RATE_PER_BLOCK) - await l1Reservoir.connect(keeper.signer).drip(toBN(0), toBN(0), toBN(0), keeper.address) + await l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), keeper.address) dripBlock = await latestBlock() }) @@ -660,18 +663,15 @@ describe('Rewards', () => { describe('takeRewards', function () { it('should distribute rewards on closed allocation and stake', async function () { // Align with the epoch boundary - // dripBlock (81) await epochManager.setEpochLength(10) - // dripBlock + 1 await advanceToNextEpoch(epochManager) - // dripBlock + 4 + // Setup await setupIndexerAllocation() const firstSnapshotBlocks = new BN((await latestBlock()).sub(dripBlock).toString()) - // dripBlock + 7 + // Jump await advanceToNextEpoch(epochManager) - // dripBlock + 14 // Before state const beforeTokenSupply = await grt.totalSupply() @@ -797,7 +797,7 @@ describe('Rewards', () => { // Jump await advanceToNextEpoch(epochManager) - // dripBlock + 14 + // dripBlock + 13 // Before state const beforeTokenSupply = await grt.totalSupply() @@ -839,17 +839,23 @@ describe('Rewards', () => { it('should deny and burn rewards if subgraph on denylist', async function () { // Setup + // dripBlock (82) await epochManager.setEpochLength(10) + // dripBlock + 1 await rewardsManager .connect(governor.signer) .setSubgraphAvailabilityOracle(governor.address) + // dripBlock + 2 await rewardsManager.connect(governor.signer).setDenied(subgraphDeploymentID1, true) + // dripBlock + 3 (epoch boundary!) await advanceToNextEpoch(epochManager) + // dripBlock + 13 await setupIndexerAllocation() const firstSnapshotBlocks = new BN((await latestBlock()).sub(dripBlock).toString()) // Jump await advanceToNextEpoch(epochManager) + // dripBlock + 23 const supplyBefore = await grt.totalSupply() // Close allocation. At this point rewards should be collected for that indexer From c4af2f615e32611a12dece9658f231162dbcf9a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Thu, 23 Jun 2022 17:54:23 +0300 Subject: [PATCH 05/21] test: add a test for the keeper reward delivery in L2 --- test/l2/l2Reservoir.test.ts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/test/l2/l2Reservoir.test.ts b/test/l2/l2Reservoir.test.ts index 55c339c60..bfa7026a8 100644 --- a/test/l2/l2Reservoir.test.ts +++ b/test/l2/l2Reservoir.test.ts @@ -106,6 +106,7 @@ describe('L2Reservoir', () => { const validGatewayFinalizeTransfer = async ( callhookData: string, + keeperReward = toGRT('0'), ): Promise => { const tx = await gatewayFinalizeTransfer(callhookData) await expect(tx) @@ -116,7 +117,7 @@ describe('L2Reservoir', () => { // newly minted GRT const receiverBalance = await grt.balanceOf(l2Reservoir.address) - await expect(receiverBalance).eq(dripAmount) + await expect(receiverBalance).eq(dripAmount.sub(keeperReward)) return tx } @@ -211,6 +212,26 @@ describe('L2Reservoir', () => { await expect(await l2Reservoir.issuanceRate()).to.eq(dripIssuanceRate) await expect(tx).emit(l2Reservoir, 'DripReceived').withArgs(dripNormalizedSupply) }) + it('delivers the keeper reward to the beneficiary address', async function () { + normalizedSupply = dripNormalizedSupply + const reward = toBN('15') + const receiveDripTx = await l2Reservoir.populateTransaction.receiveDrip( + dripNormalizedSupply, + dripIssuanceRate, + toBN('0'), + reward, + testAccount1.address, + ) + const tx = await validGatewayFinalizeTransfer(receiveDripTx.data, reward) + dripBlock = await latestBlock() + await expect(await l2Reservoir.normalizedTokenSupplyCache()).to.eq(dripNormalizedSupply) + await expect(await l2Reservoir.issuanceRate()).to.eq(dripIssuanceRate) + await expect(tx).emit(l2Reservoir, 'DripReceived').withArgs(dripNormalizedSupply) + await expect(tx) + .emit(grt, 'Transfer') + .withArgs(l2Reservoir.address, testAccount1.address, reward) + await expect(await grt.balanceOf(testAccount1.address)).to.eq(reward) + }) it('updates the normalized supply cache and issuance rate', async function () { normalizedSupply = dripNormalizedSupply let receiveDripTx = await l2Reservoir.populateTransaction.receiveDrip( From 873b60051a29f0d61014adb3ca59723fabce7554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 27 Jun 2022 15:31:39 +0300 Subject: [PATCH 06/21] fix: provide part of the keeper reward to L2 redeemer --- contracts/l2/reservoir/L2Reservoir.sol | 58 +++++++++++++-- contracts/l2/reservoir/L2ReservoirStorage.sol | 2 + test/l2/l2Reservoir.test.ts | 73 ++++++++++++++++++- 3 files changed, 123 insertions(+), 10 deletions(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 96a344f67..8813ab73a 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -4,12 +4,22 @@ pragma solidity ^0.7.6; pragma abicoder v2; import "@openzeppelin/contracts/math/SafeMath.sol"; +import "arbos-precompiles/arbos/builtin/ArbRetryableTx.sol"; import "../../reservoir/IReservoir.sol"; import "../../reservoir/Reservoir.sol"; import "./IL2Reservoir.sol"; import "./L2ReservoirStorage.sol"; +interface IArbTxWithRedeemer { + /** + * @notice Gets the redeemer of the current retryable redeem attempt. + * Returns the zero address if the current transaction is not a retryable redeem attempt. + * If this is an auto-redeem, returns the fee refund address of the retryable. + */ + function getCurrentRedeemer() external view returns (address); +} + /** * @title L2 Rewards Reservoir * @dev This contract acts as a reservoir/vault for the rewards to be distributed on Layer 2. @@ -19,8 +29,12 @@ import "./L2ReservoirStorage.sol"; contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { using SafeMath for uint256; + address public constant ARB_TX_ADDRESS = 0x000000000000000000000000000000000000006E; + event DripReceived(uint256 _issuanceBase); event NextDripNonceUpdated(uint256 _nonce); + event L1ReservoirAddressUpdated(address _l1ReservoirAddress); + event L2KeeperRewardFractionUpdated(uint256 _l2KeeperRewardFraction); /** * @dev Checks that the sender is the L2GraphTokenGateway as configured on the Controller. @@ -52,6 +66,29 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { emit NextDripNonceUpdated(_nonce); } + /** + * @dev Sets the L1 Reservoir address + * This is the address on L1 that will appear as redeemer when a ticket + * was auto-redeemed. + * @param _l1ReservoirAddress New address for the L1Reservoir on L1 + */ + function setL1ReservoirAddress(address _l1ReservoirAddress) external onlyGovernor { + l1ReservoirAddress = _l1ReservoirAddress; + emit L1ReservoirAddressUpdated(_l1ReservoirAddress); + } + + /** + * @dev Sets the L2 keeper reward fraction + * This is the fraction of the keeper reward that will be sent to the redeemer on L2 + * if the retryable ticket is not auto-redeemed + * @param _l2KeeperRewardFraction New value for the fraction, with fixed point at 1e18 + */ + function setL2KeeperRewardFraction(uint256 _l2KeeperRewardFraction) external onlyGovernor { + require(_l2KeeperRewardFraction <= FIXED_POINT_SCALING_FACTOR, "INVALID_VALUE"); + l2KeeperRewardFraction = _l2KeeperRewardFraction; + emit L2KeeperRewardFractionUpdated(_l2KeeperRewardFraction); + } + /** * @dev Get new total rewards accumulated since the last drip. * This is deltaR = p * r ^ (blocknum - t0) - p, where: @@ -115,14 +152,19 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { } issuanceBase = _issuanceBase; IGraphToken grt = graphToken(); - // We'd like to reward the keeper that redeemed the tx in L2 - // but this won't work right now as tx.origin will actually be the L1 sender. - // uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div(TOKEN_DECIMALS); - // // solhint-disable-next-line avoid-tx-origin - // grt.transfer(tx.origin, _l2KeeperReward); - // grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); - // So for now we just send all the rewards to teh L1 keeper: - grt.transfer(_l1Keeper, _keeperReward); + + // Part of the reward always goes to whoever redeemed the ticket in L2, + // unless this was an autoredeem, in which case the "redeemer" is the sender, i.e. L1Reservoir + address redeemer = IArbTxWithRedeemer(ARB_TX_ADDRESS).getCurrentRedeemer(); + if (redeemer != l1ReservoirAddress) { + uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div(FIXED_POINT_SCALING_FACTOR); + grt.transfer(redeemer, _l2KeeperReward); + grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); + } else { + // In an auto-redeem, we just send all the rewards to teh L1 keeper: + grt.transfer(_l1Keeper, _keeperReward); + } + emit DripReceived(issuanceBase); } diff --git a/contracts/l2/reservoir/L2ReservoirStorage.sol b/contracts/l2/reservoir/L2ReservoirStorage.sol index 4e8c6825d..28c562901 100644 --- a/contracts/l2/reservoir/L2ReservoirStorage.sol +++ b/contracts/l2/reservoir/L2ReservoirStorage.sol @@ -13,4 +13,6 @@ contract L2ReservoirV1Storage { contract L2ReservoirV2Storage is L2ReservoirV1Storage { // Fraction of the keeper reward to send to the retryable tx redeemer in L2 (fixed point 1e18) uint256 public l2KeeperRewardFraction; + // Address of the L1Reservoir on L1, used to check if a ticket was auto-redeemed + address public l1ReservoirAddress; } diff --git a/test/l2/l2Reservoir.test.ts b/test/l2/l2Reservoir.test.ts index bfa7026a8..0551c1e13 100644 --- a/test/l2/l2Reservoir.test.ts +++ b/test/l2/l2Reservoir.test.ts @@ -4,6 +4,7 @@ import { BigNumber, constants, ContractTransaction, utils } from 'ethers' import { L2FixtureContracts, NetworkFixture } from '../lib/fixtures' import { BigNumber as BN } from 'bignumber.js' +import { FakeContract, smock } from '@defi-wonderland/smock' import { advanceBlocks, @@ -30,11 +31,13 @@ const dripIssuanceRate = toBN('1000000023206889619') describe('L2Reservoir', () => { let governor: Account let testAccount1: Account + let testAccount2: Account let mockRouter: Account let mockL1GRT: Account let mockL1Gateway: Account let mockL1Reservoir: Account let fixture: NetworkFixture + let arbTxMock: FakeContract let grt: L2GraphToken let l2Reservoir: L2Reservoir @@ -122,7 +125,7 @@ describe('L2Reservoir', () => { } before(async function () { - ;[governor, testAccount1, mockRouter, mockL1GRT, mockL1Gateway, mockL1Reservoir] = + ;[governor, testAccount1, mockRouter, mockL1GRT, mockL1Gateway, mockL1Reservoir, testAccount2] = await getAccounts() fixture = new NetworkFixture() @@ -136,6 +139,11 @@ describe('L2Reservoir', () => { mockL1Gateway.address, mockL1Reservoir.address, ) + + arbTxMock = await smock.fake('IArbTxWithRedeemer', { + address: '0x000000000000000000000000000000000000006E', + }) + arbTxMock.getCurrentRedeemer.returns(mockL1Reservoir.address) }) beforeEach(async function () { @@ -157,7 +165,42 @@ describe('L2Reservoir', () => { await expect(await l2Reservoir.nextDripNonce()).to.eq(toBN('10')) }) }) + + describe('setL1ReservoirAddress', async function () { + it('rejects unauthorized calls', async function () { + const tx = l2Reservoir + .connect(testAccount1.signer) + .setL1ReservoirAddress(testAccount1.address) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + it('sets the L1Reservoir address', async function () { + const tx = l2Reservoir.connect(governor.signer).setL1ReservoirAddress(testAccount1.address) + await expect(tx).emit(l2Reservoir, 'L1ReservoirAddressUpdated').withArgs(testAccount1.address) + await expect(await l2Reservoir.l1ReservoirAddress()).to.eq(testAccount1.address) + }) + }) + + describe('setL2KeeperRewardFraction', async function () { + it('rejects unauthorized calls', async function () { + const tx = l2Reservoir.connect(testAccount1.signer).setL2KeeperRewardFraction(toBN(1)) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + it('rejects invalid values (> 1)', async function () { + const tx = l2Reservoir.connect(governor.signer).setL2KeeperRewardFraction(toGRT('1.000001')) + await expect(tx).revertedWith('INVALID_VALUE') + }) + it('sets the L1Reservoir address', async function () { + const tx = l2Reservoir.connect(governor.signer).setL2KeeperRewardFraction(toGRT('0.999')) + await expect(tx).emit(l2Reservoir, 'L2KeeperRewardFractionUpdated').withArgs(toGRT('0.999')) + await expect(await l2Reservoir.l2KeeperRewardFraction()).to.eq(toGRT('0.999')) + }) + }) + describe('receiveDrip', async function () { + beforeEach(async function () { + await l2Reservoir.connect(governor.signer).setL2KeeperRewardFraction(toGRT('0.2')) + await l2Reservoir.connect(governor.signer).setL1ReservoirAddress(mockL1Reservoir.address) + }) it('rejects the call when not called by the gateway', async function () { const tx = l2Reservoir .connect(governor.signer) @@ -224,7 +267,7 @@ describe('L2Reservoir', () => { ) const tx = await validGatewayFinalizeTransfer(receiveDripTx.data, reward) dripBlock = await latestBlock() - await expect(await l2Reservoir.normalizedTokenSupplyCache()).to.eq(dripNormalizedSupply) + await expect(await l2Reservoir.issuanceBase()).to.eq(dripNormalizedSupply) await expect(await l2Reservoir.issuanceRate()).to.eq(dripIssuanceRate) await expect(tx).emit(l2Reservoir, 'DripReceived').withArgs(dripNormalizedSupply) await expect(tx) @@ -232,6 +275,32 @@ describe('L2Reservoir', () => { .withArgs(l2Reservoir.address, testAccount1.address, reward) await expect(await grt.balanceOf(testAccount1.address)).to.eq(reward) }) + it('delivers part of the keeper reward to the L2 redeemer', async function () { + arbTxMock.getCurrentRedeemer.returns(testAccount2.address) + await l2Reservoir.connect(governor.signer).setL2KeeperRewardFraction(toGRT('0.25')) + normalizedSupply = dripNormalizedSupply + const reward = toGRT('16') + const receiveDripTx = await l2Reservoir.populateTransaction.receiveDrip( + dripNormalizedSupply, + dripIssuanceRate, + toBN('0'), + reward, + testAccount1.address, + ) + const tx = await validGatewayFinalizeTransfer(receiveDripTx.data, reward) + dripBlock = await latestBlock() + await expect(await l2Reservoir.issuanceBase()).to.eq(dripNormalizedSupply) + await expect(await l2Reservoir.issuanceRate()).to.eq(dripIssuanceRate) + await expect(tx).emit(l2Reservoir, 'DripReceived').withArgs(dripNormalizedSupply) + await expect(tx) + .emit(grt, 'Transfer') + .withArgs(l2Reservoir.address, testAccount1.address, toGRT('12')) + await expect(tx) + .emit(grt, 'Transfer') + .withArgs(l2Reservoir.address, testAccount2.address, toGRT('4')) + await expect(await grt.balanceOf(testAccount1.address)).to.eq(toGRT('12')) + await expect(await grt.balanceOf(testAccount2.address)).to.eq(toGRT('4')) + }) it('updates the normalized supply cache and issuance rate', async function () { normalizedSupply = dripNormalizedSupply let receiveDripTx = await l2Reservoir.populateTransaction.receiveDrip( From 634477aa3f1d6d125676b90352043b0939d854af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Fri, 15 Jul 2022 13:20:52 +0200 Subject: [PATCH 07/21] fix: clean up comments about redeemer --- contracts/l2/reservoir/IL2Reservoir.sol | 3 ++- contracts/l2/reservoir/L2Reservoir.sol | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/contracts/l2/reservoir/IL2Reservoir.sol b/contracts/l2/reservoir/IL2Reservoir.sol index 5fd2fc861..696e55797 100644 --- a/contracts/l2/reservoir/IL2Reservoir.sol +++ b/contracts/l2/reservoir/IL2Reservoir.sol @@ -22,7 +22,8 @@ interface IL2Reservoir is IReservoir { * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed * again once the ticket for previous drip has been redeemed. * A keeper reward will be sent to the keeper that dripped on L1, and part of it - * to whoever redeemed the current retryable ticket (tx.origin) + * to whoever redeemed the current retryable ticket (as reported by ArbRetryableTx.getCurrentRedeemer) if + * the ticket is not auto-redeemed. * @param _issuanceBase Base value for token issuance (approximation for token supply times L2 rewards fraction) * @param _issuanceRate Rewards issuance rate, using fixed point at 1e18, and including a +1 * @param _nonce Incrementing nonce to ensure messages are received in order diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 8813ab73a..e52507f34 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -126,7 +126,8 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { * because it checks an incrementing nonce. If that is the case, the retryable ticket can be redeemed * again once the ticket for previous drip has been redeemed. * A keeper reward will be sent to the keeper that dripped on L1, and part of it - * to whoever redeemed the current retryable ticket (tx.origin) + * to whoever redeemed the current retryable ticket (as reported by ArbRetryableTx.getCurrentRedeemer) if + * the ticket is not auto-redeemed. * @param _issuanceBase Base value for token issuance (approximation for token supply times L2 rewards fraction) * @param _issuanceRate Rewards issuance rate, using fixed point at 1e18, and including a +1 * @param _nonce Incrementing nonce to ensure messages are received in order @@ -157,7 +158,9 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { // unless this was an autoredeem, in which case the "redeemer" is the sender, i.e. L1Reservoir address redeemer = IArbTxWithRedeemer(ARB_TX_ADDRESS).getCurrentRedeemer(); if (redeemer != l1ReservoirAddress) { - uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div(FIXED_POINT_SCALING_FACTOR); + uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div( + FIXED_POINT_SCALING_FACTOR + ); grt.transfer(redeemer, _l2KeeperReward); grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); } else { From b5429434cd390e61c9992606b60d95cf52c18664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Fri, 15 Jul 2022 13:28:46 +0200 Subject: [PATCH 08/21] fix: more documentation details --- contracts/l2/reservoir/L2Reservoir.sol | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index e52507f34..16f60e9e4 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -31,10 +31,10 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { address public constant ARB_TX_ADDRESS = 0x000000000000000000000000000000000000006E; - event DripReceived(uint256 _issuanceBase); - event NextDripNonceUpdated(uint256 _nonce); - event L1ReservoirAddressUpdated(address _l1ReservoirAddress); - event L2KeeperRewardFractionUpdated(uint256 _l2KeeperRewardFraction); + event DripReceived(uint256 issuanceBase); + event NextDripNonceUpdated(uint256 nonce); + event L1ReservoirAddressUpdated(address l1ReservoirAddress); + event L2KeeperRewardFractionUpdated(uint256 l2KeeperRewardFraction); /** * @dev Checks that the sender is the L2GraphTokenGateway as configured on the Controller. @@ -50,6 +50,10 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { * are not set here because they are set from L1 through the drip function. * The RewardsManager's address might also not be available in the controller at initialization * time, so approveRewardsManager() must be called separately. + * The l1ReservoirAddress must also be set separately through setL1ReservoirAddress + * for the same reason. + * In the same vein, the l2KeeperRewardFraction is assumed to be zero at initialization, + * so it must be set through setL2KeeperRewardFraction. * @param _controller Address of the Controller that manages this contract */ function initialize(address _controller) external onlyImpl { From 6ef2faa68a84def6fdfa5b4e1b0ec52eee1a67a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Fri, 15 Jul 2022 14:02:09 +0200 Subject: [PATCH 09/21] fix: use safe math for minDripInterval --- contracts/l2/reservoir/IL2Reservoir.sol | 2 +- contracts/l2/reservoir/L2Reservoir.sol | 4 ++-- contracts/reservoir/L1Reservoir.sol | 7 ++++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/contracts/l2/reservoir/IL2Reservoir.sol b/contracts/l2/reservoir/IL2Reservoir.sol index 696e55797..b8a2311c9 100644 --- a/contracts/l2/reservoir/IL2Reservoir.sol +++ b/contracts/l2/reservoir/IL2Reservoir.sol @@ -27,7 +27,7 @@ interface IL2Reservoir is IReservoir { * @param _issuanceBase Base value for token issuance (approximation for token supply times L2 rewards fraction) * @param _issuanceRate Rewards issuance rate, using fixed point at 1e18, and including a +1 * @param _nonce Incrementing nonce to ensure messages are received in order - * @param _keeperReward Keeper reward to distribute between keeper that called drip and keeper that redeemed the retryable tx + * @param _keeperReward Keeper reward to distribute between keeper that called drip and keeper that redeemed the retryable tx * @param _l1Keeper Address of the keeper that called drip in L1 */ function receiveDrip( diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 16f60e9e4..8803fac68 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -135,7 +135,7 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { * @param _issuanceBase Base value for token issuance (approximation for token supply times L2 rewards fraction) * @param _issuanceRate Rewards issuance rate, using fixed point at 1e18, and including a +1 * @param _nonce Incrementing nonce to ensure messages are received in order - * @param _keeperReward Keeper reward to distribute between keeper that called drip and keeper that redeemed the retryable tx + * @param _keeperReward Keeper reward to distribute between keeper that called drip and keeper that redeemed the retryable tx * @param _l1Keeper Address of the keeper that called drip in L1 */ function receiveDrip( @@ -168,7 +168,7 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { grt.transfer(redeemer, _l2KeeperReward); grt.transfer(_l1Keeper, _keeperReward.sub(_l2KeeperReward)); } else { - // In an auto-redeem, we just send all the rewards to teh L1 keeper: + // In an auto-redeem, we just send all the rewards to the L1 keeper: grt.transfer(_l1Keeper, _keeperReward); } diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 30fa4d82a..807b56914 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -73,6 +73,8 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * to the drip function, that also requires the initial supply snapshot to be taken * using initialSnapshot. For this reason, issuanceRate and l2RewardsFraction * are not initialized here and instead need a call to setIssuanceRate and setL2RewardsFraction. + * The same applies to minDripInterval (set through setMinDripInterval) and dripRewardPerBlock + * (set through setDripRewardPerBlock). * On the other hand, the l2ReservoirAddress is not expected to be known at initialization * time and must therefore be set using setL2ReservoirAddress. * The RewardsManager's address might also not be available in the controller at initialization @@ -292,7 +294,10 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { uint256 _l2MaxSubmissionCost, address _keeperRewardBeneficiary ) private { - require(block.number > lastRewardsUpdateBlock + minDripInterval, "WAIT_FOR_MIN_INTERVAL"); + require( + block.number > lastRewardsUpdateBlock.add(minDripInterval), + "WAIT_FOR_MIN_INTERVAL" + ); uint256 mintedRewardsTotal = getNewGlobalRewards(rewardsMintedUntilBlock); uint256 mintedRewardsActual = getNewGlobalRewards(block.number); From 1a6df5d6bc9b54ef0d0aab9dc9b07efae8919332 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Fri, 15 Jul 2022 14:46:21 +0200 Subject: [PATCH 10/21] fix: validate input when granting/revoking drip permission --- contracts/reservoir/L1Reservoir.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 807b56914..d9fb22f0e 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -170,6 +170,8 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * @param _dripper Address that will be an allowed dripper */ function grantDripPermission(address _dripper) external onlyGovernor { + require(_dripper != address(0), "INVALID_ADDRESS"); + require(!allowedDrippers[_dripper], "ALREADY_A_DRIPPER"); allowedDrippers[_dripper] = true; emit AllowedDripperAdded(_dripper); } @@ -179,6 +181,8 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * @param _dripper Address that will not be an allowed dripper anymore */ function revokeDripPermission(address _dripper) external onlyGovernor { + require(_dripper != address(0), "INVALID_ADDRESS"); + require(allowedDrippers[_dripper], "NOT_A_DRIPPER"); allowedDrippers[_dripper] = false; emit AllowedDripperRevoked(_dripper); } From 120753f2ed1413bf994f71e9de02a16ac75e3459 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Thu, 28 Jul 2022 16:09:49 +0200 Subject: [PATCH 11/21] fix: docs and inheritance for IArbTxWithRedeemer --- contracts/l2/reservoir/L2Reservoir.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 8803fac68..839362ccd 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -11,7 +11,12 @@ import "../../reservoir/Reservoir.sol"; import "./IL2Reservoir.sol"; import "./L2ReservoirStorage.sol"; -interface IArbTxWithRedeemer { +/** + * @dev ArbRetryableTx with additional interface to query the current redeemer. + * This is being added by the Arbitrum team but hasn't made it into the arbos-precompiles + * package yet. + */ +interface IArbTxWithRedeemer is ArbRetryableTx { /** * @notice Gets the redeemer of the current retryable redeem attempt. * Returns the zero address if the current transaction is not a retryable redeem attempt. From 00d454704f58070457d7f0f7e71ecbea685f0931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Fri, 5 Aug 2022 14:17:51 +0200 Subject: [PATCH 12/21] fix: remove minDripInterval from the drip keeper reward calculation [L-01] --- contracts/reservoir/L1Reservoir.sol | 4 +--- test/reservoir/l1Reservoir.test.ts | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index d9fb22f0e..0e6fc5b47 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -307,9 +307,7 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { uint256 mintedRewardsActual = getNewGlobalRewards(block.number); // eps = (signed int) mintedRewardsTotal - mintedRewardsActual - uint256 keeperReward = dripRewardPerBlock.mul( - block.number.sub(lastRewardsUpdateBlock).sub(minDripInterval) - ); + uint256 keeperReward = dripRewardPerBlock.mul(block.number.sub(lastRewardsUpdateBlock)); if (nextIssuanceRate != issuanceRate) { rewardsManager().updateAccRewardsPerSignal(); snapshotAccumulatedRewards(mintedRewardsActual); // This updates lastRewardsUpdateBlock diff --git a/test/reservoir/l1Reservoir.test.ts b/test/reservoir/l1Reservoir.test.ts index 9c272c847..0e5fdf415 100644 --- a/test/reservoir/l1Reservoir.test.ts +++ b/test/reservoir/l1Reservoir.test.ts @@ -592,7 +592,6 @@ describe('L1Reservoir', () => { const dripBlock = (await latestBlock()).add(1) // We're gonna drip in the next transaction const expectedKeeperReward = dripBlock .sub(await l1Reservoir.lastRewardsUpdateBlock()) - .sub(toBN('2')) .mul(toGRT('3')) const tracker = await RewardsTracker.create( issuanceBase, From d36aab9647344dc357a6d9cd0e33fee04f3c832b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 8 Aug 2022 16:48:46 +0200 Subject: [PATCH 13/21] fix: use L2 alias of l1ReservoirAddress when comparing getCurrentRedeemer [H-01] --- contracts/l2/reservoir/L2Reservoir.sol | 3 ++- test/l2/l2Reservoir.test.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 839362ccd..2b8cd80fe 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -6,6 +6,7 @@ pragma abicoder v2; import "@openzeppelin/contracts/math/SafeMath.sol"; import "arbos-precompiles/arbos/builtin/ArbRetryableTx.sol"; +import "../../arbitrum/AddressAliasHelper.sol"; import "../../reservoir/IReservoir.sol"; import "../../reservoir/Reservoir.sol"; import "./IL2Reservoir.sol"; @@ -166,7 +167,7 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { // Part of the reward always goes to whoever redeemed the ticket in L2, // unless this was an autoredeem, in which case the "redeemer" is the sender, i.e. L1Reservoir address redeemer = IArbTxWithRedeemer(ARB_TX_ADDRESS).getCurrentRedeemer(); - if (redeemer != l1ReservoirAddress) { + if (redeemer != AddressAliasHelper.applyL1ToL2Alias(l1ReservoirAddress)) { uint256 _l2KeeperReward = _keeperReward.mul(l2KeeperRewardFraction).div( FIXED_POINT_SCALING_FACTOR ); diff --git a/test/l2/l2Reservoir.test.ts b/test/l2/l2Reservoir.test.ts index 0551c1e13..b7cfd2101 100644 --- a/test/l2/l2Reservoir.test.ts +++ b/test/l2/l2Reservoir.test.ts @@ -16,6 +16,7 @@ import { Account, RewardsTracker, getL2SignerFromL1, + applyL1ToL2Alias, } from '../lib/testHelpers' import { L2Reservoir } from '../../build/types/L2Reservoir' @@ -143,7 +144,7 @@ describe('L2Reservoir', () => { arbTxMock = await smock.fake('IArbTxWithRedeemer', { address: '0x000000000000000000000000000000000000006E', }) - arbTxMock.getCurrentRedeemer.returns(mockL1Reservoir.address) + arbTxMock.getCurrentRedeemer.returns(applyL1ToL2Alias(mockL1Reservoir.address)) }) beforeEach(async function () { From 2eea58c0edf1e8cbb506595f9ad64aaa45ca4ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Wed, 17 Aug 2022 13:30:58 +0200 Subject: [PATCH 14/21] fix: don't include keeper reward twice when computing what to send to L2 [H-03] [L-03] --- contracts/reservoir/L1Reservoir.sol | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 0e6fc5b47..8f62fb951 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -321,22 +321,18 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { // n = deltaR(t1, t0) uint256 newRewardsToDistribute = getNewGlobalRewards(rewardsMintedUntilBlock); // N = n - eps - uint256 tokensToMint; + uint256 rewardsTokensToMint; { - uint256 newRewardsPlusMintedActual = newRewardsToDistribute - .add(mintedRewardsActual) - .add(keeperReward); + uint256 newRewardsPlusMintedActual = newRewardsToDistribute.add(mintedRewardsActual); require( - newRewardsPlusMintedActual >= mintedRewardsTotal, - "Would mint negative tokens, wait before calling again" + newRewardsPlusMintedActual > mintedRewardsTotal, + "Would mint negative or zero tokens, wait before calling again" ); - tokensToMint = newRewardsPlusMintedActual.sub(mintedRewardsTotal); + rewardsTokensToMint = newRewardsPlusMintedActual.sub(mintedRewardsTotal); } IGraphToken grt = graphToken(); - if (tokensToMint > 0) { - grt.mint(address(this), tokensToMint); - } + grt.mint(address(this), rewardsTokensToMint.add(keeperReward)); uint256 tokensToSendToL2 = 0; if (l2RewardsFraction != nextL2RewardsFraction) { @@ -376,7 +372,7 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { _keeperRewardBeneficiary ); } else if (l2RewardsFraction > 0) { - tokensToSendToL2 = tokensToMint + tokensToSendToL2 = rewardsTokensToMint .mul(l2RewardsFraction) .div(FIXED_POINT_SCALING_FACTOR) .add(keeperReward); @@ -395,7 +391,11 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { // If we don't send rewards to L2, pay the keeper reward in L1 grt.transfer(_keeperRewardBeneficiary, keeperReward); } - emit RewardsDripped(tokensToMint, tokensToSendToL2, rewardsMintedUntilBlock); + emit RewardsDripped( + rewardsTokensToMint.add(keeperReward), + tokensToSendToL2, + rewardsMintedUntilBlock + ); } /** From 448f506d09adc75740b88dcc8098a25c23b94c8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 15:39:09 +0200 Subject: [PATCH 15/21] test: add test to ensure no DoS if l2RewardsFraction is zeroed [H-04] --- test/reservoir/l1Reservoir.test.ts | 139 +++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/test/reservoir/l1Reservoir.test.ts b/test/reservoir/l1Reservoir.test.ts index 0e5fdf415..64eedbf36 100644 --- a/test/reservoir/l1Reservoir.test.ts +++ b/test/reservoir/l1Reservoir.test.ts @@ -878,6 +878,145 @@ describe('L1Reservoir', () => { expectedNewNextDeadline, ) }) + + it('reverts for a while but can be called again later if L2 fraction goes to zero', async function () { + await l1Reservoir.connect(governor.signer).setL2RewardsFraction(toGRT('0.5')) + + // First drip call, sending half the rewards to L2 + supplyBeforeDrip = await grt.totalSupply() + const startAccrued = await l1Reservoir.getAccumulatedRewards(await latestBlock()) + expect(startAccrued).to.eq(0) + const dripBlock = (await latestBlock()).add(1) // We're gonna drip in the next transaction + const tracker = await RewardsTracker.create( + supplyBeforeDrip, + defaults.rewards.issuanceRate, + dripBlock, + ) + expect(await tracker.accRewards(dripBlock)).to.eq(0) + const expectedNextDeadline = dripBlock.add(defaults.rewards.dripInterval) + const expectedMintedAmount = await tracker.accRewards(expectedNextDeadline) + const expectedSentToL2 = expectedMintedAmount.div(2) + const tx = await l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) + const actualAmount = await grt.balanceOf(l1Reservoir.address) + const escrowedAmount = await grt.balanceOf(bridgeEscrow.address) + expect(toRound(actualAmount)).to.eq(toRound(expectedMintedAmount.sub(expectedSentToL2))) + expect(toRound((await grt.totalSupply()).sub(supplyBeforeDrip))).to.eq( + toRound(expectedMintedAmount), + ) + expect(toRound(escrowedAmount)).to.eq(toRound(expectedSentToL2)) + await expect(tx) + .emit(l1Reservoir, 'RewardsDripped') + .withArgs(actualAmount.add(escrowedAmount), escrowedAmount, expectedNextDeadline) + + let l2IssuanceBase = (await l1Reservoir.issuanceBase()) + .mul(await l1Reservoir.l2RewardsFraction()) + .div(toGRT('1')) + const issuanceRate = await l1Reservoir.issuanceRate() + let expectedCallhookData = l2ReservoirIface.encodeFunctionData('receiveDrip', [ + l2IssuanceBase, + issuanceRate, + toBN('0'), + toBN('0'), + keeper.address, + ]) + let expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( + grt.address, + l1Reservoir.address, + mockL2Reservoir.address, + escrowedAmount, + expectedCallhookData, + ) + await expect(tx) + .emit(l1GraphTokenGateway, 'TxToL2') + .withArgs(l1Reservoir.address, mockL2Gateway.address, toBN(1), expectedL2Data) + + await tracker.snapshotRewards() + + await l1Reservoir.connect(governor.signer).setL2RewardsFraction(toGRT('0')) + + // Second attempt to drip immediately afterwards will revert, because we + // would have to send negative tokens to L2 to compensate + const tx2 = l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) + await expect(tx2).revertedWith( + 'Negative amount would be sent to L2, wait before calling again', + ) + + await advanceBlocks(await l1Reservoir.dripInterval()) + + // Now we should be able to drip again, and a small amount will be sent to L2 + // to cover the few blocks since the drip interval was over + supplyBeforeDrip = await grt.totalSupply() + const secondDripBlock = (await latestBlock()).add(1) + const expectedNewNextDeadline = secondDripBlock.add(defaults.rewards.dripInterval) + const rewardsUntilSecondDripBlock = await tracker.accRewards(secondDripBlock) + const expectedTotalRewards = await tracker.accRewards(expectedNewNextDeadline) + const expectedNewMintedAmount = expectedTotalRewards.sub(expectedMintedAmount) + // The amount sent to L2 should cover up to the new drip block with the old fraction, + // and from then onwards with the new fraction, that is zero + const expectedNewTotalSentToL2 = rewardsUntilSecondDripBlock.div(2) + + const tx3 = await l1Reservoir + .connect(keeper.signer) + ['drip(uint256,uint256,uint256,address)']( + maxGas, + gasPriceBid, + maxSubmissionCost, + keeper.address, + { value: defaultEthValue }, + ) + const newActualAmount = await grt.balanceOf(l1Reservoir.address) + const newEscrowedAmount = await grt.balanceOf(bridgeEscrow.address) + expect(toRound(newActualAmount)).to.eq( + toRound(expectedTotalRewards.sub(expectedNewTotalSentToL2)), + ) + expect(toRound((await grt.totalSupply()).sub(supplyBeforeDrip))).to.eq( + toRound(expectedNewMintedAmount), + ) + expect(toRound(newEscrowedAmount)).to.eq(toRound(expectedNewTotalSentToL2)) + l2IssuanceBase = (await l1Reservoir.issuanceBase()) + .mul(await l1Reservoir.l2RewardsFraction()) + .div(toGRT('1')) + expectedCallhookData = l2ReservoirIface.encodeFunctionData('receiveDrip', [ + l2IssuanceBase, + issuanceRate, + toBN('1'), // Incremented nonce + toBN('0'), + keeper.address, + ]) + expectedL2Data = await l1GraphTokenGateway.getOutboundCalldata( + grt.address, + l1Reservoir.address, + mockL2Reservoir.address, + newEscrowedAmount.sub(escrowedAmount), + expectedCallhookData, + ) + await expect(tx3) + .emit(l1GraphTokenGateway, 'TxToL2') + .withArgs(l1Reservoir.address, mockL2Gateway.address, toBN(2), expectedL2Data) + await expect(tx3) + .emit(l1Reservoir, 'RewardsDripped') + .withArgs( + newActualAmount.add(newEscrowedAmount).sub(actualAmount.add(escrowedAmount)), + newEscrowedAmount.sub(escrowedAmount), + expectedNewNextDeadline, + ) + }) }) context('calculating rewards', async function () { From cc51cb75c954e38fe821784da71eae49e7e3ca3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 15:40:10 +0200 Subject: [PATCH 16/21] test: optimize functions to advance blocks and fix some race conditions --- test/lib/testHelpers.ts | 23 ++++++++--------------- test/rewards/rewards.test.ts | 10 +++++----- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/test/lib/testHelpers.ts b/test/lib/testHelpers.ts index 6673349e3..998d0d335 100644 --- a/test/lib/testHelpers.ts +++ b/test/lib/testHelpers.ts @@ -2,7 +2,7 @@ import hre from 'hardhat' import '@nomiclabs/hardhat-ethers' import '@nomiclabs/hardhat-waffle' import { providers, utils, BigNumber, Signer, Wallet } from 'ethers' -import { formatUnits, getAddress } from 'ethers/lib/utils' +import { formatUnits, getAddress, hexValue } from 'ethers/lib/utils' import { BigNumber as BN } from 'bignumber.js' import { EpochManager } from '../../build/types/EpochManager' @@ -66,24 +66,17 @@ export const advanceBlockTo = async (blockNumber: string | number | BigNumber): ? toBN(blockNumber) : blockNumber const currentBlock = await latestBlock() - const start = Date.now() - let notified - if (target.lt(currentBlock)) + if (target.lt(currentBlock)) { throw Error(`Target block #(${target}) is lower than current block #(${currentBlock})`) - while ((await latestBlock()).lt(target)) { - if (!notified && Date.now() - start >= 5000) { - notified = true - console.log(`advanceBlockTo: Advancing too ` + 'many blocks is causing this test to be slow.') - } - await advanceBlock() + } else if (target.eq(currentBlock)) { + return + } else { + await advanceBlocks(target.sub(currentBlock)) } } export const advanceBlocks = async (blocks: string | number | BigNumber): Promise => { - const steps = typeof blocks === 'number' || typeof blocks === 'string' ? toBN(blocks) : blocks - const currentBlock = await latestBlock() - const toBlock = currentBlock.add(steps) - return advanceBlockTo(toBlock) + await provider().send('hardhat_mine', [hexValue(BigNumber.from(blocks))]) } export const advanceToNextEpoch = async (epochManager: EpochManager): Promise => { @@ -185,7 +178,7 @@ export class RewardsTracker { async snapshotPerSignal(totalSignal: BigNumber, atBlock?: BigNumber): Promise { this.accumulatedPerSignal = await this.accRewardsPerSignal(totalSignal, atBlock) this.accumulatedAtLastPerSignalUpdatedBlock = await this.accRewards(atBlock) - this.lastPerSignalUpdatedBlock = atBlock + this.lastPerSignalUpdatedBlock = atBlock || (await latestBlock()) return this.accumulatedPerSignal } diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index 2bcae309f..38639acc4 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -79,7 +79,7 @@ describe('Rewards', () => { ) => { // -- t0 -- const tracker = await RewardsTracker.create(initialSupply, ISSUANCE_RATE_PER_BLOCK, dripBlock) - tracker.snapshotPerSignal(await grt.balanceOf(curation.address)) + await tracker.snapshotPerSignal(await grt.balanceOf(curation.address)) // Jump await advanceBlocks(nBlocks) @@ -330,11 +330,11 @@ describe('Rewards', () => { await curation.connect(curator1.signer).mint(subgraphDeploymentID1, toGRT('1000'), 0) // Minting signal triggers onSubgraphSignalUpgrade before pulling the GRT, // so we snapshot using the previous value - tracker.snapshotPerSignal(prevSignal) + await tracker.snapshotPerSignal(prevSignal) // Update await rewardsManager.updateAccRewardsPerSignal() - tracker.snapshotPerSignal(await grt.balanceOf(curation.address)) + await tracker.snapshotPerSignal(await grt.balanceOf(curation.address)) const contractAccrued = await rewardsManager.accRewardsPerSignal() @@ -359,14 +359,14 @@ describe('Rewards', () => { await curation.connect(curator1.signer).mint(subgraphDeploymentID1, toGRT('1000'), 0) // Minting signal triggers onSubgraphSignalUpgrade before pulling the GRT, // so we snapshot using the previous value - tracker.snapshotPerSignal(prevSignal) + await tracker.snapshotPerSignal(prevSignal) // Jump await advanceBlocks(ISSUANCE_RATE_PERIODS) // Update await rewardsManager.updateAccRewardsPerSignal() - tracker.snapshotPerSignal(await grt.balanceOf(curation.address)) + await tracker.snapshotPerSignal(await grt.balanceOf(curation.address)) const contractAccrued = await rewardsManager.accRewardsPerSignal() const blockNum = await latestBlock() From 0f8fb067dd92a291b49589cec9e8f85952e53b10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 16:25:19 +0200 Subject: [PATCH 17/21] fix: add some missing validation on reservoirs [M-01] --- contracts/l2/reservoir/L2Reservoir.sol | 1 + contracts/reservoir/L1Reservoir.sol | 4 ++++ test/l2/l2Reservoir.test.ts | 4 ++++ test/reservoir/l1Reservoir.test.ts | 20 +++++++++++++++++++- 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 2b8cd80fe..1d6dddf2e 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -83,6 +83,7 @@ contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { * @param _l1ReservoirAddress New address for the L1Reservoir on L1 */ function setL1ReservoirAddress(address _l1ReservoirAddress) external onlyGovernor { + require(_l1ReservoirAddress != address(0), "INVALID_L1_RESERVOIR"); l1ReservoirAddress = _l1ReservoirAddress; emit L1ReservoirAddressUpdated(_l1ReservoirAddress); } diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 8f62fb951..aa25f308d 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -150,6 +150,7 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * @param _minDripInterval Minimum number of blocks since last drip for drip to be allowed */ function setMinDripInterval(uint256 _minDripInterval) external onlyGovernor { + require(_minDripInterval < dripInterval, "MUST_BE_LT_DRIP_INTERVAL"); minDripInterval = _minDripInterval; emit MinDripIntervalUpdated(_minDripInterval); } @@ -302,6 +303,9 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { block.number > lastRewardsUpdateBlock.add(minDripInterval), "WAIT_FOR_MIN_INTERVAL" ); + // Note we only validate that the beneficiary is nonzero, as the caller might + // want to send the reward to an address that is different to the indexer/dripper's address. + require(_keeperRewardBeneficiary != address(0), "INVALID_BENEFICIARY"); uint256 mintedRewardsTotal = getNewGlobalRewards(rewardsMintedUntilBlock); uint256 mintedRewardsActual = getNewGlobalRewards(block.number); diff --git a/test/l2/l2Reservoir.test.ts b/test/l2/l2Reservoir.test.ts index b7cfd2101..26a9ccc75 100644 --- a/test/l2/l2Reservoir.test.ts +++ b/test/l2/l2Reservoir.test.ts @@ -174,6 +174,10 @@ describe('L2Reservoir', () => { .setL1ReservoirAddress(testAccount1.address) await expect(tx).revertedWith('Caller must be Controller governor') }) + it('rejects setting a zero address', async function () { + const tx = l2Reservoir.connect(governor.signer).setL1ReservoirAddress(constants.AddressZero) + await expect(tx).revertedWith('INVALID_L1_RESERVOIR') + }) it('sets the L1Reservoir address', async function () { const tx = l2Reservoir.connect(governor.signer).setL1ReservoirAddress(testAccount1.address) await expect(tx).emit(l2Reservoir, 'L1ReservoirAddressUpdated').withArgs(testAccount1.address) diff --git a/test/reservoir/l1Reservoir.test.ts b/test/reservoir/l1Reservoir.test.ts index 64eedbf36..2ea01ac6d 100644 --- a/test/reservoir/l1Reservoir.test.ts +++ b/test/reservoir/l1Reservoir.test.ts @@ -347,7 +347,18 @@ describe('L1Reservoir', () => { const tx = l1Reservoir.connect(testAccount1.signer).setMinDripInterval(toBN('200')) await expect(tx).revertedWith('Caller must be Controller governor') }) - + it('rejects setting minimum drip interval if equal to dripInterval', async function () { + const tx = l1Reservoir + .connect(governor.signer) + .setMinDripInterval(await l1Reservoir.dripInterval()) + await expect(tx).revertedWith('MUST_BE_LT_DRIP_INTERVAL') + }) + it('rejects setting minimum drip interval if larger than dripInterval', async function () { + const tx = l1Reservoir + .connect(governor.signer) + .setMinDripInterval((await l1Reservoir.dripInterval()).add(1)) + await expect(tx).revertedWith('MUST_BE_LT_DRIP_INTERVAL') + }) it('sets the minimum drip interval', async function () { const newValue = toBN('200') const tx = l1Reservoir.connect(governor.signer).setMinDripInterval(newValue) @@ -406,6 +417,13 @@ describe('L1Reservoir', () => { ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), testAccount1.address) await expect(tx).emit(l1Reservoir, 'RewardsDripped') }) + it('cannot be called with a zero address for the keeper reward beneficiary', async function () { + await l1Reservoir.connect(governor.signer).grantDripPermission(testAccount1.address) + const tx = l1Reservoir + .connect(testAccount1.signer) + ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), constants.AddressZero) + await expect(tx).revertedWith('INVALID_BENEFICIARY') + }) it('can be called by an indexer operator using an extra parameter', async function () { const stakedAmount = toGRT('100000') await grt.connect(governor.signer).mint(testAccount1.address, stakedAmount) From 365e307d021ee84cc8ceb69f51a1961ba4a98c93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 17:59:10 +0200 Subject: [PATCH 18/21] fix: add some missing docstrings [L-04] --- contracts/l2/reservoir/L2Reservoir.sol | 5 +++++ contracts/l2/reservoir/L2ReservoirStorage.sol | 6 +++++- contracts/reservoir/L1ReservoirStorage.sol | 6 +++++- contracts/reservoir/Reservoir.sol | 2 ++ contracts/reservoir/ReservoirStorage.sol | 2 +- 5 files changed, 18 insertions(+), 3 deletions(-) diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 1d6dddf2e..9a341b6b5 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -35,11 +35,16 @@ interface IArbTxWithRedeemer is ArbRetryableTx { contract L2Reservoir is L2ReservoirV2Storage, Reservoir, IL2Reservoir { using SafeMath for uint256; + // Address for the ArbRetryableTx interface provided by Arbitrum address public constant ARB_TX_ADDRESS = 0x000000000000000000000000000000000000006E; + // Emitted when a rewards drip is received from L1 event DripReceived(uint256 issuanceBase); + // Emitted when the next drip nonce is manually updated by governance event NextDripNonceUpdated(uint256 nonce); + // Emitted when the L1Reservoir's address is updated event L1ReservoirAddressUpdated(address l1ReservoirAddress); + // Emitted when the L2 keeper reward fraction is updated event L2KeeperRewardFractionUpdated(uint256 l2KeeperRewardFraction); /** diff --git a/contracts/l2/reservoir/L2ReservoirStorage.sol b/contracts/l2/reservoir/L2ReservoirStorage.sol index 28c562901..4d469f889 100644 --- a/contracts/l2/reservoir/L2ReservoirStorage.sol +++ b/contracts/l2/reservoir/L2ReservoirStorage.sol @@ -3,13 +3,17 @@ pragma solidity ^0.7.6; /** - * @dev Storage variables for the L2Reservoir + * @dev Storage variables for the L2Reservoir, version 1 */ contract L2ReservoirV1Storage { // Expected nonce value for the next drip hook uint256 public nextDripNonce; } +/** + * @dev Storage variables for the L2Reservoir, version 2 + * This version adds some variables needed when introducing the keeper reward. + */ contract L2ReservoirV2Storage is L2ReservoirV1Storage { // Fraction of the keeper reward to send to the retryable tx redeemer in L2 (fixed point 1e18) uint256 public l2KeeperRewardFraction; diff --git a/contracts/reservoir/L1ReservoirStorage.sol b/contracts/reservoir/L1ReservoirStorage.sol index 92b9d5107..9f60249bd 100644 --- a/contracts/reservoir/L1ReservoirStorage.sol +++ b/contracts/reservoir/L1ReservoirStorage.sol @@ -3,7 +3,7 @@ pragma solidity ^0.7.6; /** - * @dev Storage variables for the L1Reservoir + * @dev Storage variables for the L1Reservoir, version 1 */ contract L1ReservoirV1Storage { // Fraction of total rewards to be sent by L2, expressed in fixed point at 1e18 @@ -22,6 +22,10 @@ contract L1ReservoirV1Storage { uint256 public nextDripNonce; } +/** + * @dev Storage variables for the L1Reservoir, version 2 + * This version adds some variables that are needed when introducing keeper rewards. + */ contract L1ReservoirV2Storage is L1ReservoirV1Storage { // Minimum number of blocks since last drip for a new drip to be allowed uint256 public minDripInterval; diff --git a/contracts/reservoir/Reservoir.sol b/contracts/reservoir/Reservoir.sol index d2a0bf6cb..fff7f108a 100644 --- a/contracts/reservoir/Reservoir.sol +++ b/contracts/reservoir/Reservoir.sol @@ -19,7 +19,9 @@ import "./IReservoir.sol"; abstract contract Reservoir is GraphUpgradeable, ReservoirV1Storage, IReservoir { using SafeMath for uint256; + // Scaling factor for all fixed point arithmetics uint256 internal constant FIXED_POINT_SCALING_FACTOR = 1e18; + // Minimum issuance rate (expressed in fixed point at 1e18) uint256 internal constant MIN_ISSUANCE_RATE = 1e18; /** diff --git a/contracts/reservoir/ReservoirStorage.sol b/contracts/reservoir/ReservoirStorage.sol index b46e44d35..b964d87b0 100644 --- a/contracts/reservoir/ReservoirStorage.sol +++ b/contracts/reservoir/ReservoirStorage.sol @@ -5,7 +5,7 @@ pragma solidity ^0.7.6; import "../governance/Managed.sol"; /** - * @dev Base storage variables for the Reservoir on both layers + * @dev Base storage variables for the Reservoir on both layers, version 1 */ contract ReservoirV1Storage is Managed { // Relative increase of the total supply per block, plus 1, expressed in fixed point at 1e18. From 91a0219df44926553ec1bfd8ac70cc8add31624d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 18:14:06 +0200 Subject: [PATCH 19/21] fix: use a single-condition requires for the drip auth check [L-05] --- contracts/reservoir/L1Reservoir.sol | 3 ++- test/reservoir/l1Reservoir.test.ts | 40 ++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index aa25f308d..9c7613700 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -62,7 +62,8 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { * @param _indexer Indexer for which the sender must be an operator */ modifier onlyIndexerOperator(address _indexer) { - require(_isIndexer(_indexer) && staking().isOperator(msg.sender, _indexer), "UNAUTHORIZED"); + require(_isIndexer(_indexer), "UNAUTHORIZED_INVALID_INDEXER"); + require(staking().isOperator(msg.sender, _indexer), "UNAUTHORIZED_INVALID_OPERATOR"); _; } diff --git a/test/reservoir/l1Reservoir.test.ts b/test/reservoir/l1Reservoir.test.ts index 2ea01ac6d..261a36c39 100644 --- a/test/reservoir/l1Reservoir.test.ts +++ b/test/reservoir/l1Reservoir.test.ts @@ -45,6 +45,7 @@ describe('L1Reservoir', () => { let governor: Account let testAccount1: Account let testAccount2: Account + let testAccount3: Account let mockRouter: Account let mockL2GRT: Account let mockL2Gateway: Account @@ -162,6 +163,7 @@ describe('L1Reservoir', () => { mockL2Reservoir, keeper, testAccount2, + testAccount3, ] = await getAccounts() fixture = new NetworkFixture() @@ -424,7 +426,43 @@ describe('L1Reservoir', () => { ['drip(uint256,uint256,uint256,address)'](toBN(0), toBN(0), toBN(0), constants.AddressZero) await expect(tx).revertedWith('INVALID_BENEFICIARY') }) - it('can be called by an indexer operator using an extra parameter', async function () { + it('(operator variant) cannot be called with an invalid indexer', async function () { + const tx = l1Reservoir + .connect(testAccount2.signer) + ['drip(uint256,uint256,uint256,address,address)']( + toBN(0), + toBN(0), + toBN(0), + testAccount1.address, + testAccount1.address, + ) + await expect(tx).revertedWith('UNAUTHORIZED_INVALID_INDEXER') + }) + it('(operator variant) cannot be called by someone who is not an operator for the right indexer', async function () { + const stakedAmount = toGRT('100000') + // testAccount1 is a valid indexer + await grt.connect(governor.signer).mint(testAccount1.address, stakedAmount) + await grt.connect(testAccount1.signer).approve(staking.address, stakedAmount) + await staking.connect(testAccount1.signer).stake(stakedAmount) + // testAccount2 is an operator for testAccount1's indexer + await staking.connect(testAccount1.signer).setOperator(testAccount2.address, true) + // testAccount3 is another valid indexer + await grt.connect(governor.signer).mint(testAccount3.address, stakedAmount) + await grt.connect(testAccount3.signer).approve(staking.address, stakedAmount) + await staking.connect(testAccount3.signer).stake(stakedAmount) + // But testAccount2 is not an operator for testAccount3's indexer + const tx = l1Reservoir + .connect(testAccount2.signer) + ['drip(uint256,uint256,uint256,address,address)']( + toBN(0), + toBN(0), + toBN(0), + testAccount1.address, + testAccount3.address, + ) + await expect(tx).revertedWith('UNAUTHORIZED_INVALID_OPERATOR') + }) + it('(operator variant) can be called by an indexer operator using an extra parameter', async function () { const stakedAmount = toGRT('100000') await grt.connect(governor.signer).mint(testAccount1.address, stakedAmount) await grt.connect(testAccount1.signer).approve(staking.address, stakedAmount) From 80cc2f2b4a8c1aecaabf15f9fa412141eb0b281c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 18:48:34 +0200 Subject: [PATCH 20/21] fix: add indexed params to dripper change events [N-01] --- contracts/reservoir/L1Reservoir.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index 9c7613700..b57d38fa1 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -43,9 +43,9 @@ contract L1Reservoir is L1ReservoirV2Storage, Reservoir { // Emitted when minDripInterval is updated event MinDripIntervalUpdated(uint256 minDripInterval); // Emitted when a new allowedDripper is added - event AllowedDripperAdded(address dripper); + event AllowedDripperAdded(address indexed dripper); // Emitted when an allowedDripper is revoked - event AllowedDripperRevoked(address dripper); + event AllowedDripperRevoked(address indexed dripper); /** * @dev Checks that the sender is an indexer with stake on the Staking contract, From b99d31f4713156bf71f74dde9f7a8a9c54c99653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Mon, 22 Aug 2022 18:45:43 +0200 Subject: [PATCH 21/21] fix: use explicit imports in relevant reservoir contracts [N-02] --- contracts/l2/reservoir/IL2Reservoir.sol | 2 +- contracts/l2/reservoir/L2Reservoir.sol | 18 ++++++++++-------- contracts/reservoir/L1Reservoir.sol | 13 ++++++++----- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/contracts/l2/reservoir/IL2Reservoir.sol b/contracts/l2/reservoir/IL2Reservoir.sol index b8a2311c9..8144efae2 100644 --- a/contracts/l2/reservoir/IL2Reservoir.sol +++ b/contracts/l2/reservoir/IL2Reservoir.sol @@ -2,7 +2,7 @@ pragma solidity ^0.7.6; -import "../../reservoir/IReservoir.sol"; +import { IReservoir } from "../../reservoir/IReservoir.sol"; /** * @title Interface for the L2 Rewards Reservoir diff --git a/contracts/l2/reservoir/L2Reservoir.sol b/contracts/l2/reservoir/L2Reservoir.sol index 9a341b6b5..e5cce6fee 100644 --- a/contracts/l2/reservoir/L2Reservoir.sol +++ b/contracts/l2/reservoir/L2Reservoir.sol @@ -3,14 +3,16 @@ pragma solidity ^0.7.6; pragma abicoder v2; -import "@openzeppelin/contracts/math/SafeMath.sol"; -import "arbos-precompiles/arbos/builtin/ArbRetryableTx.sol"; - -import "../../arbitrum/AddressAliasHelper.sol"; -import "../../reservoir/IReservoir.sol"; -import "../../reservoir/Reservoir.sol"; -import "./IL2Reservoir.sol"; -import "./L2ReservoirStorage.sol"; +import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol"; +import { ArbRetryableTx } from "arbos-precompiles/arbos/builtin/ArbRetryableTx.sol"; + +import { Managed } from "../../governance/Managed.sol"; +import { IGraphToken } from "../../token/IGraphToken.sol"; +import { AddressAliasHelper } from "../../arbitrum/AddressAliasHelper.sol"; +import { IReservoir } from "../../reservoir/IReservoir.sol"; +import { Reservoir } from "../../reservoir/Reservoir.sol"; +import { IL2Reservoir } from "./IL2Reservoir.sol"; +import { L2ReservoirV2Storage } from "./L2ReservoirStorage.sol"; /** * @dev ArbRetryableTx with additional interface to query the current redeemer. diff --git a/contracts/reservoir/L1Reservoir.sol b/contracts/reservoir/L1Reservoir.sol index b57d38fa1..1fc1c30ce 100644 --- a/contracts/reservoir/L1Reservoir.sol +++ b/contracts/reservoir/L1Reservoir.sol @@ -3,13 +3,16 @@ pragma solidity ^0.7.6; pragma abicoder v2; -import "@openzeppelin/contracts/math/SafeMath.sol"; +import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol"; -import "../arbitrum/ITokenGateway.sol"; +import { ITokenGateway } from "../arbitrum/ITokenGateway.sol"; -import "../l2/reservoir/IL2Reservoir.sol"; -import "./Reservoir.sol"; -import "./L1ReservoirStorage.sol"; +import { Managed } from "../governance/Managed.sol"; +import { IGraphToken } from "../token/IGraphToken.sol"; +import { IStaking } from "../staking/IStaking.sol"; +import { IL2Reservoir } from "../l2/reservoir/IL2Reservoir.sol"; +import { Reservoir } from "./Reservoir.sol"; +import { L1ReservoirV2Storage } from "./L1ReservoirStorage.sol"; /** * @title L1 Rewards Reservoir