diff --git a/src/clients/MultiCallerClient.ts b/src/clients/MultiCallerClient.ts index 851fbe3cc..93956232b 100644 --- a/src/clients/MultiCallerClient.ts +++ b/src/clients/MultiCallerClient.ts @@ -79,7 +79,7 @@ export interface TryMulticallTransaction { export class MultiCallerClient { protected txnClient: TransactionClient; protected txns: { [chainId: number]: AugmentedTransaction[] } = {}; - protected valueTxns: { [chainId: number]: AugmentedTransaction[] } = {}; + protected nonMulticallTxns: { [chainId: number]: AugmentedTransaction[] } = {}; constructor( readonly logger: winston.Logger, readonly chunkSize: { [chainId: number]: number } = {}, @@ -90,8 +90,8 @@ export class MultiCallerClient { getQueuedTransactions(chainId: number): AugmentedTransaction[] { const allTxns = []; - if (this.valueTxns?.[chainId]) { - allTxns.push(...this.valueTxns[chainId]); + if (this.nonMulticallTxns?.[chainId]) { + allTxns.push(...this.nonMulticallTxns[chainId]); } if (this.txns?.[chainId]) { allTxns.push(...this.txns[chainId]); @@ -101,8 +101,8 @@ export class MultiCallerClient { // Adds all information associated with a transaction to the transaction queue. enqueueTransaction(txn: AugmentedTransaction): void { - // Value transactions are sorted immediately because the UMA multicall implementation rejects them. - const txnQueue = txn.value && txn.value.gt(0) ? this.valueTxns : this.txns; + // We do not attempt to batch together transactions that have value or are explicitly nonMulticall. + const txnQueue = (txn.value && txn.value.gt(0)) || txn.nonMulticall ? this.nonMulticallTxns : this.txns; if (txnQueue[txn.chainId] === undefined) { txnQueue[txn.chainId] = []; } @@ -111,17 +111,17 @@ export class MultiCallerClient { transactionCount(): number { return Object.values(this.txns) - .concat(Object.values(this.valueTxns)) + .concat(Object.values(this.nonMulticallTxns)) .reduce((count, txnQueue) => (count += txnQueue.length), 0); } clearTransactionQueue(chainId: number | null = null): void { if (chainId !== null) { this.txns[chainId] = []; - this.valueTxns[chainId] = []; + this.nonMulticallTxns[chainId] = []; } else { this.txns = {}; - this.valueTxns = {}; + this.nonMulticallTxns = {}; } } @@ -129,7 +129,7 @@ export class MultiCallerClient { async executeTxnQueues(simulate = false, chainIds: number[] = []): Promise> { if (chainIds.length === 0) { chainIds = sdkUtils.dedupArray([ - ...Object.keys(this.valueTxns).map(Number), + ...Object.keys(this.nonMulticallTxns).map(Number), ...Object.keys(this.txns).map(Number), ]); } @@ -174,9 +174,9 @@ export class MultiCallerClient { // For a single chain, take any enqueued transactions and attempt to execute them. async executeTxnQueue(chainId: number, simulate = false): Promise { const txns: AugmentedTransaction[] | undefined = this.txns[chainId]; - const valueTxns: AugmentedTransaction[] | undefined = this.valueTxns[chainId]; + const nonMulticallTxns: AugmentedTransaction[] | undefined = this.nonMulticallTxns[chainId]; this.clearTransactionQueue(chainId); - return this._executeTxnQueue(chainId, txns, valueTxns, simulate); + return this._executeTxnQueue(chainId, txns, nonMulticallTxns, simulate); } // For a single chain, simulate all potential multicall txns and group the ones that pass into multicall bundles. @@ -185,10 +185,10 @@ export class MultiCallerClient { protected async _executeTxnQueue( chainId: number, txns: AugmentedTransaction[] = [], - valueTxns: AugmentedTransaction[] = [], + nonMulticallTxns: AugmentedTransaction[] = [], simulate = false ): Promise { - const nTxns = txns.length + valueTxns.length; + const nTxns = txns.length + nonMulticallTxns.length; if (nTxns === 0) { return []; } @@ -204,7 +204,7 @@ export class MultiCallerClient { // First try to simulate the transaction as a batch. If the full batch succeeded, then we don't // need to simulate transactions individually. If the batch failed, then we need to // simulate the transactions individually and pick out the successful ones. - const batchTxns: AugmentedTransaction[] = valueTxns.concat( + const batchTxns: AugmentedTransaction[] = nonMulticallTxns.concat( await this.buildMultiCallBundles(txns, this.chunkSize[chainId]) ); const batchSimResults = await this.txnClient.simulate(batchTxns); @@ -227,7 +227,7 @@ export class MultiCallerClient { } else { const individualTxnSimResults = await Promise.allSettled([ this.simulateTransactionQueue(txns), - this.simulateTransactionQueue(valueTxns), + this.simulateTransactionQueue(nonMulticallTxns), ]); const [_txns, _valueTxns] = individualTxnSimResults.map((result): AugmentedTransaction[] => { return isPromiseFulfilled(result) ? result.value : []; diff --git a/src/clients/TransactionClient.ts b/src/clients/TransactionClient.ts index 3c8aacc86..acfa06bcf 100644 --- a/src/clients/TransactionClient.ts +++ b/src/clients/TransactionClient.ts @@ -29,6 +29,9 @@ export interface AugmentedTransaction { canFailInSimulation?: boolean; // Optional batch ID to use to group transactions groupId?: string; + // If true, the transaction is being sent to a non Multicall contract so we can't batch it together + // with other transactions. + nonMulticall?: boolean; } const { fixedPointAdjustment: fixedPoint } = sdkUtils; diff --git a/src/dataworker/Dataworker.ts b/src/dataworker/Dataworker.ts index f3db13fdc..3ba94f529 100644 --- a/src/dataworker/Dataworker.ts +++ b/src/dataworker/Dataworker.ts @@ -1690,9 +1690,14 @@ export class Dataworker { value: requiredAmount, }); } else { + // We can't call multicall() here because the feeToken is not guaranteed to be a Multicaller + // contract and this is a permissioned function where the msg.sender needs to be the + // feeToken balance owner, so we can't simply set `unpermissioned: true` to send it through the Multisender. + // Instead, we need to set `nonMulticall: true` and avoid batch calling this transaction. this.clients.multiCallerClient.enqueueTransaction({ contract: new Contract(feeToken, ERC20.abi, signer), chainId: hubPoolChainId, + nonMulticall: true, method: "transfer", args: [holder, requiredAmount], message: `Loaded orbit gas token for message to ${getNetworkName(leaf.chainId)} 📨!`, diff --git a/src/finalizer/index.ts b/src/finalizer/index.ts index 253c00da0..7496f8100 100644 --- a/src/finalizer/index.ts +++ b/src/finalizer/index.ts @@ -330,7 +330,9 @@ export async function finalize( if (finalizations.length > 0) { // @dev use multicaller client to execute batched txn to take advantage of its native txn simulation - // safety features + // safety features. This only works because we assume all finalizer transactions are + // unpermissioned (i.e. msg.sender can be anyone). If this is not true for any chain then we'd need to use + // the TransactionClient. const multicallerClient = new MultiCallerClient(logger); let txnHashLookup: Record = {}; try { diff --git a/test/MultiCallerClient.ts b/test/MultiCallerClient.ts index 1ce18fd1f..e69c11db1 100644 --- a/test/MultiCallerClient.ts +++ b/test/MultiCallerClient.ts @@ -34,8 +34,8 @@ class DummyMultiCallerClient extends MockedMultiCallerClient { return Object.values(txnQueue).reduce((count, txnQueue) => (count += txnQueue.length), 0); } - valueTxnCount(): number { - return this.txnCount(this.valueTxns); + nonMulticallTxnCount(): number { + return this.txnCount(this.nonMulticallTxns); } multiCallTransactionCount(): number { @@ -68,7 +68,15 @@ describe("MultiCallerClient", async function () { it("Correctly enqueues value transactions", async function () { chainIds.forEach((chainId) => multiCaller.enqueueTransaction({ chainId, value: toBN(1) } as AugmentedTransaction)); - expect(multiCaller.valueTxnCount()).to.equal(chainIds.length); + expect(multiCaller.nonMulticallTxnCount()).to.equal(chainIds.length); + expect(multiCaller.transactionCount()).to.equal(chainIds.length); + }); + + it("Correctly enqueues non-multicall transactions", async function () { + chainIds.forEach((chainId) => + multiCaller.enqueueTransaction({ chainId, nonMulticall: true } as AugmentedTransaction) + ); + expect(multiCaller.nonMulticallTxnCount()).to.equal(chainIds.length); expect(multiCaller.transactionCount()).to.equal(chainIds.length); }); @@ -87,10 +95,11 @@ describe("MultiCallerClient", async function () { chainIds.forEach((chainId) => { multiCaller.enqueueTransaction({ chainId } as AugmentedTransaction); multiCaller.enqueueTransaction({ chainId, value: bnOne } as AugmentedTransaction); + multiCaller.enqueueTransaction({ chainId, nonMulticall: true } as AugmentedTransaction); }); - expect(multiCaller.valueTxnCount()).to.equal(chainIds.length); expect(multiCaller.multiCallTransactionCount()).to.equal(chainIds.length); - expect(multiCaller.transactionCount()).to.equal(2 * chainIds.length); + expect(multiCaller.nonMulticallTxnCount()).to.equal(2 * chainIds.length); + expect(multiCaller.transactionCount()).to.equal(3 * chainIds.length); }); it("Propagates input transaction gasLimits: internal multicall", async function () { @@ -271,6 +280,33 @@ describe("MultiCallerClient", async function () { } }); + it("Submits non-multicall txns", async function () { + const nTxns = 3; + for (let txn = 1; txn <= nTxns; ++txn) { + const chainId = chainIds[0]; + const txnRequest: AugmentedTransaction = { + chainId, + contract: { + address, + interface: { encodeFunctionData }, + multicall: 1, + } as unknown as Contract, + method: "test", + args: [{ result: txnClientPassResult }], + nonMulticall: true, + message: `Test nonMulticall transaction (${txn}/${nTxns}) on chain ${chainId}`, + mrkdwn: `Sample markdown string for chain ${chainId} transaction`, + }; + + multiCaller.enqueueTransaction(txnRequest); + } + expect(multiCaller.transactionCount()).to.equal(nTxns); + + // Should have nTxns since non-multicall txns are not batched. + const results = await multiCaller.executeTxnQueues(); + expect(Object.values(results).flat().length).to.equal(nTxns); + }); + it("Correctly filters loggable vs. ignorable simulation failures", async function () { const txn = { chainId: chainIds[0], diff --git a/test/TryMulticallClient.ts b/test/TryMulticallClient.ts index cb18079b0..e44659364 100644 --- a/test/TryMulticallClient.ts +++ b/test/TryMulticallClient.ts @@ -37,8 +37,8 @@ class DummyTryMulticallClient extends TryMulticallClient { return Object.values(txnQueue).reduce((count, txnQueue) => (count += txnQueue.length), 0); } - valueTxnCount(): number { - return this.txnCount(this.valueTxns); + nonMulticallTxnCount(): number { + return this.txnCount(this.nonMulticallTxns); } multiCallTransactionCount(): number {