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

feat(common): add sendTransaction, add mempool queue to nonce manager #1717

Merged
merged 8 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/few-brooms-accept.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@latticexyz/common": minor
---

- Added a `sendTransaction` helper to mirror viem's `sendTransaction`, but with our nonce manager
- Added an internal mempool queue to `sendTransaction` and `writeContract` for better nonce handling
- Defaults block tag to `pending` for transaction simulation and transaction count (when initializing the nonce manager)
1 change: 1 addition & 0 deletions packages/common/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"chalk": "^5.2.0",
"debug": "^4.3.4",
"execa": "^7.0.0",
"p-queue": "^7.4.1",
"p-retry": "^5.1.2",
"prettier": "^2.8.4",
"prettier-plugin-solidity": "^1.1.2",
Expand Down
9 changes: 7 additions & 2 deletions packages/common/src/createNonceManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { BaseError, BlockTag, Client, Hex, NonceTooHighError, NonceTooLowError }
import { debug as parentDebug } from "./debug";
import { getNonceManagerId } from "./getNonceManagerId";
import { getTransactionCount } from "viem/actions";
import PQueue from "p-queue";

const debug = parentDebug.extend("createNonceManager");

Expand All @@ -17,12 +18,13 @@ export type CreateNonceManagerResult = {
nextNonce: () => number;
resetNonce: () => Promise<void>;
shouldResetNonce: (error: unknown) => boolean;
mempoolQueue: PQueue;
};

export function createNonceManager({
client,
address,
blockTag = "latest",
address, // TODO: rename to account?
Copy link
Member Author

Choose a reason for hiding this comment

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

didn't want to introduce a breaking change here so leaving this as a TODO

blockTag = "pending",
broadcastChannelName,
}: CreateNonceManagerOptions): CreateNonceManagerResult {
const nonceRef = { current: -1 };
Expand Down Expand Up @@ -68,10 +70,13 @@ export function createNonceManager({
);
}

const mempoolQueue = new PQueue({ concurrency: 1 });
Copy link
Member Author

Choose a reason for hiding this comment

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

My initial prototype of the nonce manager had this but later removed it because it didn't seem to help much, but once I started working on #1702 and aligned the block tag for 1) getting initial nonce value and 2) simulating txs, I found it much more stable to have a queue in front of the mempool.

Once you start sending lots of parallel txs (like we do in deploys), it's important that they land in the mempool in nonce order, and you can't really guarantee this unless you queue them like this. Submitting to mempool is fairly quick in most cases, but is slower than what we had before. #1702 does a better job at parallelizing overall, so it should be noticeably faster to deploy, even though we're technically waiting longer for submitting to the mempool.

Copy link
Member Author

Choose a reason for hiding this comment

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

and yes, we could make the concurrency configurable, but I think it might be worth waiting until we have tests and can really stress test the different options to decide if it's worth allowing this to be parallelized


return {
hasNonce,
nextNonce,
resetNonce,
shouldResetNonce,
mempoolQueue,
};
}
22 changes: 18 additions & 4 deletions packages/common/src/getContract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ import {
Chain,
GetContractParameters,
GetContractReturnType,
Hex,
PublicClient,
Transport,
WalletClient,
WriteContractParameters,
getContract as viem_getContract,
} from "viem";
import { UnionOmit } from "./type-utils/common";
import { WriteContractOptions, writeContract } from "./writeContract";
import { writeContract } from "./writeContract";

// copied from viem because this isn't exported
// TODO: import from viem?
Expand All @@ -26,6 +27,12 @@ function getFunctionParameters(values: [args?: readonly unknown[], options?: obj
return { args, options };
}

export type ContractWrite = {
id: string;
request: WriteContractParameters;
result: Promise<Hex>;
};

export type GetContractOptions<
TTransport extends Transport,
TAddress extends Address,
Expand All @@ -35,7 +42,7 @@ export type GetContractOptions<
TPublicClient extends PublicClient<TTransport, TChain>,
TWalletClient extends WalletClient<TTransport, TChain, TAccount>
> = Required<GetContractParameters<TTransport, TChain, TAccount, TAbi, TPublicClient, TWalletClient, TAddress>> & {
onWrite?: WriteContractOptions["onWrite"];
onWrite?: (write: ContractWrite) => void;
};

// TODO: migrate away from this approach once we can hook into viem: https://github.com/wagmi-dev/viem/discussions/1230
Expand Down Expand Up @@ -72,6 +79,7 @@ export function getContract<

if (contract.write) {
// Replace write calls with our own. Implemented ~the same as viem, but adds better handling of nonces (via queue + retries).
let nextWriteId = 0;
contract.write = new Proxy(
{},
{
Expand All @@ -83,14 +91,20 @@ export function getContract<
]
) => {
const { args, options } = getFunctionParameters(parameters);
return writeContract(walletClient, {
const request = {
abi,
address,
functionName,
args,
...options,
onWrite,
} as unknown as WriteContractOptions<TAbi, typeof functionName, TChain, TAccount>);
} as unknown as WriteContractParameters<TAbi, typeof functionName, TChain, TAccount>;
const result = writeContract(walletClient, request);

const id = `${walletClient.chain.id}:${walletClient.account.address}:${nextWriteId++}`;
onWrite?.({ id, request: request as WriteContractParameters, result });

return result;
};
},
}
Expand Down
4 changes: 2 additions & 2 deletions packages/common/src/getNonceManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ const nonceManagers = new Map<string, CreateNonceManagerResult>();

export async function getNonceManager({
client,
address,
blockTag = "latest",
address, // TODO: rename to account?
blockTag = "pending",
}: CreateNonceManagerOptions): Promise<CreateNonceManagerResult> {
const id = await getNonceManagerId({ client, address, blockTag });

Expand Down
1 change: 1 addition & 0 deletions packages/common/src/getNonceManagerId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export async function getNonceManagerId({
address: Hex;
blockTag: BlockTag;
}): Promise<string> {
// TODO: improve this so we don't have to call getChainId every time
Copy link
Member Author

Choose a reason for hiding this comment

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

right now, this is going to make an eth_chainId call per use of writeContract and sendTransaction

we can improve this by threading through a chain or chain ID from the upstream callers, but it's unclear what the "best practice" is in terms of viem client handling of chains (should they be mutable or not) and what to do for wallets that do have "mutable" chains (e.g. a Metamask wallet can switch network, but unclear if that should create a new client object or if it should mutate the chain on the existing client object)

once I get clarity, I'll follow up and optimize this to remove the extra call

const chainId = client.chain?.id ?? (await getChainId(client));
return `mud:createNonceManager:${chainId}:${getAddress(address)}:${blockTag}`;
}
1 change: 1 addition & 0 deletions packages/common/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export * from "./hexToResource";
export * from "./readHex";
export * from "./resourceToHex";
export * from "./resourceTypes";
export * from "./sendTransaction";
export * from "./spliceHex";
export * from "./transportObserver";
export * from "./writeContract";
Expand Down
89 changes: 89 additions & 0 deletions packages/common/src/sendTransaction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import {
Account,
CallParameters,
Chain,
Client,
SendTransactionParameters,
Transport,
WriteContractReturnType,
} from "viem";
import { call, sendTransaction as viem_sendTransaction } from "viem/actions";
import pRetry from "p-retry";
import { debug as parentDebug } from "./debug";
import { getNonceManager } from "./getNonceManager";
import { parseAccount } from "viem/accounts";

const debug = parentDebug.extend("sendTransaction");

// TODO: migrate away from this approach once we can hook into viem's nonce management: https://github.com/wagmi-dev/viem/discussions/1230

export async function sendTransaction<
TChain extends Chain | undefined,
TAccount extends Account | undefined,
TChainOverride extends Chain | undefined
>(
client: Client<Transport, TChain, TAccount>,
request: SendTransactionParameters<TChain, TAccount, TChainOverride>
): Promise<WriteContractReturnType> {
const rawAccount = request.account ?? client.account;
if (!rawAccount) {
// TODO: replace with viem AccountNotFoundError once its exported
throw new Error("No account provided");
}
const account = parseAccount(rawAccount);

const nonceManager = await getNonceManager({
client,
address: account.address,
blockTag: "pending",
});

async function prepare(): Promise<SendTransactionParameters<TChain, TAccount, TChainOverride>> {
if (request.gas) {
debug("gas provided, skipping simulate", request.to);
return request;
}

debug("simulating tx to", request.to);
await call(client, {
...request,
blockTag: "pending",
account,
} as CallParameters<TChain>);

// TODO: estimate gas
Comment on lines +47 to +54
Copy link
Member

Choose a reason for hiding this comment

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

is there more to do here than to use estimateGas instead of call?

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh so I thought viem internally did both a call/simulate + estimate gas, but looking closer at sendTransaction and prepareTransactionRequest, it looks like it's just a gas estimate (likely because we don't need the simulated return value in this context)

will see about updating, but may need to also have to add gas retry handling if there's too much time between calling this function and the tx getting sent to the mem pool (due to queue and/or nonce retries)

Copy link
Member Author

@holic holic Oct 9, 2023

Choose a reason for hiding this comment

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

for context: I was initially just doing the simulate, not the gas estimate, because I wanted to know if it was safe to increment the nonce but still allow for the gas to be computed just before sending the tx

but since gas estimate is ~the same RPC cost as simulate and we can attempt to send the tx with potentially outdated gas and save a call, this seems worth it (assuming we have proper retry handling for bad gas)

Copy link
Member Author

@holic holic Oct 9, 2023

Choose a reason for hiding this comment

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

looks like viem does use the return data of the simulate call for better error messages in case of failures/reverts seems like both estimate gas and call revert with the same data, so I am now thinking there's something wrong with how errors are being parsed

Copy link
Member Author

Choose a reason for hiding this comment

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

I attempted to do this, along with consolidating behavior, but ran into issues with being able to parse errors from viem. I've already burned a few hours on this, so I'd like to pause and come back to this another time in a follow-up: 97fd6d7

Copy link
Member

Choose a reason for hiding this comment

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

sounds good. Do you think it's a viem bug or something on our side? If the former, is there a repro we could share with them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't able to confirm if it was a viem issue or not. Using their StackBlitz starter kit didn't seem to surface it. But it definitely happens within the MUD repo, even though everything is pinned to exactly the same version (tried clearing all node modules and reinstalling and also tried bumping to a different exactly version, with no change in behavior I was seeing).


return request;
}

const preparedRequest = await prepare();

return await nonceManager.mempoolQueue.add(
() =>
pRetry(
async () => {
if (!nonceManager.hasNonce()) {
await nonceManager.resetNonce();
}

const nonce = nonceManager.nextNonce();
debug("sending tx with nonce", nonce, "to", preparedRequest.to);
return await viem_sendTransaction(client, { nonce, ...preparedRequest });
},
{
retries: 3,
onFailedAttempt: async (error) => {
// On nonce errors, reset the nonce and retry
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;
},
}
),
{ throwOnTimeout: true }
);
Comment on lines +61 to +88
Copy link
Member

Choose a reason for hiding this comment

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

for context / my understanding: with mempoolQueue concurrency set to 1 we wait for the transaction to be accepted by the rpc node before attempting to send the next transaction, but we're not waiting for the transaction to be included in a block, so it's still possible to include multiple transactions in one block, correct? While before we were "preparing" to see if the node would accept the transaction, but we weren't actually waiting for it to accept it before sending another one, so in some edge case it could have happened that we're sending two transactions with subsequent nonces, the first one gets dropped for some reason, and the second one is stuck?

Is this correct or am I missing something in my understanding?

Copy link
Member Author

Choose a reason for hiding this comment

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

with mempoolQueue concurrency set to 1 we wait for the transaction to be accepted by the rpc node before attempting to send the next transaction, but we're not waiting for the transaction to be included in a block, so it's still possible to include multiple transactions in one block, correct?

correct

While before we were "preparing" to see if the node would accept the transaction, but we weren't actually waiting for it to accept it before sending another one, so in some edge case it could have happened that we're sending two transactions with subsequent nonces, the first one gets dropped for some reason, and the second one is stuck?

correct! or, what I think I was seeing is that when parallelizing lots of txs, the node would receive the set of txs out of nonce order, so some would just fail/not be accepted by the node

}
96 changes: 34 additions & 62 deletions packages/common/src/writeContract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
Account,
Chain,
Client,
Hex,
SimulateContractParameters,
Transport,
WriteContractParameters,
Expand All @@ -16,29 +15,6 @@ import { getNonceManager } from "./getNonceManager";
import { parseAccount } from "viem/accounts";

const debug = parentDebug.extend("writeContract");
let nextWriteId = 0;

export type ContractWrite<
TAbi extends Abi | readonly unknown[] = Abi,
TFunctionName extends string = string,
TChain extends Chain | undefined = Chain,
TAccount extends Account | undefined = Account | undefined,
TChainOverride extends Chain | undefined = Chain | undefined
> = {
id: string;
request: WriteContractParameters<TAbi, TFunctionName, TChain, TAccount, TChainOverride>;
result: Promise<Hex>;
};

export type WriteContractOptions<
TAbi extends Abi | readonly unknown[] = Abi,
TFunctionName extends string = string,
TChain extends Chain | undefined = Chain,
TAccount extends Account | undefined = Account | undefined,
TChainOverride extends Chain | undefined = Chain | undefined
> = WriteContractParameters<TAbi, TFunctionName, TChain, TAccount, TChainOverride> & {
onWrite?: (write: ContractWrite<TAbi, TFunctionName, TChain, TAccount, TChainOverride>) => void;
};

// TODO: migrate away from this approach once we can hook into viem's nonce management: https://github.com/wagmi-dev/viem/discussions/1230

Expand All @@ -50,71 +26,67 @@ export async function writeContract<
TChainOverride extends Chain | undefined
>(
client: Client<Transport, TChain, TAccount>,
{ onWrite, ...request_ }: WriteContractOptions<TAbi, TFunctionName, TChain, TAccount, TChainOverride>
request: WriteContractParameters<TAbi, TFunctionName, TChain, TAccount, TChainOverride>
): Promise<WriteContractReturnType> {
const request = request_ as WriteContractParameters<TAbi, TFunctionName, TChain, TAccount, TChainOverride>;

const account_ = request.account ?? client.account;
if (!account_) {
const rawAccount = request.account ?? client.account;
if (!rawAccount) {
// TODO: replace with viem AccountNotFoundError once its exported
throw new Error("No account provided");
}
const account = parseAccount(account_);
const account = parseAccount(rawAccount);

const nonceManager = await getNonceManager({
client,
address: account.address,
blockTag: "pending",
});

async function prepareWrite(): Promise<
WriteContractParameters<TAbi, TFunctionName, TChain, TAccount, TChainOverride>
> {
if (request.gas) {
debug("gas provided, skipping simulate", request);
debug("gas provided, skipping simulate", request.functionName, request.address);
return request;
}

debug("simulating write", request);
debug("simulating", request.functionName, "at", request.address);
const result = await simulateContract<TChain, TAbi, TFunctionName, TChainOverride>(client, {
...request,
blockTag: "pending",
account,
} as unknown as SimulateContractParameters<TAbi, TFunctionName, TChain, TChainOverride>);

return result.request as unknown as WriteContractParameters<TAbi, TFunctionName, TChain, TAccount, TChainOverride>;
}

async function write(): Promise<Hex> {
const preparedWrite = await prepareWrite();
const preparedWrite = await prepareWrite();

return await pRetry(
async () => {
if (!nonceManager.hasNonce()) {
await nonceManager.resetNonce();
}

const nonce = nonceManager.nextNonce();
debug("calling write function with nonce", nonce, preparedWrite);
return await viem_writeContract(client, { nonce, ...preparedWrite } as typeof preparedWrite);
},
{
retries: 3,
onFailedAttempt: async (error) => {
// On nonce errors, reset the nonce and retry
if (nonceManager.shouldResetNonce(error)) {
debug("got nonce error, retrying", error);
return nonceManager.mempoolQueue.add(
() =>
pRetry(
async () => {
if (!nonceManager.hasNonce()) {
await nonceManager.resetNonce();
return;
}
// TODO: prepareWrite again if there are gas errors?
throw error;
},
}
);
}

const result = write();

onWrite?.({ id: `${nextWriteId++}`, request, result });

return result;
const nonce = nonceManager.nextNonce();
debug("calling", preparedWrite.functionName, "with nonce", nonce, "at", preparedWrite.address);
return await viem_writeContract(client, { nonce, ...preparedWrite } as typeof preparedWrite);
},
{
retries: 3,
onFailedAttempt: async (error) => {
// On nonce errors, reset the nonce and retry
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;
},
}
),
{ throwOnTimeout: true }
);
}
Loading