From bd22184b299e24dd2fbbafa6c54a4b30dd0c404d Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Fri, 16 Aug 2024 17:17:28 +0100 Subject: [PATCH 1/5] fix(common): decode errors during writeContract --- packages/common/src/writeContract.ts | 37 +++++++++++++++++++--------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/packages/common/src/writeContract.ts b/packages/common/src/writeContract.ts index 676ee4ad09..b65ed9d541 100644 --- a/packages/common/src/writeContract.ts +++ b/packages/common/src/writeContract.ts @@ -11,6 +11,8 @@ import { PublicClient, encodeFunctionData, EncodeFunctionDataParameters, + getContractError, + BaseError, } from "viem"; import { prepareTransactionRequest as viem_prepareTransactionRequest, @@ -123,20 +125,33 @@ export async function writeContract< () => pRetry( async () => { - if (!nonceManager.hasNonce()) { - await nonceManager.resetNonce(); - } + try { + if (!nonceManager.hasNonce()) { + await nonceManager.resetNonce(); + } - // We estimate gas before increasing the local nonce to prevent nonce gaps. - // Invalid transactions fail the gas estimation step are never submitted - // to the network, so they should not increase the nonce. - const preparedRequest = await prepare(); + // We estimate gas before increasing the local nonce to prevent nonce gaps. + // Invalid transactions fail the gas estimation step are never submitted + // to the network, so they should not increase the nonce. + const preparedRequest = await prepare(); - const nonce = nonceManager.nextNonce(); + const nonce = nonceManager.nextNonce(); - const fullRequest = { ...preparedRequest, nonce, ...feeRef.fees }; - debug("calling", fullRequest.functionName, "with nonce", nonce, "at", fullRequest.address); - return await viem_writeContract(client, fullRequest as never); + const fullRequest = { ...preparedRequest, nonce, ...feeRef.fees }; + debug("calling", fullRequest.functionName, "with nonce", nonce, "at", fullRequest.address); + + return await getAction(client, viem_writeContract, "writeContract")(fullRequest as never); + } catch (error) { + // TODO: remove once viem handles this (https://github.com/wevm/viem/pull/2624) + throw getContractError(error as BaseError, { + abi: request.abi as Abi, + address: request.address, + args: request.args, + docsPath: "/docs/contract/writeContract", + functionName: request.functionName, + sender: account?.address, + }); + } }, { retries: 3, From 07255fbf17bb9e086945c0c56275867cbffe16f5 Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Fri, 16 Aug 2024 09:45:01 -0700 Subject: [PATCH 2/5] Create three-flies-fly.md --- .changeset/three-flies-fly.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/three-flies-fly.md diff --git a/.changeset/three-flies-fly.md b/.changeset/three-flies-fly.md new file mode 100644 index 0000000000..ee98e22706 --- /dev/null +++ b/.changeset/three-flies-fly.md @@ -0,0 +1,5 @@ +--- +"@latticexyz/common": patch +--- + +Improved error decoding in `writeContract` action for reverts with custom errors. From 1673b6d99c291bfd946fbb234a15e2e4bc70257d Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 20 Aug 2024 15:06:33 +0100 Subject: [PATCH 3/5] rework writeContract/sendTransaction with nonce manager --- packages/common/src/createNonceManager.ts | 23 +++--- packages/common/src/getNonceManager.ts | 12 +-- packages/common/src/sendTransaction.ts | 39 ++-------- packages/common/src/writeContract.ts | 93 +++-------------------- 4 files changed, 40 insertions(+), 127 deletions(-) diff --git a/packages/common/src/createNonceManager.ts b/packages/common/src/createNonceManager.ts index 87806ec578..3c48eb72eb 100644 --- a/packages/common/src/createNonceManager.ts +++ b/packages/common/src/createNonceManager.ts @@ -30,7 +30,7 @@ export function createNonceManager({ broadcastChannelName, queueConcurrency = 1, }: CreateNonceManagerOptions): CreateNonceManagerResult { - const nonceRef = { current: -1 }; + const ref = { nonce: -1, noncePromise: null as Promise | null }; let channel: BroadcastChannel | null = null; if (typeof BroadcastChannel !== "undefined") { @@ -43,32 +43,35 @@ export function createNonceManager({ channel.addEventListener("message", (event) => { const nonce = JSON.parse(event.data); debug("got nonce from broadcast channel", nonce); - nonceRef.current = nonce; + ref.nonce = nonce; }); }); } function hasNonce(): boolean { - return nonceRef.current >= 0; + return ref.nonce >= 0; } function getNonce(): number { if (!hasNonce()) throw new Error("call resetNonce before using getNonce"); - return nonceRef.current; + return ref.nonce; } function nextNonce(): number { if (!hasNonce()) throw new Error("call resetNonce before using nextNonce"); - const nonce = nonceRef.current++; - channel?.postMessage(JSON.stringify(nonceRef.current)); + const nonce = ref.nonce++; + channel?.postMessage(JSON.stringify(ref.nonce)); return nonce; } async function resetNonce(): Promise { - const nonce = await getTransactionCount(client, { address, blockTag }); - nonceRef.current = nonce; - channel?.postMessage(JSON.stringify(nonceRef.current)); - debug("reset nonce to", nonceRef.current); + ref.noncePromise ??= (async (): Promise => { + ref.nonce = await getTransactionCount(client, { address, blockTag }); + ref.noncePromise = null; + channel?.postMessage(JSON.stringify(ref.nonce)); + debug("reset nonce to", ref.nonce); + })(); + await ref.noncePromise; } function shouldResetNonce(error: unknown): boolean { diff --git a/packages/common/src/getNonceManager.ts b/packages/common/src/getNonceManager.ts index 1209894863..f006f53e57 100644 --- a/packages/common/src/getNonceManager.ts +++ b/packages/common/src/getNonceManager.ts @@ -11,12 +11,14 @@ export async function getNonceManager({ }: CreateNonceManagerOptions): Promise { const id = await getNonceManagerId({ client, address, blockTag }); - const existingNonceManager = nonceManagers.get(id); - if (existingNonceManager) { - return existingNonceManager; + const nonceManager = nonceManagers.get(id) ?? createNonceManager({ client, address, blockTag, ...opts }); + if (!nonceManagers.has(id)) { + nonceManagers.set(id, nonceManager); + } + + if (!nonceManager.hasNonce()) { + await nonceManager.resetNonce(); } - const nonceManager = createNonceManager({ client, address, blockTag, ...opts }); - nonceManagers.set(id, nonceManager); return nonceManager; } diff --git a/packages/common/src/sendTransaction.ts b/packages/common/src/sendTransaction.ts index 9d323015fe..ad511a2405 100644 --- a/packages/common/src/sendTransaction.ts +++ b/packages/common/src/sendTransaction.ts @@ -1,6 +1,5 @@ import { Account, - CallParameters, Chain, Client, SendTransactionParameters, @@ -8,7 +7,7 @@ import { SendTransactionReturnType, PublicClient, } from "viem"; -import { call, sendTransaction as viem_sendTransaction } from "viem/actions"; +import { sendTransaction as viem_sendTransaction } from "viem/actions"; import pRetry from "p-retry"; import { debug as parentDebug } from "./debug"; import { getNonceManager } from "./getNonceManager"; @@ -65,51 +64,29 @@ export async function sendTransaction< args: { chain }, }); - async function prepare(): Promise> { - if (request.gas) { - debug("gas provided, skipping simulate", request.to); - return request; - } - - debug("simulating tx to", request.to); - await call(opts.publicClient ?? client, { - ...request, - blockTag: "pending", - account, - } as CallParameters); - - return request; - } - return await nonceManager.mempoolQueue.add( () => pRetry( async () => { - const preparedRequest = await prepare(); - - if (!nonceManager.hasNonce()) { - await nonceManager.resetNonce(); - } - const nonce = nonceManager.nextNonce(); - debug("sending tx with nonce", nonce, "to", preparedRequest.to); - const parameters: SendTransactionParameters = { - ...preparedRequest, + const params: SendTransactionParameters = { + ...request, nonce, ...feeRef.fees, }; - return await viem_sendTransaction(client, parameters); + debug("sending tx to", request.to, "with nonce", nonce); + return await viem_sendTransaction(client, params); }, { retries: 3, onFailedAttempt: async (error) => { - // On nonce errors, reset the nonce and retry + await nonceManager.resetNonce(); + // retry nonce errors + // TODO: upgrade p-retry and move this to shouldRetry if (nonceManager.shouldResetNonce(error)) { debug("got nonce error, retrying", error.message); - await nonceManager.resetNonce(); return; } - // TODO: prepare again if there are gas errors? throw error; }, }, diff --git a/packages/common/src/writeContract.ts b/packages/common/src/writeContract.ts index b65ed9d541..8c0d8f8c49 100644 --- a/packages/common/src/writeContract.ts +++ b/packages/common/src/writeContract.ts @@ -9,15 +9,8 @@ import { ContractFunctionName, ContractFunctionArgs, PublicClient, - encodeFunctionData, - EncodeFunctionDataParameters, - getContractError, - BaseError, } from "viem"; -import { - prepareTransactionRequest as viem_prepareTransactionRequest, - writeContract as viem_writeContract, -} from "viem/actions"; +import { writeContract as viem_writeContract } from "viem/actions"; import pRetry from "p-retry"; import { debug as parentDebug } from "./debug"; import { getNonceManager } from "./getNonceManager"; @@ -65,11 +58,6 @@ export async function writeContract< const account = parseAccount(rawAccount); const chain = client.chain; - const defaultParameters = { - chain, - ...(chain?.fees ? { type: "eip1559" } : {}), - } satisfies Omit; - const nonceManager = await getNonceManager({ client: opts.publicClient ?? client, address: account.address, @@ -83,86 +71,29 @@ export async function writeContract< args: { chain }, }); - async function prepare(): Promise> { - if (request.gas) { - debug("gas provided, skipping preparation", request.functionName, request.address); - return request; - } - - const { abi, address, args, dataSuffix, functionName } = request; - const data = encodeFunctionData({ - abi, - args, - functionName, - } as EncodeFunctionDataParameters); - - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { nonce, maxFeePerGas, maxPriorityFeePerGas, ...preparedTransaction } = await getAction( - client, - viem_prepareTransactionRequest, - "prepareTransactionRequest", - )({ - // The fee values don't need to be accurate for gas estimation - // and we can save a couple rpc calls by providing stubs here. - // These are later overridden with accurate values from `feeRef`. - maxFeePerGas: 0n, - maxPriorityFeePerGas: 0n, - // Send the current nonce without increasing the stored value - nonce: nonceManager.getNonce(), - ...defaultParameters, - ...request, - blockTag: "pending", - account, - // From `viem/writeContract` - data: `${data}${dataSuffix ? dataSuffix.replace("0x", "") : ""}`, - to: address, - } as never); - - return preparedTransaction as never; - } - return nonceManager.mempoolQueue.add( () => pRetry( async () => { - try { - if (!nonceManager.hasNonce()) { - await nonceManager.resetNonce(); - } - - // We estimate gas before increasing the local nonce to prevent nonce gaps. - // Invalid transactions fail the gas estimation step are never submitted - // to the network, so they should not increase the nonce. - const preparedRequest = await prepare(); - - const nonce = nonceManager.nextNonce(); - - const fullRequest = { ...preparedRequest, nonce, ...feeRef.fees }; - debug("calling", fullRequest.functionName, "with nonce", nonce, "at", fullRequest.address); - - return await getAction(client, viem_writeContract, "writeContract")(fullRequest as never); - } catch (error) { - // TODO: remove once viem handles this (https://github.com/wevm/viem/pull/2624) - throw getContractError(error as BaseError, { - abi: request.abi as Abi, - address: request.address, - args: request.args, - docsPath: "/docs/contract/writeContract", - functionName: request.functionName, - sender: account?.address, - }); - } + const nonce = nonceManager.nextNonce(); + const params: WriteContractParameters = { + ...request, + nonce, + ...feeRef.fees, + }; + debug("calling", params.functionName, "at", params.address, "with nonce", nonce); + return await getAction(client, viem_writeContract, "writeContract")(params); }, { retries: 3, onFailedAttempt: async (error) => { - // On nonce errors, reset the nonce and retry + await nonceManager.resetNonce(); + // retry nonce errors + // TODO: upgrade p-retry and move this to shouldRetry if (nonceManager.shouldResetNonce(error)) { debug("got nonce error, retrying", error.message); - await nonceManager.resetNonce(); return; } - // TODO: prepareWrite again if there are gas errors? throw error; }, }, From 5facde39c7ce3978600fe58b02a3b0da10c106bd Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 20 Aug 2024 15:17:31 +0100 Subject: [PATCH 4/5] add log to clarify why nonce gets reset --- packages/common/src/sendTransaction.ts | 2 ++ packages/common/src/writeContract.ts | 2 ++ 2 files changed, 4 insertions(+) diff --git a/packages/common/src/sendTransaction.ts b/packages/common/src/sendTransaction.ts index ad511a2405..349a68cd1a 100644 --- a/packages/common/src/sendTransaction.ts +++ b/packages/common/src/sendTransaction.ts @@ -80,6 +80,8 @@ export async function sendTransaction< { retries: 3, onFailedAttempt: async (error) => { + // in case this tx failed before hitting the mempool (i.e. gas estimation error), reset nonce so we don't skip past the unused nonce + debug("failed, resetting nonce"); await nonceManager.resetNonce(); // retry nonce errors // TODO: upgrade p-retry and move this to shouldRetry diff --git a/packages/common/src/writeContract.ts b/packages/common/src/writeContract.ts index 8c0d8f8c49..4dd2e2d283 100644 --- a/packages/common/src/writeContract.ts +++ b/packages/common/src/writeContract.ts @@ -87,6 +87,8 @@ export async function writeContract< { retries: 3, onFailedAttempt: async (error) => { + // in case this tx failed before hitting the mempool (i.e. gas estimation error), reset nonce so we don't skip past the unused nonce + debug("failed, resetting nonce"); await nonceManager.resetNonce(); // retry nonce errors // TODO: upgrade p-retry and move this to shouldRetry From 5483ba3842821bb145866c3e684590fae4369901 Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 20 Aug 2024 07:27:17 -0700 Subject: [PATCH 5/5] Update three-flies-fly.md --- .changeset/three-flies-fly.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/three-flies-fly.md b/.changeset/three-flies-fly.md index ee98e22706..043634fae0 100644 --- a/.changeset/three-flies-fly.md +++ b/.changeset/three-flies-fly.md @@ -2,4 +2,4 @@ "@latticexyz/common": patch --- -Improved error decoding in `writeContract` action for reverts with custom errors. +Refactored `writeContract` and `sendTransaction` actions for simplicity and better error messages.