From d439c9a88463fb2ff071c8809fea78842e2b8f31 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 13 May 2024 13:07:07 -0400 Subject: [PATCH 1/6] WIP: improve(InventoryClient): Align allocation percentage computations between Rebalancer and Relayer As part of a larger inventory client cleanup/refactor I noticed that there is possibly a bug in how we compute "allocation" percentages when (1) determining repayment chain for fills and (2) computing possible rebalances. For (2), we subtract the "token shortfall" from the numerator but in (2) we subtract it from both the numerator and the denominator. This is inconsistent and I don't think it makes sense. A shortfall represents a lack of tokens on a chain suggesting that we need that amount of tokens to fill a deposit. The rebalancer (i.e. function (2) above) it designed to rebalance inventory from L1 to L2 to fulfill these shortfalls. Therefore, it makes sense to me that in (2) the shortfall is subtracted from the numerator to compute the allocation percentage on the L2. The cumulative balance technically is the total inventory across all chains. This should intuitively include all outstanding transfers between L1 and L2 because this is still part of the inventory, its just in-flight between chains. A shortfall will lead to an outstanding transfer but it will also lead to a subtraction in the total inventory once the transfer finalizes and part of that new L2 balance is used to make a fill. However, if the shortfall is subtracted from the denominator in the rebalancer/(2) function, then the amount of tokens bridged over to the L2 will be insufficient to fulfill the shortfall. This PR makes the allocation % computation consistent between (1) and (2) by removing the shortfall subtraction from the denominator in (2). --- src/clients/InventoryClient.ts | 14 +++++++------- test/InventoryClient.RefundChain.ts | 21 +++++++++------------ 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/clients/InventoryClient.ts b/src/clients/InventoryClient.ts index 9539188ce..b6857e015 100644 --- a/src/clients/InventoryClient.ts +++ b/src/clients/InventoryClient.ts @@ -473,7 +473,6 @@ export class InventoryClient { const chainShortfall = this.tokenClient.getShortfallTotalRequirement(_chain, repaymentToken); const chainVirtualBalance = this.getBalanceOnChain(_chain, l1Token, repaymentToken); const chainVirtualBalanceWithShortfall = chainVirtualBalance.sub(chainShortfall); - let cumulativeVirtualBalanceWithShortfall = cumulativeVirtualBalance.sub(chainShortfall); // @dev No need to factor in outputAmount when computing origin chain balance since funds only leave relayer // on destination chain // @dev Do not subtract outputAmount from virtual balance if output token and input token are not equivalent. @@ -491,14 +490,13 @@ export class InventoryClient { ); // To correctly compute the allocation % for this destination chain, we need to add all upcoming refunds for the // equivalents of l1Token on all chains. - cumulativeVirtualBalanceWithShortfall = cumulativeVirtualBalanceWithShortfall.add(cumulativeRefunds); - const cumulativeVirtualBalanceWithShortfallPostRelay = cumulativeVirtualBalanceWithShortfall.sub(outputAmount); + const cumulativeVirtualBalancePostRelay = cumulativeVirtualBalance.add(cumulativeRefunds).sub(outputAmount); // Compute what the balance will be on the target chain, considering this relay and the finalization of the // transfers that are currently flowing through the canonical bridge. const expectedPostRelayAllocation = chainVirtualBalanceWithShortfallPostRelay .mul(this.scalar) - .div(cumulativeVirtualBalanceWithShortfallPostRelay); + .div(cumulativeVirtualBalancePostRelay); // Consider configured buffer for target to allow relayer to support slight overages. const tokenConfig = this.getTokenConfig(l1Token, _chain, repaymentToken); @@ -522,8 +520,7 @@ export class InventoryClient { chainVirtualBalanceWithShortfall, chainVirtualBalanceWithShortfallPostRelay, cumulativeVirtualBalance, - cumulativeVirtualBalanceWithShortfall, - cumulativeVirtualBalanceWithShortfallPostRelay, + cumulativeVirtualBalancePostRelay, thresholdPct, expectedPostRelayAllocation, chainsToEvaluate, @@ -864,7 +861,10 @@ export class InventoryClient { this.crossChainTransferClient .getOutstandingCrossChainTransferAmount(this.relayer, chainId, l1Token, l2Token) .toString() - )}.\n`; + )}.` + + ` This chain has a shortfall of ${formatter( + this.tokenClient.getShortfallTotalRequirement(chainId, l2Token).toString() + )} ${symbol}.\n`; } } diff --git a/test/InventoryClient.RefundChain.ts b/test/InventoryClient.RefundChain.ts index cf6b3000a..bd1fbcc17 100644 --- a/test/InventoryClient.RefundChain.ts +++ b/test/InventoryClient.RefundChain.ts @@ -202,11 +202,10 @@ describe("InventoryClient: Refund chain selection", async function () { // 3. chainVirtualBalanceWithShortfallPostRelay: virtual balance with shortfall minus the relay amount. 9.8-1.69=8.11. // 4. cumulativeVirtualBalance: total balance across all chains considering fund movement. funds moving over the bridge // does not impact the balance; they are just "moving" so it should be 140-15+15=140 - // 5. cumulativeVirtualBalanceWithShortfall: cumulative virtual balance minus the shortfall. 140-15+15=140-15=125. - // 6. cumulativeVirtualBalanceWithShortfallPostRelay: cumulative virtual balance with shortfall minus the relay amount - // 125-1.69=124.31. This is total funds considering the shortfall and the relay amount that is to be executed. - // 7. expectedPostRelayAllocation: the expected post relay allocation is the chainVirtualBalanceWithShortfallPostRelay - // divided by the cumulativeVirtualBalanceWithShortfallPostRelay. 8.11/123.31 = 0.0657. + // 5. cumulativeVirtualBalancePostRelay: cumulative virtual balance plus upcoming refunds minus the relay amount + // 140-1.69=138.31. This is total funds considering the relay amount that is to be executed. + // 6. expectedPostRelayAllocation: the expected post relay allocation is the chainVirtualBalanceWithShortfallPostRelay + // divided by the cumulativeVirtualBalancePostRelay. 8.11/138.31 = 0.0586. // This number is then used to decide on where funds should be allocated! If this number is above the threshold plus // the buffer then refund on L1. if it is below the threshold then refund on the target chain. As this number is // is below the buffer plus the threshold then the bot should refund on L2. @@ -222,20 +221,18 @@ describe("InventoryClient: Refund chain selection", async function () { expect(lastSpyLogIncludes(spy, 'chainVirtualBalanceWithShortfall":"9800000000000000000"')).to.be.true; // 24.8-15=9.8 expect(lastSpyLogIncludes(spy, 'chainVirtualBalanceWithShortfallPostRelay":"8110000000000000000"')).to.be.true; // 9.8-1.69=8.11 expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalance":"140000000000000000000')).to.be.true; // 140-15+15=140 - expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalanceWithShortfall":"125000000000000000000"')).to.be.true; // 140-15=125 - expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalanceWithShortfallPostRelay":"123310000000000000000"')).to.be - .true; // 125-1.69=123.31 - expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"65769199578298597')).to.be.true; // 8.11/123.31 = 0.0657 + expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalancePostRelay":"138310000000000000000"')).to.be.true; // 125-1.69=123.31 + expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"58636396500614561')).to.be.true; // 8.11/123.31 = 0.0657 // Now consider if this small relay was larger to the point that we should be refunding on the L2. set it to 5 WETH. // Numerically we can shortcut some of the computations above to the following: chain virtual balance with shortfall - // post relay is 9.8 - 5 = 4.8. cumulative virtual balance with shortfall post relay is 125 - 5 = 120. Expected post - // relay allocation is 4.8/120 = 0.04. This is below the threshold of 0.05 so the bot should refund on the target. + // post relay is 9.8 - 5 = 4.8. cumulative virtual balance post relay is 140 - 5 = 135. Expected post + // relay allocation is 4.8/135 = 0.036. This is below the threshold of 0.05 so the bot should refund on the target. sampleDepositData.inputAmount = toWei(5); sampleDepositData.outputAmount = await computeOutputAmount(sampleDepositData); expect(await inventoryClient.determineRefundChainId(sampleDepositData)).to.equal(ARBITRUM); // Check only the final step in the computation. - expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"40000000000000000"')).to.be.true; // 4.8/120 = 0.04 + expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"35555555555555555"')).to.be.true; // 4.8/120 = 0.04 // Consider that we manually send the relayer som funds while it's large transfer is currently in the bridge. This // is to validate that the module considers funds in transit correctly + dropping funds indirectly onto the L2 wallet. From 02f77b35c162bb248a19aa8af3adc6ac57898f72 Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Sun, 18 Aug 2024 19:03:42 -0400 Subject: [PATCH 2/6] Update InventoryClient.ts --- src/clients/InventoryClient.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/clients/InventoryClient.ts b/src/clients/InventoryClient.ts index b75d108c9..1f81bcf87 100644 --- a/src/clients/InventoryClient.ts +++ b/src/clients/InventoryClient.ts @@ -505,8 +505,6 @@ export class InventoryClient { const chainShortfall = this.tokenClient.getShortfallTotalRequirement(chainId, repaymentToken); const chainVirtualBalance = this.getBalanceOnChain(chainId, l1Token, repaymentToken); const chainVirtualBalanceWithShortfall = chainVirtualBalance.sub(chainShortfall); - // @dev No need to factor in outputAmount when computing origin chain balance since funds only leave relayer - // on destination chain // @dev Do not subtract outputAmount from virtual balance if output token and input token are not equivalent. // This is possible when the output token is USDC.e and the input token is USDC which would still cause // validateOutputToken() to return true above. @@ -522,7 +520,7 @@ export class InventoryClient { ); // To correctly compute the allocation % for this destination chain, we need to add all upcoming refunds for the // equivalents of l1Token on all chains. - const cumulativeVirtualBalancePostRelay = cumulativeVirtualBalance.add(cumulativeRefunds).sub(outputAmount); + const cumulativeVirtualBalancePostRelay = cumulativeVirtualBalance.add(cumulativeRefunds); // Compute what the balance will be on the target chain, considering this relay and the finalization of the // transfers that are currently flowing through the canonical bridge. From c8dfbf0bc37fb1f9c954ffd10e956f59190e6309 Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Sun, 18 Aug 2024 19:04:59 -0400 Subject: [PATCH 3/6] Update InventoryClient.ts --- src/clients/InventoryClient.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/clients/InventoryClient.ts b/src/clients/InventoryClient.ts index 1f81bcf87..edb4f202d 100644 --- a/src/clients/InventoryClient.ts +++ b/src/clients/InventoryClient.ts @@ -520,13 +520,13 @@ export class InventoryClient { ); // To correctly compute the allocation % for this destination chain, we need to add all upcoming refunds for the // equivalents of l1Token on all chains. - const cumulativeVirtualBalancePostRelay = cumulativeVirtualBalance.add(cumulativeRefunds); + const cumulativeVirtualBalancePostRefunds = cumulativeVirtualBalance.add(cumulativeRefunds); // Compute what the balance will be on the target chain, considering this relay and the finalization of the // transfers that are currently flowing through the canonical bridge. const expectedPostRelayAllocation = chainVirtualBalanceWithShortfallPostRelay .mul(this.scalar) - .div(cumulativeVirtualBalancePostRelay); + .div(cumulativeVirtualBalancePostRefunds); // Consider configured buffer for target to allow relayer to support slight overages. const tokenConfig = this.getTokenConfig(l1Token, chainId, repaymentToken); @@ -564,11 +564,10 @@ export class InventoryClient { chainShortfall, chainVirtualBalance, chainVirtualBalanceWithShortfall, - chainVirtualBalanceWithShortfallPostRelay, + cumulativeVirtualBalancePostRefunds, cumulativeVirtualBalance, cumulativeVirtualBalancePostRelay, cumulativeVirtualBalanceWithShortfallPostRefunds, - thresholdPct, targetPct: ethersUtils.formatUnits(tokenConfig.targetPct, 18), targetOverage: ethersUtils.formatUnits(targetOverageBuffer, 18), effectiveTargetPct: ethersUtils.formatUnits(effectiveTargetPct, 18), From 71c1d0ff43cb4a8044683bc20142a94fe45b0806 Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Sun, 18 Aug 2024 19:06:13 -0400 Subject: [PATCH 4/6] Update InventoryClient.ts --- src/clients/InventoryClient.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/clients/InventoryClient.ts b/src/clients/InventoryClient.ts index edb4f202d..0192f3b68 100644 --- a/src/clients/InventoryClient.ts +++ b/src/clients/InventoryClient.ts @@ -564,10 +564,9 @@ export class InventoryClient { chainShortfall, chainVirtualBalance, chainVirtualBalanceWithShortfall, - cumulativeVirtualBalancePostRefunds, + chainVirtualBalanceWithShortfallPostRelay, cumulativeVirtualBalance, - cumulativeVirtualBalancePostRelay, - cumulativeVirtualBalanceWithShortfallPostRefunds, + cumulativeVirtualBalancePostRefunds, targetPct: ethersUtils.formatUnits(tokenConfig.targetPct, 18), targetOverage: ethersUtils.formatUnits(targetOverageBuffer, 18), effectiveTargetPct: ethersUtils.formatUnits(effectiveTargetPct, 18), From 5f69ee112b007e1f35993753390dc4b6d6643208 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Sun, 18 Aug 2024 19:20:27 -0400 Subject: [PATCH 5/6] Update InventoryClient.RefundChain.ts --- test/InventoryClient.RefundChain.ts | 38 +++++++++-------------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/test/InventoryClient.RefundChain.ts b/test/InventoryClient.RefundChain.ts index 3a2942409..c0411625e 100644 --- a/test/InventoryClient.RefundChain.ts +++ b/test/InventoryClient.RefundChain.ts @@ -205,10 +205,9 @@ describe("InventoryClient: Refund chain selection", async function () { // same as above for destination chain. // 4. cumulativeVirtualBalance: total balance across all chains considering fund movement. funds moving over the bridge // does not impact the balance; they are just "moving" so it should be 140-15+15=140 - // 5. cumulativeVirtualBalancePostRelay: cumulative virtual balance plus upcoming refunds minus the relay amount - // 140-1.69=138.31. This is total funds considering the relay amount that is to be executed. + // 5. cumulativeVirtualBalancePostRefunds: should be same as above because there are no refunds. // 6. expectedPostRelayAllocation: the expected post relay allocation is the chainVirtualBalanceWithShortfallPostRelay - // divided by the cumulativeVirtualBalancePostRelay. 8.11/138.31 = 0.0586. + // divided by the cumulativeVirtualBalancePostRefunds. 9.8/140 // This number is then used to decide on where funds should be allocated! If this number is above the threshold plus // the buffer then refund on L1. if it is below the threshold then refund on the target chain. @@ -216,7 +215,7 @@ describe("InventoryClient: Refund chain selection", async function () { sampleDepositData.outputToken = l2TokensForWeth[ARBITRUM]; sampleDepositData.inputAmount = toWei(1.69); sampleDepositData.outputAmount = toWei(1.69); - expect(await inventoryClient.determineRefundChainId(sampleDepositData)).to.deep.equal([MAINNET]); + expect(await inventoryClient.determineRefundChainId(sampleDepositData)).to.deep.equal([ARBITRUM, MAINNET]); // We're evaluating destination chain since origin chain is mainnet which always gets evaluated last. expect(lastSpyLogIncludes(spy, 'chainShortfall":"15000000000000000000"')).to.be.true; @@ -225,32 +224,17 @@ describe("InventoryClient: Refund chain selection", async function () { expect(lastSpyLogIncludes(spy, 'chainVirtualBalanceWithShortfallPostRelay":"9800000000000000000"')).to.be.true; // same as above for destination chain expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalance":"140000000000000000000')).to.be.true; // 140-15+15=140 - expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalancePostRelay":"138310000000000000000"')).to.be.true; // 125-1.69=123.31 - expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"58636396500614561')).to.be.true; // 8.11/123.31 = 0.0657 + expect(lastSpyLogIncludes(spy, 'cumulativeVirtualBalancePostRefunds":"140000000000000000000"')).to.be.true; + expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"70000000000000000')).to.be.true; // 9.8 / 140 - // Now consider if this small relay was larger to the point that we should be refunding on the L2. set it to 5 WETH. - // Numerically we can shortcut some of the computations above to the following: chain virtual balance with shortfall - // post relay is 9.8 - 5 = 4.8. cumulative virtual balance post relay is 140 - 5 = 135. Expected post - // relay allocation is 4.8/135 = 0.036. This is below the threshold of 0.05 so the bot should refund on the target. - sampleDepositData.inputAmount = toWei(5); - sampleDepositData.outputAmount = await computeOutputAmount(sampleDepositData); - expect(await inventoryClient.determineRefundChainId(sampleDepositData)).to.deep.equal([ARBITRUM, MAINNET]); - // Check only the final step in the computation. - expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"35555555555555555"')).to.be.true; // 4.8/120 = 0.04 - - // Consider that we manually send the relayer som funds while it's large transfer is currently in the bridge. This - // is to validate that the module considers funds in transit correctly + dropping funds indirectly onto the L2 wallet. - // Say we magically give the bot 10 WETH on Arbitrum and try to repeat the previous example. Now, we should a - // chain virtual balance with shortfall post relay is 9.8 - 5 + 10 = 14.8. cumulative virtual balance with shortfall - // post relay is 125 - 5 + 10 = 130. Expected post relay allocation is 14.8/130 = 0.11. This is above the threshold - // of 0.05 so the bot should refund on L1. + // Consider that we decrease the relayer's balance while it's large transfer is currently in the bridge. tokenClient.setTokenData( ARBITRUM, l2TokensForWeth[ARBITRUM], initialAllocation[ARBITRUM][mainnetWeth].sub(toWei(5)) ); expect(await inventoryClient.determineRefundChainId(sampleDepositData)).to.deep.equal([ARBITRUM, MAINNET]); - expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"40000000000000000')).to.be.true; // 4.8/120 + expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"35555555555555555')).to.be.true; // 4.8/(140-5) }); it("Correctly decides where to refund based on upcoming refunds", async function () { @@ -427,14 +411,14 @@ describe("InventoryClient: Refund chain selection", async function () { tokenClient.setTokenData(POLYGON, l2TokensForWeth[POLYGON], toWei(5)); tokenClient.setTokenData(OPTIMISM, l2TokensForWeth[OPTIMISM], toWei(25)); - // Shortfalls are subtracted from both numerator and denominator. + // Shortfalls are subtracted from just numerator tokenClient.setTokenShortFallData(POLYGON, l2TokensForWeth[POLYGON], [6969], toWei(5)); // Mock the shortfall. // Post relay allocations: - // Optimism (destination chain): (25-5)/(140-5) > 12% - // Polygon (origin chain): (5-5+1)/(140-5) < 7% + // Optimism (destination chain): (25-5)/(140) > 12% + // Polygon (origin chain): (5-5+1)/(140) < 7% // Relayer should still use origin chain expect(await inventoryClient.determineRefundChainId(sampleDepositData)).to.deep.equal([POLYGON, MAINNET]); - expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"7407407407407407"')).to.be.true; + expect(lastSpyLogIncludes(spy, 'expectedPostRelayAllocation":"7142857142857142"')).to.be.true; }); it("Origin allocation depends on upcoming refunds", async function () { // Set Polygon allocation lower than target: From bc78c74a68e3e825f5f25be54ce0cb1b71c2f897 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Thu, 2 Jan 2025 10:05:03 -0500 Subject: [PATCH 6/6] Update InventoryClient.ts --- src/clients/InventoryClient.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/clients/InventoryClient.ts b/src/clients/InventoryClient.ts index 24cbfb86e..b5452973c 100644 --- a/src/clients/InventoryClient.ts +++ b/src/clients/InventoryClient.ts @@ -578,8 +578,7 @@ export class InventoryClient { chainVirtualBalanceWithShortfall, chainVirtualBalanceWithShortfallPostRelay, cumulativeVirtualBalance, - cumulativeVirtualBalanceWithShortfall, - cumulativeVirtualBalanceWithShortfallPostRefunds, + cumulativeVirtualBalancePostRefunds, targetPct: formatUnits(tokenConfig.targetPct, 18), targetOverage: formatUnits(targetOverageBuffer, 18), effectiveTargetPct: formatUnits(effectiveTargetPct, 18),