From 7cbf9beed100d9a7a95fd067ae9da3f4ae57591d Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 5 Jul 2023 15:04:23 +0200 Subject: [PATCH] fix(Relayer): Correct deposit lookback (#787) fix(Relayer): Correct deposit lookback The MAX_RELAYER_DEPOSIT_LOOKBACK config item is currently intended to be specified in units of seconds, but was previously configured with a number of blocks. The Relayer class however passed MAX_RELAYER_DEPOSIT_LOOKBACK directly in to the getUnfilledDeposits() helper function, where it is compared directly against a block number. This should to be translated from seconds to a block number first. The effect of this misalignment in units is probably that the relayer looks back further than anticipated on mainnet, where 1 block is ~12 seconds, and potentially less than anticipated on some L2s, where block production might occur multiple times per second. For larger lookback configurations this potentially imposes a hefty time penalty in looping mode when searching for unfilled deposits originating on mainnet. This may resolve an issue that we've received vague reports about in the past from the relayer community. Feedback has been that the relayer is less likely to capture fills if it runs in looping mode, so many from the community choose to run in serverless mode and loop their bot externally. Thanks to James for the quick SDK update that was needed in this PR. --- package.json | 2 +- src/monitor/Monitor.ts | 6 +- src/relayer/Relayer.ts | 2 +- src/utils/BlockUtils.ts | 2 +- src/utils/FillUtils.ts | 48 +++++++---- test/Monitor.ts | 5 ++ test/Relayer.BasicFill.ts | 3 - test/Relayer.SlowFill.ts | 1 - test/Relayer.TokenShortfall.ts | 1 - test/Relayer.UnfilledDeposits.ts | 136 ++++++++++--------------------- yarn.lock | 8 +- 11 files changed, 89 insertions(+), 125 deletions(-) diff --git a/package.json b/package.json index da48f3455..b24932528 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "dependencies": { "@across-protocol/contracts-v2": "2.3.3", "@across-protocol/optimism-sdk": "2.1.3", - "@across-protocol/sdk-v2": "0.10.4", + "@across-protocol/sdk-v2": "0.11.4", "@arbitrum/sdk": "^3.1.3", "@defi-wonderland/smock": "^2.3.5", "@eth-optimism/sdk": "^2.1.0", diff --git a/src/monitor/Monitor.ts b/src/monitor/Monitor.ts index 58764cafa..27773d4c0 100644 --- a/src/monitor/Monitor.ts +++ b/src/monitor/Monitor.ts @@ -181,10 +181,10 @@ export class Monitor { } async reportUnfilledDeposits(): Promise { - const unfilledDeposits = getUnfilledDeposits( + const unfilledDeposits = await getUnfilledDeposits( this.clients.spokePoolClients, - this.monitorConfig.maxRelayerLookBack, - this.clients.configStoreClient + this.clients.configStoreClient, + this.monitorConfig.maxRelayerLookBack ); // Group unfilled amounts by chain id and token id. diff --git a/src/relayer/Relayer.ts b/src/relayer/Relayer.ts index 81a0ab99e..8bbd655d9 100644 --- a/src/relayer/Relayer.ts +++ b/src/relayer/Relayer.ts @@ -42,7 +42,7 @@ export class Relayer { this.clients; const maxVersion = configStoreClient.configStoreVersion; - const unfilledDeposits = getUnfilledDeposits(spokePoolClients, config.maxRelayerLookBack, configStoreClient); + const unfilledDeposits = await getUnfilledDeposits(spokePoolClients, configStoreClient, config.maxRelayerLookBack); const { supportedDeposits = [], unsupportedDeposits = [] } = groupBy(unfilledDeposits, (deposit) => deposit.version <= maxVersion ? "supportedDeposits" : "unsupportedDeposits" ); diff --git a/src/utils/BlockUtils.ts b/src/utils/BlockUtils.ts index b99ccd50f..c0cffa9a9 100644 --- a/src/utils/BlockUtils.ts +++ b/src/utils/BlockUtils.ts @@ -24,7 +24,7 @@ export async function getBlockFinder(chainId: number): Promise { const unfilledDeposits: RelayerUnfilledDeposit[] = []; + const chainIds = Object.values(spokePoolClients).map(({ chainId }) => chainId); + + let earliestBlockNumbers = Object.values(spokePoolClients).map(({ deploymentBlock }) => deploymentBlock); + if (isDefined(depositLookBack)) { + earliestBlockNumbers = await Promise.all( + Object.values(spokePoolClients).map((spokePoolClient) => { + const currentTime = spokePoolClient.getCurrentTime(); + return getBlockForTimestamp(spokePoolClient.chainId, currentTime - depositLookBack); + }) + ); + } + // Iterate over each chainId and check for unfilled deposits. - const chainIds = Object.keys(spokePoolClients); - for (const originChain of chainIds) { - const originClient = spokePoolClients[originChain]; - for (const destinationChain of chainIds) { - if (originChain === destinationChain) { - continue; - } + for (const originClient of Object.values(spokePoolClients)) { + const { chainId: originChainId } = originClient; + const chainIdx = chainIds.indexOf(originChainId); + assert(chainIdx !== -1, `Invalid chain index for chainId ${originChainId} (${chainIdx})`); + const earliestBlockNumber = earliestBlockNumbers[chainIdx]; + + for (const destinationChain of chainIds.filter((chainId) => chainId !== originChainId)) { // Find all unfilled deposits for the current loops originChain -> destinationChain. Note that this also // validates that the deposit is filled "correctly" for the given deposit information. This includes validation // of the all deposit -> relay props, the realizedLpFeePct and the origin->destination token mapping. @@ -254,10 +272,8 @@ export function getUnfilledDeposits( const depositsForDestinationChain: DepositWithBlock[] = originClient.getDepositsForDestinationChain(destinationChain); - // If deposit is older than unfilled deposit lookback, ignore it. - const cutOff = originClient.latestBlockNumber - maxUnfilledDepositLookBack; const unfilledDepositsForDestinationChain = depositsForDestinationChain - .filter((deposit) => deposit.blockNumber >= cutOff) + .filter((deposit) => deposit.blockNumber >= earliestBlockNumber) .map((deposit) => { const version = configStoreClient.getConfigStoreVersionForTimestamp(deposit.quoteTimestamp); return { ...destinationClient.getValidUnfilledAmountForDeposit(deposit), deposit, version }; diff --git a/test/Monitor.ts b/test/Monitor.ts index da480f1ca..f4e846ef3 100644 --- a/test/Monitor.ts +++ b/test/Monitor.ts @@ -86,6 +86,11 @@ describe("Monitor", async function () { }; const monitorConfig = new MonitorConfig(defaultMonitorEnvVars); + // @dev: Force maxRelayerLookBack to undefined to skip lookback block resolution. + // This overrules the readonly property for MonitorConfig.maxRelayerLookBack (...bodge!!). + // @todo: Relocate getUnfilledDeposits() into a class to permit it to be overridden in test. + monitorConfig["maxRelayerLookBack"] = undefined; + // Set the config store version to 0 to match the default version in the ConfigStoreClient. process.env.CONFIG_STORE_VERSION = "0"; diff --git a/test/Relayer.BasicFill.ts b/test/Relayer.BasicFill.ts index 584a89263..e067df4e4 100644 --- a/test/Relayer.BasicFill.ts +++ b/test/Relayer.BasicFill.ts @@ -116,7 +116,6 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () { { relayerTokens: [], relayerDestinationChains: [originChainId, destinationChainId], - maxRelayerLookBack: 24 * 60 * 60, minDepositConfirmations: defaultMinDepositConfirmations, quoteTimeBuffer: 0, } as unknown as RelayerConfig @@ -402,7 +401,6 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () { { relayerTokens: [], relayerDestinationChains: [originChainId], - maxRelayerLookBack: 24 * 60 * 60, minDepositConfirmations: defaultMinDepositConfirmations, quoteTimeBuffer: 0, } as unknown as RelayerConfig @@ -452,7 +450,6 @@ describe("Relayer: Check for Unfilled Deposits and Fill", async function () { { relayerTokens: [], relayerDestinationChains: [originChainId, destinationChainId], - maxRelayerLookBack: 24 * 60 * 60, minDepositConfirmations: defaultMinDepositConfirmations, quoteTimeBuffer: 0, } as unknown as RelayerConfig diff --git a/test/Relayer.SlowFill.ts b/test/Relayer.SlowFill.ts index a4ef9052d..c8b27758b 100644 --- a/test/Relayer.SlowFill.ts +++ b/test/Relayer.SlowFill.ts @@ -93,7 +93,6 @@ describe("Relayer: Zero sized fill for slow relay", async function () { relayerTokens: [], slowDepositors: [], relayerDestinationChains: [], - maxRelayerLookBack: 24 * 60 * 60, quoteTimeBuffer: 0, minDepositConfirmations: defaultMinDepositConfirmations, } as unknown as RelayerConfig diff --git a/test/Relayer.TokenShortfall.ts b/test/Relayer.TokenShortfall.ts index 4807190dd..bf6242434 100644 --- a/test/Relayer.TokenShortfall.ts +++ b/test/Relayer.TokenShortfall.ts @@ -91,7 +91,6 @@ describe("Relayer: Token balance shortfall", async function () { relayerTokens: [], slowDepositors: [], relayerDestinationChains: [], - maxRelayerLookBack: 24 * 60 * 60, quoteTimeBuffer: 0, minDepositConfirmations: defaultMinDepositConfirmations, } as unknown as RelayerConfig diff --git a/test/Relayer.UnfilledDeposits.ts b/test/Relayer.UnfilledDeposits.ts index b9b52b01c..d4568bb28 100644 --- a/test/Relayer.UnfilledDeposits.ts +++ b/test/Relayer.UnfilledDeposits.ts @@ -15,19 +15,13 @@ import { toBNWei, } from "./utils"; import { simpleDeposit, ethers, Contract, SignerWithAddress, setupTokensForWallet } from "./utils"; -import { - amountToLp, - originChainId, - defaultMinDepositConfirmations, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - modifyRelayHelper, -} from "./constants"; +import { amountToLp, originChainId, defaultMinDepositConfirmations, modifyRelayHelper } from "./constants"; import { SpokePoolClient, HubPoolClient, MultiCallerClient, TokenClient, AcrossApiClient } from "../src/clients"; import { MockInventoryClient, MockProfitClient } from "./mocks"; // Tested import { Relayer } from "../src/relayer/Relayer"; -import { getUnfilledDeposits, toBN, UnfilledDeposit, utf8ToHex } from "../src/utils"; +import { getUnfilledDeposits, toBN, RelayerUnfilledDeposit, utf8ToHex } from "../src/utils"; import { RelayerConfig } from "../src/relayer/RelayerConfig"; import { MockConfigStoreClient, MockedMultiCallerClient } from "./mocks"; @@ -43,6 +37,9 @@ let profitClient: MockProfitClient; let spokePool1DeploymentBlock: number, spokePool2DeploymentBlock: number; let relayerInstance: Relayer; +let unfilledDeposits: RelayerUnfilledDeposit[] = []; + +let _getUnfilledDeposits: Promise; describe("Relayer: Unfilled Deposits", async function () { beforeEach(async function () { @@ -106,7 +103,6 @@ describe("Relayer: Unfilled Deposits", async function () { { relayerTokens: [], relayerDestinationChains: [], - maxRelayerLookBack: 24 * 60 * 60, quoteTimeBuffer: 0, minDepositConfirmations: defaultMinDepositConfirmations, acceptInvalidFills: false, @@ -130,6 +126,11 @@ describe("Relayer: Unfilled Deposits", async function () { await spokePool_1.setCurrentTime(await getLastBlockTime(spokePool_1.provider)); await spokePool_2.setCurrentTime(await getLastBlockTime(spokePool_2.provider)); await updateAllClients(); + + _getUnfilledDeposits = async (): Promise => { + return await getUnfilledDeposits(relayerInstance.clients.spokePoolClients, configStoreClient); + }; + unfilledDeposits = []; }); it("Correctly fetches unfilled deposits", async function () { @@ -140,13 +141,8 @@ describe("Relayer: Unfilled Deposits", async function () { const deposit1Complete = await buildDepositStruct(deposit1, hubPoolClient, l1Token); const deposit2Complete = await buildDepositStruct(deposit2, hubPoolClient, l1Token); - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -169,16 +165,6 @@ describe("Relayer: Unfilled Deposits", async function () { it("Correctly defers deposits with future quote timestamps", async function () { const delta = await spokePool_1.depositQuoteTimeBuffer(); // seconds - const _getUnfilledDeposits = () => { - return (unfilledDeposits = getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - )); - }; - - let unfilledDeposits: UnfilledDeposit[] = null; - const deposit1 = await simpleDeposit(spokePool_1, erc20_1, depositor, depositor, destinationChainId); await spokePool_1.setCurrentTime(deposit1.quoteTimestamp + delta); @@ -187,21 +173,24 @@ describe("Relayer: Unfilled Deposits", async function () { // One deposit is eligible. await spokePool_1.setCurrentTime(deposit1.quoteTimestamp); await updateAllClients(); - unfilledDeposits = _getUnfilledDeposits(); + + unfilledDeposits = await _getUnfilledDeposits(); expect(unfilledDeposits.length).to.eq(1); expect(unfilledDeposits[0].deposit.depositId).to.equal(deposit1.depositId); // Still only one deposit. await spokePool_1.setCurrentTime(deposit2.quoteTimestamp - 1); await updateAllClients(); - unfilledDeposits = _getUnfilledDeposits(); + + unfilledDeposits = await _getUnfilledDeposits(); expect(unfilledDeposits.length).to.equal(1); expect(unfilledDeposits[0].deposit.depositId).to.equal(deposit1.depositId); // Step slightly beyond the future quoteTimestamp; now both deposits are eligible. await spokePool_1.setCurrentTime(deposit2.quoteTimestamp); await updateAllClients(); - unfilledDeposits = _getUnfilledDeposits(); + + unfilledDeposits = await _getUnfilledDeposits(); expect(unfilledDeposits.length).to.equal(2); expect(unfilledDeposits[0].deposit.depositId).to.equal(deposit1.depositId); expect(unfilledDeposits[1].deposit.depositId).to.equal(deposit2.depositId); @@ -219,14 +208,10 @@ describe("Relayer: Unfilled Deposits", async function () { const fill1 = await buildFill(spokePool_2, erc20_2, depositor, relayer, deposit1Complete, 0.25); await updateAllClients(); + // Validate the relayer correctly computes the unfilled amount. - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -251,13 +236,9 @@ describe("Relayer: Unfilled Deposits", async function () { await updateAllClients(); // Deposit 1 should now be partially filled by all three fills. This should be correctly reflected. const unfilledAmount = deposit1.amount.sub(fill1.fillAmount.add(fill2.fillAmount).add(fill3.fillAmount)); - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -280,13 +261,9 @@ describe("Relayer: Unfilled Deposits", async function () { const fill4 = await buildFill(spokePool_2, erc20_2, depositor, relayer, deposit1Complete, 1); expect(fill4.totalFilledAmount).to.equal(deposit1.amount); // should be 100% filled at this point. await updateAllClients(); - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -307,14 +284,10 @@ describe("Relayer: Unfilled Deposits", async function () { // Partially fill the deposit, incorrectly by setting the wrong deposit ID. await buildFill(spokePool_2, erc20_2, depositor, relayer, { ...deposit1Complete, depositId: 1337 }, 0.25); await updateAllClients(); + // The deposit should show up as unfilled, since the fill was incorrectly applied to the wrong deposit. - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -329,7 +302,6 @@ describe("Relayer: Unfilled Deposits", async function () { it("Correctly selects unfilled deposit with updated fee", async function () { const delta = await spokePool_1.depositQuoteTimeBuffer(); // seconds - let unfilledDeposits: UnfilledDeposit[]; // perform simple deposit const deposit1 = await simpleDeposit(spokePool_1, erc20_1, depositor, depositor, destinationChainId); @@ -362,12 +334,7 @@ describe("Relayer: Unfilled Deposits", async function () { } await spokePoolClient_1.update(); - unfilledDeposits = getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ); - + unfilledDeposits = await _getUnfilledDeposits(); // expect only one unfilled deposit expect(unfilledDeposits.length).to.eq(1); expect(unfilledDeposits[0].deposit.depositId).to.equal(deposit1.depositId); @@ -380,12 +347,7 @@ describe("Relayer: Unfilled Deposits", async function () { await spokePool_1.setCurrentTime(deposit2.quoteTimestamp); await updateAllClients(); - unfilledDeposits = getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ); - + unfilledDeposits = await _getUnfilledDeposits(); expect(unfilledDeposits.length).to.eq(2); expect(unfilledDeposits[1].deposit.depositId).to.equal(deposit2.depositId); // The new relayer fee was still applied to the early deposit. @@ -398,13 +360,9 @@ describe("Relayer: Unfilled Deposits", async function () { const deposit1Complete = await buildDepositStruct(deposit1, hubPoolClient, l1Token); const fill1 = await buildFill(spokePool_2, erc20_2, depositor, relayer, deposit1Complete, 0.25); await updateAllClients(); - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -442,13 +400,9 @@ describe("Relayer: Unfilled Deposits", async function () { updatedMessage: "0x", speedUpSignature: speedUpSignature.signature, }; - expect( - getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ) - ) + + unfilledDeposits = await _getUnfilledDeposits(); + expect(unfilledDeposits) .excludingEvery(["blockNumber", "quoteBlockNumber", "logIndex", "transactionIndex", "transactionHash"]) .to.deep.equal([ { @@ -477,11 +431,8 @@ describe("Relayer: Unfilled Deposits", async function () { await updateAllClients(); // getUnfilledDeposit still returns the deposit as unfilled but with the invalid fill. - const unfilledDeposit = getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - )[0]; + const unfilledDeposit = (await _getUnfilledDeposits())[0]; + expect(unfilledDeposit === undefined).to.be.false; expect(unfilledDeposit.unfilledAmount).to.equal(deposit.amount); expect(unfilledDeposit.deposit.depositId).to.equal(deposit.depositId); expect(unfilledDeposit.invalidFills.length).to.equal(1); @@ -509,11 +460,8 @@ describe("Relayer: Unfilled Deposits", async function () { await updateAllClients(); await simpleDeposit(spokePool_1, erc20_1, depositor, depositor, destinationChainId); await updateAllClients(); - const unfilledDeposits = getUnfilledDeposits( - relayerInstance.clients.spokePoolClients, - DEFAULT_UNFILLED_DEPOSIT_LOOKBACK, - configStoreClient - ); + + unfilledDeposits = await _getUnfilledDeposits(); expect(unfilledDeposits.length).to.equal(1); expect(unfilledDeposits[0].requiresNewConfigStoreVersion).to.be.true; diff --git a/yarn.lock b/yarn.lock index 609d40b1b..3ac036ba0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -48,10 +48,10 @@ merkletreejs "^0.2.27" rlp "^2.2.7" -"@across-protocol/sdk-v2@0.10.4": - version "0.10.4" - resolved "https://registry.yarnpkg.com/@across-protocol/sdk-v2/-/sdk-v2-0.10.4.tgz#dc4b89f84ff453142bb1a2ad22be3d8ddec59b8a" - integrity sha512-x+jGXKHBGcpJkXYG3W826MIinne5bpkCm/1P7aFDrSs0PRzqAgpSvpmoW2Z968ucJiPGt4mrIDiCLZ55shimXw== +"@across-protocol/sdk-v2@0.11.4": + version "0.11.4" + resolved "https://registry.yarnpkg.com/@across-protocol/sdk-v2/-/sdk-v2-0.11.4.tgz#c28e09d1d3382e8a16f53bed19b61190575c423e" + integrity sha512-IzmOvy+9yc1jILxlm1oTJ6ioHBzibI0+TDQB2UXslumac/BT7LemjfSEraQriIO3klY8DYsNxHQ86R/fidoQoQ== dependencies: "@across-protocol/across-token" "^1.0.0" "@across-protocol/contracts-v2" "2.3.3"