Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve(inventory-manager): Check ERC20 balance before submitting cross chain bridge #972

Merged
merged 13 commits into from
Oct 9, 2023
Merged
33 changes: 30 additions & 3 deletions src/clients/InventoryClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
DefaultLogLevels,
TransactionResponse,
AnyObject,
ERC20,
} from "../utils";
import { HubPoolClient, TokenClient, BundleDataClient } from ".";
import { AdapterManager, CrossChainTransferClient } from "./bridges";
Expand Down Expand Up @@ -316,9 +317,35 @@ export class InventoryClient {
// the rebalance to this particular chain. Note that if the sum of all rebalances required exceeds the l1
// balance then this logic ensures that we only fill the first n number of chains where we can.
if (amount.lt(balance)) {
possibleRebalances.push(rebalance);
// Decrement token balance in client for this chain and increment cross chain counter.
this.trackCrossChainTransfer(l1Token, amount, chainId);
// As a precautionary step before proceeding, check that the token balance for the token we're about to send
// hasn't changed on L1. It's possible its changed since we updated the inventory due to one or more of the
// RPC's returning slowly, leading to concurrent/overlapping instances of the bot running.
const expectedBalance = this.tokenClient.getBalance(1, l1Token);
const tokenContract = new Contract(l1Token, ERC20.abi, this.hubPoolClient.hubPool.signer);
const currentBalance = await tokenContract.balanceOf(this.relayer);
if (!expectedBalance.eq(currentBalance)) {
this.logger.warn({
at: "InventoryClient",
message:
"🚧 Token balance in relayer on Ethereum changed before sending cross chain transfer, skipping rebalance",
nicholaspai marked this conversation as resolved.
Show resolved Hide resolved
l1Token,
l2ChainId: chainId,
expectedBalance,
currentBalance,
});
continue;
} else {
this.logger.debug({
at: "InventoryClient",
message: "Token balance in relayer on Ethereum is as expected, sending cross chain transfer",
l1Token,
l2ChainId: chainId,
expectedBalance,
});
possibleRebalances.push(rebalance);
// Decrement token balance in client for this chain and increment cross chain counter.
this.trackCrossChainTransfer(l1Token, amount, chainId);
}
} else {
// Extract unexecutable rebalances for logging.
unexecutedRebalances.push(rebalance);
Expand Down
7 changes: 5 additions & 2 deletions src/relayer/RelayerConfig.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check, the skipRelays flag is really a secondary change to permit the InventoryClient to run in a standalone instance, right? I otherwise don't think this change should add any noticeable delay to the existing relayer run (as I read the commit message, it seems to indicate that this change slows the bot down).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(So basically, I think it'd be sufficient to just have the final erc20.balanceOf() check before submitting any rebalance transactions, as a mitigation for the fact that we don't know whether there are concurrent relayer instances running)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check, the skipRelays flag is really a secondary change to permit the InventoryClient to run in a standalone instance, right?

Yes exactly, it should not add any delay to the existing relayer run. The slowdown is moreso related to the ERC20.balanceOf check added in the inventory manager

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export class RelayerConfig extends CommonConfig {
readonly debugProfitability: boolean;
// Whether token price fetch failures will be ignored when computing relay profitability.
// If this is false, the relayer will throw an error when fetching prices fails.
readonly skipRelays: boolean;
readonly sendingRelaysEnabled: boolean;
readonly sendingSlowRelaysEnabled: boolean;
readonly sendingRefundRequestsEnabled: boolean;
Expand Down Expand Up @@ -43,7 +44,8 @@ export class RelayerConfig extends CommonConfig {
RELAYER_GAS_MULTIPLIER,
RELAYER_INVENTORY_CONFIG,
RELAYER_TOKENS,
SEND_RELAYS,
SUBMIT_TRANSACTIONS,
SKIP_RELAYS,
SEND_SLOW_RELAYS,
SEND_REFUND_REQUESTS,
MIN_RELAYER_FEE_PCT,
Expand Down Expand Up @@ -131,7 +133,8 @@ export class RelayerConfig extends CommonConfig {
}
this.debugProfitability = DEBUG_PROFITABILITY === "true";
this.relayerGasMultiplier = toBNWei(RELAYER_GAS_MULTIPLIER || Constants.DEFAULT_RELAYER_GAS_MULTIPLIER);
this.sendingRelaysEnabled = SEND_RELAYS === "true";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think it's worth calling this out in the commit message and I probably think it's better to just do it in a separate commit, since it is a breaking change for existing relayer configs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point reverted here 9818120

this.sendingRelaysEnabled = SUBMIT_TRANSACTIONS === "true";
this.skipRelays = SKIP_RELAYS === "true";
this.sendingRefundRequestsEnabled = SEND_REFUND_REQUESTS !== "false";
this.sendingSlowRelaysEnabled = SEND_SLOW_RELAYS === "true";
this.acceptInvalidFills = ACCEPT_INVALID_FILLS === "true";
Expand Down
18 changes: 10 additions & 8 deletions src/relayer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,18 @@ export async function runRelayer(_logger: winston.Logger, baseSigner: Wallet): P
for (;;) {
await updateRelayerClients(relayerClients, config);

// @note: For fills with a different repaymentChainId, refunds are requested on the _subsequent_ relayer run.
// Refunds requests are enqueued before new fills, so fillRelay simulation occurs closest to txn submission.
const version = configStoreClient.getConfigStoreVersionForTimestamp();
if (sdkUtils.isUBA(version) && version <= configStoreClient.configStoreVersion) {
await relayer.requestRefunds(config.sendingSlowRelaysEnabled);
}
if (!config.skipRelays) {
// @note: For fills with a different repaymentChainId, refunds are requested on the _subsequent_ relayer run.
// Refunds requests are enqueued before new fills, so fillRelay simulation occurs closest to txn submission.
const version = configStoreClient.getConfigStoreVersionForTimestamp();
if (sdkUtils.isUBA(version) && version <= configStoreClient.configStoreVersion) {
await relayer.requestRefunds(config.sendingSlowRelaysEnabled);
}

await relayer.checkForUnfilledDepositsAndFill(config.sendingSlowRelaysEnabled);
await relayer.checkForUnfilledDepositsAndFill(config.sendingSlowRelaysEnabled);

await relayerClients.multiCallerClient.executeTransactionQueue(!config.sendingRelaysEnabled);
await relayerClients.multiCallerClient.executeTransactionQueue(!config.sendingRelaysEnabled);
}

// Unwrap WETH after filling deposits so we don't mess up slow fill logic, but before rebalancing
// any tokens so rebalancing can take into account unwrapped WETH balances.
Expand Down
43 changes: 43 additions & 0 deletions test/InventoryClient.InventoryRebalance.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
BigNumber,
FakeContract,
SignerWithAddress,
createSpyLogger,
deployConfigStore,
Expand All @@ -9,6 +10,7 @@ import {
lastSpyLogIncludes,
randomAddress,
sinon,
smock,
spyLogIncludes,
toBN,
toWei,
Expand All @@ -19,6 +21,7 @@ import { ConfigStoreClient, InventoryClient } from "../src/clients"; // Tested
import { CrossChainTransferClient } from "../src/clients/bridges";
import { InventoryConfig } from "../src/interfaces";
import { MockAdapterManager, MockBundleDataClient, MockHubPoolClient, MockTokenClient } from "./mocks/";
import { ERC20 } from "../src/utils";

const toMegaWei = (num: string | number | BigNumber) => ethers.utils.parseUnits(num.toString(), 6);

Expand All @@ -33,6 +36,9 @@ const enabledChainIds = [1, 10, 137, 42161];
const mainnetWeth = "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2";
const mainnetUsdc = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";

let mainnetWethContract: FakeContract;
let mainnetUsdcContract: FakeContract;

// construct two mappings of chainId to token address. Set the l1 token address to the "real" token address.
const l2TokensForWeth = { 1: mainnetWeth };
const l2TokensForUsdc = { 1: mainnetUsdc };
Expand Down Expand Up @@ -103,6 +109,12 @@ describe("InventoryClient: Rebalancing inventory", async function () {
crossChainTransferClient
);

mainnetWethContract = await smock.fake(ERC20.abi, { address: mainnetWeth });
mainnetUsdcContract = await smock.fake(ERC20.abi, { address: mainnetUsdc });

mainnetWethContract.balanceOf.whenCalledWith(owner.address).returns(initialAllocation[1][mainnetWeth]);
mainnetUsdcContract.balanceOf.whenCalledWith(owner.address).returns(initialAllocation[1][mainnetUsdc]);

seedMocks(initialAllocation);
});

Expand Down Expand Up @@ -257,6 +269,37 @@ describe("InventoryClient: Rebalancing inventory", async function () {
// expect(spyLogIncludes(spy, -2, `"42161":{"actualBalanceOnChain":"945.00"`)).to.be.true;
// expect(spyLogIncludes(spy, -2, `"proRataShare":"7.00%"`)).to.be.true;
});

it("Refuses to send rebalance when ERC20 balance changes", async function () {
await inventoryClient.update();
await inventoryClient.rebalanceInventoryIfNeeded();

// Now, simulate the re-allocation of funds. Say that the USDC on arbitrum is half used up. This will leave arbitrum
// with 500 USDC, giving a percentage of 500/14000 = 0.035. This is below the threshold of 0.5 so we should see
// a re-balance executed in size of the target allocation + overshoot percentage.
const initialBalance = initialAllocation[42161][mainnetUsdc];
expect(tokenClient.getBalance(42161, l2TokensForUsdc[42161])).to.equal(initialBalance);
const withdrawAmount = toMegaWei(500);
tokenClient.decrementLocalBalance(42161, l2TokensForUsdc[42161], withdrawAmount);
expect(tokenClient.getBalance(42161, l2TokensForUsdc[42161])).to.equal(withdrawAmount);

// The allocation of this should now be below the threshold of 5% so the inventory client should instruct a rebalance.
const expectedAlloc = withdrawAmount.mul(toWei(1)).div(initialUsdcTotal.sub(withdrawAmount));
expect(inventoryClient.getCurrentAllocationPct(mainnetUsdc, 42161)).to.equal(expectedAlloc);

// Set USDC balance to be lower than expected.
mainnetUsdcContract.balanceOf
.whenCalledWith(owner.address)
.returns(initialAllocation[1][mainnetUsdc].sub(toMegaWei(1)));
await inventoryClient.rebalanceInventoryIfNeeded();
expect(spyLogIncludes(spy, -2, "balance in relayer on Ethereum changed before sending cross chain transfer")).to.be
.true;

// Reset and check again.
mainnetUsdcContract.balanceOf.whenCalledWith(owner.address).returns(initialAllocation[1][mainnetUsdc]);
await inventoryClient.rebalanceInventoryIfNeeded();
expect(lastSpyLogIncludes(spy, "Executed Inventory rebalances")).to.be.true;
});
});

function seedMocks(seedBalances: { [chainId: string]: { [token: string]: BigNumber } }) {
Expand Down