diff --git a/packages/perennial-account/contracts/Controller_Arbitrum.sol b/packages/perennial-account/contracts/Controller_Arbitrum.sol index 4627466f6..e3f959530 100644 --- a/packages/perennial-account/contracts/Controller_Arbitrum.sol +++ b/packages/perennial-account/contracts/Controller_Arbitrum.sol @@ -11,8 +11,11 @@ import { Controller_Incentivized } from "./Controller_Incentivized.sol"; contract Controller_Arbitrum is Controller_Incentivized, Kept_Arbitrum { /// @dev Creates instance of Controller which compensates keepers /// @param implementation Pristine collateral account contract - constructor(address implementation, IVerifierBase nonceManager) - Controller_Incentivized(implementation, nonceManager) {} + /// @param nonceManager Verifier contract to which nonce and group cancellations are relayed + constructor( + address implementation, + IVerifierBase nonceManager + ) Controller_Incentivized(implementation, nonceManager) {} /// @dev Use the Kept_Arbitrum implementation for calculating the dynamic fee function _calldataFee( diff --git a/packages/perennial-account/contracts/Controller_Incentivized.sol b/packages/perennial-account/contracts/Controller_Incentivized.sol index 15d6865fb..5582280c9 100644 --- a/packages/perennial-account/contracts/Controller_Incentivized.sol +++ b/packages/perennial-account/contracts/Controller_Incentivized.sol @@ -34,14 +34,23 @@ import { Withdrawal } from "./types/Withdrawal.sol"; abstract contract Controller_Incentivized is Controller, IRelayer, Kept { /// @dev Handles relayed messages for nonce cancellation IVerifierBase public immutable nonceManager; - + /// @dev Configuration used to calculate keeper compensation KeepConfig public keepConfig; + /// @dev Configuration used to calculate keeper compensation with buffered gas + KeepConfig public keepConfigBuffered; + + /// @dev Configuration used to calculate keeper compensation to withdraw from a collateral account + KeepConfig public keepConfigWithdrawal; + /// @dev Creates instance of Controller which compensates keepers /// @param implementation_ Pristine collateral account contract - constructor(address implementation_, IVerifierBase nonceManager_) - Controller(implementation_) { + /// @param nonceManager_ Verifier contract to which nonce and group cancellations are relayed + constructor( + address implementation_, + IVerifierBase nonceManager_ + ) Controller(implementation_) { nonceManager = nonceManager_; } @@ -49,64 +58,93 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { /// @param marketFactory_ Contract used to validate delegated signers /// @param verifier_ Contract used to validate collateral account message signatures /// @param chainlinkFeed_ ETH-USD price feed used for calculating keeper compensation - /// @param keepConfig_ Configuration used to compensate keepers + /// @param keepConfig_ Configuration used for unbuffered keeper compensation + /// @param keepConfigBuffered_ Configuration used for buffered keeper compensation + /// @param keepConfigWithdrawal_ Configuration used to compensate keepers for withdrawals function initialize( IMarketFactory marketFactory_, IAccountVerifier verifier_, AggregatorV3Interface chainlinkFeed_, - KeepConfig memory keepConfig_ + KeepConfig memory keepConfig_, + KeepConfig memory keepConfigBuffered_, + KeepConfig memory keepConfigWithdrawal_ ) external initializer(1) { __Factory__initialize(); __Kept__initialize(chainlinkFeed_, DSU); marketFactory = marketFactory_; verifier = verifier_; keepConfig = keepConfig_; + keepConfigBuffered = keepConfigBuffered_; + keepConfigWithdrawal = keepConfigWithdrawal_; } /// @inheritdoc IController function changeRebalanceConfigWithSignature( RebalanceConfigChange calldata configChange, bytes calldata signature - ) override external { + ) + external + override + keepCollateralAccount( + configChange.action.common.account, + abi.encode(configChange, signature), + configChange.action.maxFee, + 0 + ) + { _changeRebalanceConfigWithSignature(configChange, signature); - _compensateKeeper(configChange.action); } /// @inheritdoc IController function deployAccountWithSignature( DeployAccount calldata deployAccount_, bytes calldata signature - ) override external { - IAccount account = _deployAccountWithSignature(deployAccount_, signature); - bytes memory data = abi.encode(address(account), deployAccount_.action.maxFee); - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); + ) + external + override + keepCollateralAccount( + deployAccount_.action.common.account, + abi.encode(deployAccount_, signature), + deployAccount_.action.maxFee, + 0 + ) + { + _deployAccountWithSignature(deployAccount_, signature); } /// @inheritdoc IController function marketTransferWithSignature( MarketTransfer calldata marketTransfer, bytes calldata signature - ) override external { + ) + external + override + keepCollateralAccount( + marketTransfer.action.common.account, + abi.encode(marketTransfer, signature), + marketTransfer.action.maxFee, + 1 + ) + { IAccount account = IAccount(getAccountAddress(marketTransfer.action.common.account)); - bytes memory data = abi.encode(account, marketTransfer.action.maxFee); - - // if we're depositing collateral to the market, pay the keeper before transferring funds - if (marketTransfer.amount.gte(Fixed6Lib.ZERO)) { - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); - _marketTransferWithSignature(account, marketTransfer, signature); - // otherwise handle the keeper fee normally, after withdrawing to the collateral account - } else { - _marketTransferWithSignature(account, marketTransfer, signature); - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); - } + _marketTransferWithSignature(account, marketTransfer, signature); } /// @inheritdoc IController - function rebalanceGroup(address owner, uint256 group) override external { + function rebalanceGroup( + address owner, + uint256 group + ) + external + override + keepCollateralAccount( + owner, + abi.encode(owner, group), + groupToMaxRebalanceFee[owner][group], + groupToMarkets[owner][group].length + ) + { _rebalanceGroup(owner, group); - address account = getAccountAddress(owner); - bytes memory data = abi.encode(account, groupToMaxRebalanceFee[owner][group]); - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); } /// @inheritdoc IController @@ -117,7 +155,13 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { address account = getAccountAddress(withdrawal.action.common.account); // levy fee prior to withdrawal bytes memory data = abi.encode(account, withdrawal.action.maxFee); - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); + _handleKeeperFee( + keepConfigWithdrawal, + 0, // no way to calculate applicable gas prior to invocation + abi.encode(withdrawal, signature), + 0, + data + ); _withdrawWithSignature(IAccount(account), withdrawal, signature); } @@ -126,13 +170,20 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { RelayedNonceCancellation calldata message, bytes calldata outerSignature, bytes calldata innerSignature - ) override external { + ) + external + override + keepCollateralAccount( + message.action.common.account, + abi.encode(message, outerSignature, innerSignature), + message.action.maxFee, + 0 + ) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyRelayedNonceCancellation(message, outerSignature); _ensureValidSigner(message.action.common.account, message.action.common.signer); - _compensateKeeper(message.action); - // relay the message to Verifier nonceManager.cancelNonceWithSignature(message.nonceCancellation, innerSignature); } @@ -142,13 +193,20 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { RelayedGroupCancellation calldata message, bytes calldata outerSignature, bytes calldata innerSignature - ) override external { + ) + external + override + keepCollateralAccount( + message.action.common.account, + abi.encode(message, outerSignature, innerSignature), + message.action.maxFee, + 0 + ) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyRelayedGroupCancellation(message, outerSignature); _ensureValidSigner(message.action.common.account, message.action.common.signer); - _compensateKeeper(message.action); - // relay the message to Verifier nonceManager.cancelGroupWithSignature(message.groupCancellation, innerSignature); } @@ -158,13 +216,20 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { RelayedOperatorUpdate calldata message, bytes calldata outerSignature, bytes calldata innerSignature - ) override external { + ) + external + override + keepCollateralAccount( + message.action.common.account, + abi.encode(message, outerSignature, innerSignature), + message.action.maxFee, + 0 + ) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyRelayedOperatorUpdate(message, outerSignature); _ensureValidSigner(message.action.common.account, message.action.common.signer); - _compensateKeeper(message.action); - // relay the message to MarketFactory marketFactory.updateOperatorWithSignature(message.operatorUpdate, innerSignature); } @@ -174,13 +239,20 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { RelayedSignerUpdate calldata message, bytes calldata outerSignature, bytes calldata innerSignature - ) override external { + ) + external + override + keepCollateralAccount( + message.action.common.account, + abi.encode(message, outerSignature, innerSignature), + message.action.maxFee, + 0 + ) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyRelayedSignerUpdate(message, outerSignature); _ensureValidSigner(message.action.common.account, message.action.common.signer); - _compensateKeeper(message.action); - // relay the message to MarketFactory marketFactory.updateSignerWithSignature(message.signerUpdate, innerSignature); } @@ -190,22 +262,24 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { RelayedAccessUpdateBatch calldata message, bytes calldata outerSignature, bytes calldata innerSignature - ) override external { + ) + external + override + keepCollateralAccount( + message.action.common.account, + abi.encode(message, outerSignature, innerSignature), + message.action.maxFee, + 0 + ) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyRelayedAccessUpdateBatch(message, outerSignature); _ensureValidSigner(message.action.common.account, message.action.common.signer); - _compensateKeeper(message.action); - // relay the message to MarketFactory marketFactory.updateAccessBatchWithSignature(message.accessUpdateBatch, innerSignature); } - function _compensateKeeper(Action calldata action) internal virtual { - bytes memory data = abi.encode(getAccountAddress(action.common.account), action.maxFee); - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); - } - /// @dev Transfers funds from collateral account to controller, and limits compensation /// to the user-defined maxFee in the Action message /// @param amount Calculated keeper fee @@ -224,4 +298,33 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { // transfer DSU to the Controller, such that Kept can transfer to keeper DSU.pull(account, raisedKeeperFee); } -} + + modifier keepCollateralAccount( + address account, + bytes memory applicableCalldata, + UFixed6 maxFee, + uint256 bufferMultiplier + ) { + bytes memory data = abi.encode(getAccountAddress(account), maxFee); + uint256 startGas = gasleft(); + + _; + + uint256 applicableGas = startGas - gasleft(); + + _handleKeeperFee( + bufferMultiplier > 0 + ? KeepConfig( + keepConfigBuffered.multiplierBase, + keepConfigBuffered.bufferBase * (bufferMultiplier), + keepConfigBuffered.multiplierCalldata, + keepConfigBuffered.bufferCalldata * (bufferMultiplier) + ) + : keepConfig, + applicableGas, + applicableCalldata, + 0, + data + ); + } +} \ No newline at end of file diff --git a/packages/perennial-account/test/helpers/arbitrumHelpers.ts b/packages/perennial-account/test/helpers/arbitrumHelpers.ts index ab8ef219e..17002ad05 100644 --- a/packages/perennial-account/test/helpers/arbitrumHelpers.ts +++ b/packages/perennial-account/test/helpers/arbitrumHelpers.ts @@ -25,7 +25,6 @@ import { IOracleProvider, GasOracle__factory, } from '../../types/generated' -import { IKept } from '../../types/generated/contracts/Controller_Arbitrum' import { impersonate } from '../../../common/testutil' import { IVerifier } from '@equilibria/perennial-v2/types/generated' diff --git a/packages/perennial-account/test/integration/Controller_Arbitrum.ts b/packages/perennial-account/test/integration/Controller_Arbitrum.ts index 379882b0d..a7bb1ee78 100644 --- a/packages/perennial-account/test/integration/Controller_Arbitrum.ts +++ b/packages/perennial-account/test/integration/Controller_Arbitrum.ts @@ -58,11 +58,10 @@ import { IKeeperOracle, IOracleFactory, PythFactory } from '@equilibria/perennia const { ethers } = HRE const CHAINLINK_ETH_USD_FEED = '0x639Fe6ab55C921f74e7fac1ee960C0B6293ba612' // price feed used for keeper compensation -const DEFAULT_MAX_FEE_6 = parse6decimal('0.5') -const DEFAULT_MAX_FEE = DEFAULT_MAX_FEE_6.mul(1e12) +const DEFAULT_MAX_FEE = parse6decimal('0.5') // hack around issues estimating gas for instrumented contracts when running tests under coverage -const TX_OVERRIDES = { gasLimit: 3_000_000, maxFeePerGas: 200_000_000 } +const TX_OVERRIDES = { gasLimit: 3_000_000, maxPriorityFeePerGas: 0, maxFeePerGas: 100_000_000 } describe('Controller_Arbitrum', () => { let dsu: IERC20Metadata @@ -84,12 +83,14 @@ describe('Controller_Arbitrum', () => { let receiver: SignerWithAddress let lastNonce = 0 let currentTime: BigNumber + let keeperBalanceBefore: BigNumber + let keeperEthBalanceBefore: BigNumber // create a default action for the specified user with reasonable fee and expiry function createAction( userAddress: Address, signerAddress = userAddress, - maxFee = DEFAULT_MAX_FEE_6, + maxFee = DEFAULT_MAX_FEE, expiresInSeconds = 45, ) { return { @@ -118,6 +119,7 @@ describe('Controller_Arbitrum', () => { const tx = await controller .connect(keeper) .deployAccountWithSignature(deployAccountMessage, signatureCreate, TX_OVERRIDES) + // verify the address from event arguments const creationArgs = await getEventArguments(tx, 'AccountDeployed') expect(creationArgs.account).to.equal(accountAddress) @@ -133,6 +135,19 @@ describe('Controller_Arbitrum', () => { await fundWalletUSDC(wallet, parse6decimal('50000'), { maxFeePerGas: 100000000 }) } + async function checkCompensation(priceCommitments = 0) { + const keeperFeesPaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) + let keeperEthSpentOnGas = keeperEthBalanceBefore.sub(await keeper.getBalance()) + + // if TXes in test required outside price commitments, compensate the keeper for them + keeperEthSpentOnGas = keeperEthSpentOnGas.add(utils.parseEther('0.0000644306').mul(priceCommitments)) + + // cost of transaction + const keeperGasCostInUSD = keeperEthSpentOnGas.mul(3413) + // keeper should be compensated between 100-125% of actual gas cost + expect(keeperFeesPaid).to.be.within(keeperGasCostInUSD, keeperGasCostInUSD.mul(125).div(100)) + } + // create a serial nonce for testing purposes; real users may choose a nonce however they please function nextNonce(): BigNumber { lastNonce += 1 @@ -180,20 +195,35 @@ describe('Controller_Arbitrum', () => { // set up users and deploy artifacts const keepConfig = { - multiplierBase: 0, - bufferBase: 1_000_000, - multiplierCalldata: 0, - bufferCalldata: 500_000, + multiplierBase: ethers.utils.parseEther('1'), + bufferBase: 275_000, // buffer for handling the keeper fee + multiplierCalldata: ethers.utils.parseEther('1'), + bufferCalldata: 0, + } + const keepConfigBuffered = { + multiplierBase: ethers.utils.parseEther('1.08'), + bufferBase: 1_500_000, // for price commitment + multiplierCalldata: ethers.utils.parseEther('1.08'), + bufferCalldata: 35_200, + } + const keepConfigWithdrawal = { + multiplierBase: ethers.utils.parseEther('1.05'), + bufferBase: 1_500_000, + multiplierCalldata: ethers.utils.parseEther('1.05'), + bufferCalldata: 35_200, } const marketVerifier = IVerifier__factory.connect(await marketFactory.verifier(), owner) controller = await deployControllerArbitrum(owner, marketVerifier, { maxFeePerGas: 100000000 }) accountVerifier = await new AccountVerifier__factory(owner).deploy({ maxFeePerGas: 100000000 }) // chainlink feed is used by Kept for keeper compensation - await controller['initialize(address,address,address,(uint256,uint256,uint256,uint256))']( + const KeepConfig = '(uint256,uint256,uint256,uint256)' + await controller[`initialize(address,address,address,${KeepConfig},${KeepConfig},${KeepConfig})`]( marketFactory.address, accountVerifier.address, CHAINLINK_ETH_USD_FEED, keepConfig, + keepConfigBuffered, + keepConfigWithdrawal, ) // fund userA await fundWallet(userA) @@ -211,12 +241,13 @@ describe('Controller_Arbitrum', () => { beforeEach(async () => { // update the timestamp used for calculating expiry and adjusting oracle price currentTime = BigNumber.from(await currentBlockTimestamp()) - await loadFixture(fixture) // set a realistic base gas fee await HRE.ethers.provider.send('hardhat_setNextBlockBaseFeePerGas', ['0x5F5E100']) // 0.1 gwei + keeperBalanceBefore = await dsu.balanceOf(keeper.address) + keeperEthBalanceBefore = await keeper.getBalance() currentTime = BigNumber.from(await currentBlockTimestamp()) }) @@ -238,7 +269,7 @@ describe('Controller_Arbitrum', () => { it('can create an account', async () => { // pre-fund the address where the account will be deployed - await usdc.connect(userA).transfer(accountAddressA, parse6decimal('15000'), { maxFeePerGas: 100000000 }) + await usdc.connect(userA).transfer(accountAddressA, parse6decimal('15000'), TX_OVERRIDES) // sign a message to deploy the account const deployAccountMessage = { @@ -247,37 +278,26 @@ describe('Controller_Arbitrum', () => { const signature = await signDeployAccount(userA, accountVerifier, deployAccountMessage) // keeper executes deployment of the account and is compensated - const keeperBalanceBefore = await dsu.balanceOf(keeper.address) - await expect( - controller - .connect(keeper) - .deployAccountWithSignature(deployAccountMessage, signature, { maxFeePerGas: 100000000 }), - ) + await expect(controller.connect(keeper).deployAccountWithSignature(deployAccountMessage, signature, TX_OVERRIDES)) .to.emit(controller, 'AccountDeployed') .withArgs(userA.address, accountAddressA) - const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0.001'), DEFAULT_MAX_FEE) + await checkCompensation() }) it('keeper fee is limited by maxFee', async () => { // pre-fund the address where the account will be deployed - await usdc.connect(userA).transfer(accountAddressA, parse6decimal('15000'), { maxFeePerGas: 100000000 }) + await usdc.connect(userA).transfer(accountAddressA, parse6decimal('15000'), TX_OVERRIDES) // sign a message with maxFee smaller than the calculated keeper fee (~0.0033215) - const maxFee = parse6decimal('0.000789') + const maxFee = parse6decimal('0.0789') const deployAccountMessage = { ...createAction(userA.address, userA.address, maxFee), } const signature = await signDeployAccount(userA, accountVerifier, deployAccountMessage) // keeper executes deployment of the account and is compensated - const keeperBalanceBefore = await dsu.balanceOf(keeper.address) - await expect( - controller - .connect(keeper) - .deployAccountWithSignature(deployAccountMessage, signature, { maxFeePerGas: 100000000 }), - ) + await expect(controller.connect(keeper).deployAccountWithSignature(deployAccountMessage, signature, TX_OVERRIDES)) .to.emit(controller, 'AccountDeployed') .withArgs(userA.address, accountAddressA) @@ -308,18 +328,10 @@ describe('Controller_Arbitrum', () => { describe('#transfer', async () => { const INITIAL_DEPOSIT_6 = parse6decimal('13000') let accountA: Account - let keeperBalanceBefore: BigNumber beforeEach(async () => { // deploy collateral account for userA accountA = await createCollateralAccount(userA, INITIAL_DEPOSIT_6) - keeperBalanceBefore = await dsu.balanceOf(keeper.address) - }) - - afterEach(async () => { - // confirm keeper earned their fee - const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0.001'), DEFAULT_MAX_FEE) }) it('collects fee for depositing some funds to market', async () => { @@ -350,6 +362,8 @@ describe('Controller_Arbitrum', () => { .to.emit(controller, 'KeeperCall') .withArgs(keeper.address, anyValue, 0, anyValue, anyValue, anyValue) expect((await market.locals(userA.address)).collateral).to.equal(transferAmount) + + await checkCompensation(1) }) it('collects fee for withdrawing some funds from market', async () => { @@ -384,6 +398,8 @@ describe('Controller_Arbitrum', () => { .to.emit(controller, 'KeeperCall') .withArgs(keeper.address, anyValue, 0, anyValue, anyValue, anyValue) expect((await market.locals(userA.address)).collateral).to.equal(parse6decimal('10000')) // 12k-2k + + await checkCompensation(2) }) it('collects fee for withdrawing native deposit from market', async () => { @@ -429,6 +445,8 @@ describe('Controller_Arbitrum', () => { .to.emit(controller, 'KeeperCall') .withArgs(keeper.address, anyValue, 0, anyValue, anyValue, anyValue) expect((await market.locals(userA.address)).collateral).to.equal(0) + + await checkCompensation(1) }) it('collects fee for withdrawing funds into empty collateral account', async () => { @@ -468,6 +486,8 @@ describe('Controller_Arbitrum', () => { parse6decimal('9999'), parse6decimal('10000'), ) // 12k-2k + + await checkCompensation(2) }) }) @@ -479,14 +499,12 @@ describe('Controller_Arbitrum', () => { }) it('collects fee for changing rebalance configuration', async () => { - const keeperBalanceBefore = await dsu.balanceOf(keeper.address) - // sign message to create a new group const message = { group: 5, markets: [market.address], configs: [{ target: parse6decimal('1'), threshold: parse6decimal('0.0901') }], - maxFee: DEFAULT_MAX_FEE_6, + maxFee: DEFAULT_MAX_FEE, ...(await createAction(userA.address)), } const signature = await signRebalanceConfigChange(userA, accountVerifier, message) @@ -499,8 +517,7 @@ describe('Controller_Arbitrum', () => { .withArgs(userA.address, message.group, 1) // ensure keeper was compensated - const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0.01'), DEFAULT_MAX_FEE) + await checkCompensation() }) it('collects fee for rebalancing a group', async () => { @@ -514,7 +531,7 @@ describe('Controller_Arbitrum', () => { { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') }, { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') }, ], - maxFee: DEFAULT_MAX_FEE_6, + maxFee: DEFAULT_MAX_FEE, ...(await createAction(userA.address)), } const signature = await signRebalanceConfigChange(userA, accountVerifier, message) @@ -526,9 +543,6 @@ describe('Controller_Arbitrum', () => { expect((await ethMarket.locals(userA.address)).collateral).to.equal(parse6decimal('10000')) expect((await btcMarket.locals(userA.address)).collateral).to.equal(0) - // now that test setup is complete, record keeper balance - const keeperBalanceBefore = await dsu.balanceOf(keeper.address) - // rebalance the group await expect(controller.connect(keeper).rebalanceGroup(userA.address, 4, TX_OVERRIDES)) .to.emit(dsu, 'Transfer') @@ -541,8 +555,7 @@ describe('Controller_Arbitrum', () => { .withArgs(keeper.address, anyValue, 0, anyValue, anyValue, anyValue) // confirm keeper earned their fee - const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0.01'), DEFAULT_MAX_FEE) + await checkCompensation(2) }) it('honors max rebalance fee when rebalancing a group', async () => { @@ -563,7 +576,7 @@ describe('Controller_Arbitrum', () => { await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature, TX_OVERRIDES)).to .not.be.reverted - // transfer some collateral to ethMarket and record keeper balance + // transfer some collateral to ethMarket and record keeper balance after account creation await deposit(parse6decimal('5000'), accountA) const keeperBalanceBefore = await dsu.balanceOf(keeper.address) @@ -594,7 +607,7 @@ describe('Controller_Arbitrum', () => { { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') }, { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') }, ], - maxFee: DEFAULT_MAX_FEE_6, + maxFee: DEFAULT_MAX_FEE, ...(await createAction(userA.address)), } const signature = await signRebalanceConfigChange(userA, accountVerifier, message) @@ -649,19 +662,16 @@ describe('Controller_Arbitrum', () => { describe('#withdrawal', async () => { let accountA: Account let userBalanceBefore: BigNumber - let keeperBalanceBefore: BigNumber beforeEach(async () => { // deploy collateral account for userA accountA = await createCollateralAccount(userA, parse6decimal('17000')) userBalanceBefore = await usdc.balanceOf(userA.address) - keeperBalanceBefore = await dsu.balanceOf(keeper.address) }) afterEach(async () => { // confirm keeper earned their fee - const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0.001'), DEFAULT_MAX_FEE) + await checkCompensation(1) }) it('collects fee for partial withdrawal from a delegated signer', async () => { @@ -716,7 +726,6 @@ describe('Controller_Arbitrum', () => { describe('#relay', async () => { let downstreamVerifier: Verifier - let keeperBalanceBefore: BigNumber function createCommon(domain: Address) { return { @@ -734,13 +743,11 @@ describe('Controller_Arbitrum', () => { beforeEach(async () => { await createCollateralAccount(userA, parse6decimal('6')) downstreamVerifier = Verifier__factory.connect(await marketFactory.verifier(), owner) - keeperBalanceBefore = await dsu.balanceOf(keeper.address) }) afterEach(async () => { // confirm keeper earned their fee - const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0'), DEFAULT_MAX_FEE) + await checkCompensation() }) it('relays nonce cancellation messages', async () => { diff --git a/packages/perennial-order/contracts/Manager.sol b/packages/perennial-order/contracts/Manager.sol index d9ca85e63..5c7f9a8c6 100644 --- a/packages/perennial-order/contracts/Manager.sol +++ b/packages/perennial-order/contracts/Manager.sol @@ -38,6 +38,9 @@ abstract contract Manager is IManager, Kept { /// @dev Configuration used for keeper compensation KeepConfig public keepConfig; + /// @dev Configuration used to compensate keepers for price commitments + KeepConfig public keepConfigBuffered; + /// @dev Stores trigger orders while awaiting their conditions to become true /// Market => Account => Nonce => Order mapping(IMarket => mapping(address => mapping(uint256 => TriggerOrderStorage))) private _orders; @@ -62,12 +65,15 @@ abstract contract Manager is IManager, Kept { /// @notice Initialize the contract /// @param ethOracle_ Chainlink ETH/USD oracle used for keeper compensation /// @param keepConfig_ Keeper compensation configuration + /// @param keepConfigBuffered_ Configuration used for price commitments function initialize( AggregatorV3Interface ethOracle_, - KeepConfig memory keepConfig_ + KeepConfig memory keepConfig_, + KeepConfig memory keepConfigBuffered_ ) external initializer(1) { __Kept__initialize(ethOracle_, DSU); keepConfig = keepConfig_; + keepConfigBuffered = keepConfigBuffered_; // allows DSU to unwrap to USDC DSU.approve(address(reserve)); } @@ -78,12 +84,14 @@ abstract contract Manager is IManager, Kept { } /// @inheritdoc IManager - function placeOrderWithSignature(PlaceOrderAction calldata request, bytes calldata signature) external { + function placeOrderWithSignature(PlaceOrderAction calldata request, bytes calldata signature) + external + keepAction(request.action, abi.encode(request, signature)) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyPlaceOrder(request, signature); _ensureValidSigner(request.action.common.account, request.action.common.signer); - _compensateKeeperAction(request.action); _placeOrder(request.action.market, request.action.common.account, request.action.orderId, request.order); } @@ -93,12 +101,14 @@ abstract contract Manager is IManager, Kept { } /// @inheritdoc IManager - function cancelOrderWithSignature(CancelOrderAction calldata request, bytes calldata signature) external { + function cancelOrderWithSignature(CancelOrderAction calldata request, bytes calldata signature) + external + keepAction(request.action, abi.encode(request, signature)) + { // ensure the message was signed by the owner or a delegated signer verifier.verifyCancelOrder(request, signature); _ensureValidSigner(request.action.common.account, request.action.common.signer); - _compensateKeeperAction(request.action); _cancelOrder(request.action.market, request.action.common.account, request.action.orderId); } @@ -120,12 +130,17 @@ abstract contract Manager is IManager, Kept { } /// @inheritdoc IManager - function executeOrder(IMarket market, address account, uint256 orderId) external { + function executeOrder(IMarket market, address account, uint256 orderId) external + { + // Using a modifier to measure gas would require us reading order from storage twice. + // Instead, measure gas within the method itself. + uint256 startGas = gasleft(); + // check conditions to ensure order is executable (TriggerOrder memory order, bool canExecute) = checkOrder(market, account, orderId); + if (!canExecute) revert ManagerCannotExecuteError(); - _compensateKeeper(market, account, order.maxFee); order.execute(market, account); bool interfaceFeeCharged = _chargeInterfaceFee(market, account, order); @@ -135,17 +150,11 @@ abstract contract Manager is IManager, Kept { emit TriggerOrderExecuted(market, account, order, orderId); if (interfaceFeeCharged) emit TriggerOrderInterfaceFeeCharged(account, market, order.interfaceFee); - } - - /// @notice reads keeper compensation parameters from an action message - function _compensateKeeperAction(Action calldata action) internal { - _compensateKeeper(action.market, action.common.account, action.maxFee); - } - /// @notice encodes data needed to pull DSU from market to pay keeper for fulfilling requests - function _compensateKeeper(IMarket market, address account, UFixed6 maxFee) internal { - bytes memory data = abi.encode(market, account, maxFee); - _handleKeeperFee(keepConfig, 0, msg.data[0:0], 0, data); + // compensate keeper + uint256 applicableGas = startGas - gasleft(); + bytes memory data = abi.encode(market, account, order.maxFee); + _handleKeeperFee(keepConfigBuffered, applicableGas, abi.encode(market, account, orderId), 0, data); } /// @notice reverts if user is not authorized to sign transactions for the account @@ -193,7 +202,7 @@ abstract contract Manager is IManager, Kept { _marketWithdraw(market, account, feeAmount); - if (order.interfaceFee.unwrap) _unwrapAndWithdaw(order.interfaceFee.receiver, UFixed18Lib.from(feeAmount)); + if (order.interfaceFee.unwrap) _unwrapAndWithdraw(order.interfaceFee.receiver, UFixed18Lib.from(feeAmount)); else DSU.push(order.interfaceFee.receiver, UFixed18Lib.from(feeAmount)); return true; @@ -216,8 +225,18 @@ abstract contract Manager is IManager, Kept { } /// @notice Unwraps DSU to USDC and pushes to interface fee receiver - function _unwrapAndWithdaw(address receiver, UFixed18 amount) private { + function _unwrapAndWithdraw(address receiver, UFixed18 amount) private { reserve.redeem(amount); USDC.push(receiver, UFixed6Lib.from(amount)); } + + modifier keepAction(Action calldata action, bytes memory applicableCalldata) { + bytes memory data = abi.encode(action.market, action.common.account, action.maxFee); + + uint256 startGas = gasleft(); + _; + uint256 applicableGas = startGas - gasleft(); + + _handleKeeperFee(keepConfig, applicableGas, applicableCalldata, 0, data); + } } diff --git a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts index 988b8eace..0d5eeb9be 100644 --- a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts +++ b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts @@ -46,13 +46,6 @@ const CHAINLINK_ETH_USD_FEED = '0x639Fe6ab55C921f74e7fac1ee960C0B6293ba612' // p const DSU_RESERVE = '0x0d49c416103Cbd276d9c3cd96710dB264e3A0c27' const USDC_ADDRESS = '0xaf88d065e77c8cC2239327C5EDb3A432268e5831' // Arbitrum native USDC, a 6-decimal token -const KEEP_CONFIG = { - multiplierBase: 0, - bufferBase: 1_000_000, - multiplierCalldata: 0, - bufferCalldata: 500_000, -} - const MAX_FEE = utils.parseEther('0.88') const NO_INTERFACE_FEE = { @@ -65,7 +58,7 @@ const NO_INTERFACE_FEE = { } // because we called hardhat_setNextBlockBaseFeePerGas, need this when running tests under coverage -const TX_OVERRIDES = { maxFeePerGas: 150_000_000 } +const TX_OVERRIDES = { maxPriorityFeePerGas: 0, maxFeePerGas: 150_000_000 } describe('Manager_Arbitrum', () => { let dsu: IERC20Metadata @@ -84,9 +77,9 @@ describe('Manager_Arbitrum', () => { let userD: SignerWithAddress let keeper: SignerWithAddress let oracleFeeReceiver: SignerWithAddress - let checkKeeperCompensation: boolean let currentTime: BigNumber let keeperBalanceBefore: BigNumber + let keeperEthBalanceBefore: BigNumber let lastMessageNonce = 0 let lastPriceCommitted: BigNumber const nextOrderId: { [key: string]: BigNumber } = {} @@ -114,7 +107,20 @@ describe('Manager_Arbitrum', () => { marketFactory.address, verifier.address, ) - await manager.initialize(CHAINLINK_ETH_USD_FEED, KEEP_CONFIG) + + const keepConfig = { + multiplierBase: ethers.utils.parseEther('1'), + bufferBase: 1_000_000, // buffer for withdrawing keeper fee from market + multiplierCalldata: 0, + bufferCalldata: 0, + } + const keepConfigBuffered = { + multiplierBase: ethers.utils.parseEther('1.05'), + bufferBase: 1_500_000, // for price commitment + multiplierCalldata: ethers.utils.parseEther('1.05'), + bufferCalldata: 35_200, + } + await manager.initialize(CHAINLINK_ETH_USD_FEED, keepConfig, keepConfigBuffered) // commit a start price await commitPrice(parse6decimal('4444')) @@ -138,6 +144,20 @@ describe('Manager_Arbitrum', () => { await marketFactory.connect(user).updateOperator(manager.address, true) } + async function checkCompensation(priceCommitments = 0) { + const keeperFeesPaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore) + let keeperEthSpentOnGas = keeperEthBalanceBefore.sub(await keeper.getBalance()) + + // If TXes in test required outside price commitments, compensate the keeper for them. + // Note that calls to `commitPrice` in this module do not consume keeper gas. + keeperEthSpentOnGas = keeperEthSpentOnGas.add(utils.parseEther('0.0000644306').mul(priceCommitments)) + + // cost of transaction + const keeperGasCostInUSD = keeperEthSpentOnGas.mul(2603) + // keeper should be compensated between 100-200% of actual gas cost + expect(keeperFeesPaid).to.be.within(keeperGasCostInUSD, keeperGasCostInUSD.mul(2)) + } + // commits an oracle version and advances time 10 seconds async function commitPrice( price = lastPriceCommitted, @@ -354,17 +374,11 @@ describe('Manager_Arbitrum', () => { beforeEach(async () => { await setNextBlockBaseFee() currentTime = BigNumber.from(await currentBlockTimestamp()) - checkKeeperCompensation = false keeperBalanceBefore = await dsu.balanceOf(keeper.address) + keeperEthBalanceBefore = await keeper.getBalance() }) afterEach(async () => { - // ensure keeper was paid for their transaction - if (checkKeeperCompensation) { - const keeperBalanceAfter = await dsu.balanceOf(keeper.address) - const keeperFeePaid = keeperBalanceAfter.sub(keeperBalanceBefore) - expect(keeperFeePaid).to.be.within(utils.parseEther('0.001'), MAX_FEE) - } // ensure manager has no funds at rest expect(await dsu.balanceOf(manager.address)).to.equal(constants.Zero) }) @@ -445,7 +459,7 @@ describe('Manager_Arbitrum', () => { expect(await getPendingPosition(userB, Side.LONG)).to.equal(parse6decimal('2.5')) await market.connect(userA).settle(userA.address, TX_OVERRIDES) - checkKeeperCompensation = true + await checkCompensation(3) }) it('user can place an order using a signed message', async () => { @@ -457,13 +471,12 @@ describe('Manager_Arbitrum', () => { parse6decimal('-10'), ) expect(orderId).to.equal(BigNumber.from(503)) - await executeOrder(userA, 503) + await checkCompensation(0) + await executeOrder(userA, 503) expect(await getPendingPosition(userA, Side.MAKER)).to.equal(parse6decimal('90')) expect(await getPendingPosition(userB, Side.LONG)).to.equal(parse6decimal('2.5')) await commitPrice(parse6decimal('2801')) - - checkKeeperCompensation = true }) it('user can cancel an order', async () => { @@ -498,13 +511,13 @@ describe('Manager_Arbitrum', () => { await expect(manager.connect(keeper).cancelOrderWithSignature(message, signature, TX_OVERRIDES)) .to.emit(manager, 'TriggerOrderCancelled') .withArgs(market.address, userA.address, orderId) + await checkCompensation(0) const storedOrder = await manager.orders(market.address, userA.address, orderId) expect(storedOrder.isSpent).to.be.true expect(await getPendingPosition(userA, Side.MAKER)).to.equal(parse6decimal('90')) expect(await getPendingPosition(userB, Side.LONG)).to.equal(parse6decimal('2.5')) - checkKeeperCompensation = true }) it('non-delegated signer cannot interact', async () => { @@ -555,7 +568,7 @@ describe('Manager_Arbitrum', () => { // order was not executed, so no change in position expect(await getPendingPosition(userA, Side.MAKER)).to.equal(parse6decimal('90')) expect(await getPendingPosition(userB, Side.LONG)).to.equal(parse6decimal('2.5')) - checkKeeperCompensation = true + await checkCompensation(0) }) it('charges flat interface fee upon execution', async () => { @@ -597,7 +610,7 @@ describe('Manager_Arbitrum', () => { // ensure fees were paid expect(await dsu.balanceOf(userC.address)).to.equal(interfaceBalanceBefore.add(feeAmount.mul(1e12))) - checkKeeperCompensation = true + await checkCompensation(1) }) it('unwraps flat interface fee upon execution', async () => { @@ -633,7 +646,7 @@ describe('Manager_Arbitrum', () => { // ensure fees were paid expect(await usdc.balanceOf(userC.address)).to.equal(feeAmount) - checkKeeperCompensation = true + await checkCompensation(1) }) it('unwraps notional interface fee upon execution', async () => { @@ -672,7 +685,7 @@ describe('Manager_Arbitrum', () => { // ensure fees were paid expect(await usdc.balanceOf(userC.address)).to.equal(interfaceBalanceBefore.add(expectedInterfaceFee)) - checkKeeperCompensation = true + await checkCompensation(1) }) it('users can close positions', async () => { @@ -742,6 +755,10 @@ describe('Manager_Arbitrum', () => { nextOrderId[userD.address] = BigNumber.from(600) }) + afterEach(async () => { + await checkCompensation(1) + }) + it('can execute an order with pending position before oracle request fulfilled', async () => { // userB has an unsettled long 1.2 position await changePosition(userB, 0, parse6decimal('1.2'), 0) @@ -765,8 +782,6 @@ describe('Manager_Arbitrum', () => { // settle userB and check position await market.settle(userB.address, TX_OVERRIDES) expect((await market.positions(userB.address)).long).to.equal(parse6decimal('2.0')) - - checkKeeperCompensation = true }) it('can execute an order with pending position after oracle request fulfilled', async () => { @@ -793,8 +808,6 @@ describe('Manager_Arbitrum', () => { // after settling userC, they should be short 2.5 await market.settle(userC.address, TX_OVERRIDES) expect((await market.positions(userC.address)).short).to.equal(parse6decimal('2.5')) - - checkKeeperCompensation = true }) it('can execute an order once market conditions allow', async () => { @@ -849,8 +862,6 @@ describe('Manager_Arbitrum', () => { await market.settle(userD.address, TX_OVERRIDES) expect((await market.positions(userC.address)).short).to.equal(parse6decimal('2.26')) expect((await market.positions(userD.address)).long).to.equal(parse6decimal('3')) - - checkKeeperCompensation = true }) it('market reverts when attempting to close an unsettled position', async () => { @@ -882,8 +893,6 @@ describe('Manager_Arbitrum', () => { await commitPrice(parse6decimal('2000.2'), longOrderTimestamp) await market.settle(userD.address, TX_OVERRIDES) expect((await market.positions(userD.address)).long).to.equal(parse6decimal('4.5')) - - checkKeeperCompensation = true }) it('charges notional interface fee on whole position when closing', async () => { @@ -916,7 +925,6 @@ describe('Manager_Arbitrum', () => { // ensure fees were paid expect(await dsu.balanceOf(userB.address)).to.equal(interfaceBalanceBefore.add(expectedInterfaceFee.mul(1e12))) - checkKeeperCompensation = true }) }) }) diff --git a/packages/perennial-order/test/unit/Manager.test.ts b/packages/perennial-order/test/unit/Manager.test.ts index 587b894e9..fadb943c5 100644 --- a/packages/perennial-order/test/unit/Manager.test.ts +++ b/packages/perennial-order/test/unit/Manager.test.ts @@ -30,9 +30,9 @@ const MAX_FEE = utils.parseEther('3.8') const KEEP_CONFIG = { multiplierBase: 0, - bufferBase: 1_000_000, + bufferBase: 0, multiplierCalldata: 0, - bufferCalldata: 500_000, + bufferCalldata: 0, } const MAKER_ORDER = { @@ -110,7 +110,8 @@ describe('Manager', () => { updatedAt: 0, answeredInRound: 0, }) - await manager.initialize(ethOracle.address, KEEP_CONFIG) + // no need for meaningful keep configs, as keeper compensation is not tested here + await manager.initialize(ethOracle.address, KEEP_CONFIG, KEEP_CONFIG) } before(async () => {