Skip to content

fix: retry-failed creates undefined nonce value #766

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.

Sign up for GitHub

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

Merged
arcoraven merged 4 commits into from
Nov 8, 2024
Merged

fix: retry-failed creates undefined nonce value #766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41674fc
fix: retry-failed creates undefined nonce value
arcoraven Nov 8, 2024
6729fd0
Merge remote-tracking branch 'origin/main' into ph/fixRetryTransactio…
arcoraven Nov 8, 2024
7bfeb0c
Merge remote-tracking branch 'origin/main' into ph/fixRetryTransactio…
arcoraven Nov 8, 2024
57f9205
type import
arcoraven Nov 8, 2024
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
33 changes: 21 additions & 12 deletions src/server/middleware/error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,17 @@ const isZodError = (err: unknown): boolean => {

export const withErrorHandler = async (server: FastifyInstance) => {
server.setErrorHandler(
(error: Error | CustomError | ZodError, request, reply) => {
(error: string | Error | CustomError | ZodError, request, reply) => {
if (typeof error === "string") {
Copy link
Contributor Author

@arcoraven arcoraven Nov 8, 2024

Choose a reason for hiding this comment

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

Currently a thrown string is not logged.
While we shouldn't throw raw strings, this update at least prints the string value.

return reply.status(StatusCodes.INTERNAL_SERVER_ERROR).send({
error: {
statusCode: 500,
code: "INTERNAL_SERVER_ERROR",
message: error || ReasonPhrases.INTERNAL_SERVER_ERROR,
},
});
}

// Ethers Error Codes
if (parseEthersError(error)) {
return reply.status(StatusCodes.BAD_REQUEST).send({
Expand Down Expand Up @@ -103,25 +113,24 @@ export const withErrorHandler = async (server: FastifyInstance) => {
StatusCodes.INTERNAL_SERVER_ERROR;

const message = error.message ?? ReasonPhrases.INTERNAL_SERVER_ERROR;
reply.status(statusCode).send({
return reply.status(statusCode).send({
error: {
code,
message,
statusCode,
stack: env.NODE_ENV !== "production" ? error.stack : undefined,
},
});
} else {
// Handle non-custom errors
reply.status(StatusCodes.INTERNAL_SERVER_ERROR).send({
error: {
statusCode: 500,
code: "INTERNAL_SERVER_ERROR",
message: error.message || ReasonPhrases.INTERNAL_SERVER_ERROR,
stack: env.NODE_ENV !== "production" ? error.stack : undefined,
},
});
}

reply.status(StatusCodes.INTERNAL_SERVER_ERROR).send({
error: {
statusCode: 500,
code: "INTERNAL_SERVER_ERROR",
message: error.message || ReasonPhrases.INTERNAL_SERVER_ERROR,
stack: env.NODE_ENV !== "production" ? error.stack : undefined,
},
});
},
);
};
3 changes: 1 addition & 2 deletions src/server/routes/transaction/retry-failed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,9 @@ export async function retryFailedTransaction(fastify: FastifyInstance) {
);
}

// temp do not handle userop
if (transaction.isUserOp) {
throw createCustomError(
`Transaction cannot be retried because it is a userop`,
"Transaction cannot be retried because it is a userop",
StatusCodes.BAD_REQUEST,
"TRANSACTION_CANNOT_BE_RETRIED",
);
Expand Down
36 changes: 20 additions & 16 deletions src/worker/tasks/sendTransactionWorker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,15 @@ const handler: Processor<string, void, string> = async (job: Job<string>) => {
// For example, the developer retried all failed transactions during an RPC outage.
// An errored queued transaction (resendCount = 0) is safe to retry: the transaction wasn't sent to RPC.
if (transaction.status === "errored" && resendCount === 0) {
const { errorMessage, ...omitted } = transaction;
transaction = {
...{
...transaction,
nonce: undefined,
Copy link
Contributor Author

@arcoraven arcoraven Nov 8, 2024

Choose a reason for hiding this comment

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

This was the bug. By setting values to undefined, they were explicitly set to undefined in the type. This is unexpected because the typing implies undefined is not a valid value, which leads to weird edge cases like the code not handling undefined values.

Copy link
Contributor Author

@arcoraven arcoraven Nov 8, 2024

Choose a reason for hiding this comment

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

The fix here was to flatten this code to override certain fields only.

Note:

  • nonce doesn't need to be unset since we're calling _sendTransaction() again which generates a new nonce as expected.
  • gas fees don't need to be unset since overrides were moved to a separate nested object.

errorMessage: undefined,
gas: undefined,
gasPrice: undefined,
maxFeePerGas: undefined,
maxPriorityFeePerGas: undefined,
},
...omitted,
status: "queued",
resendCount: 0,
queueId: transaction.queueId,
queuedAt: transaction.queuedAt,
value: transaction.value,
data: transaction.data,
manuallyResentAt: new Date(),
} satisfies QueuedTransaction;
}
Expand Down Expand Up @@ -246,11 +244,14 @@ const _sendUserOp = async (
waitForDeployment: false,
})) as UserOperation; // TODO support entrypoint v0.7 accounts
} catch (error) {
return {
const errorMessage = wrapError(error, "Bundler").message;
const erroredTransaction: ErroredTransaction = {
...queuedTransaction,
status: "errored",
errorMessage: wrapError(error, "Bundler").message,
} satisfies ErroredTransaction;
errorMessage,
};
job.log(`Failed to populate transaction: ${errorMessage}`);
return erroredTransaction;
Comment on lines +247 to +254
Copy link
Contributor Author

@arcoraven arcoraven Nov 8, 2024

Choose a reason for hiding this comment

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

Unrelated fix, I previously removed the job.log() here but now I want it back. It's important for us to know if an error was thrown during simulation/estimation or when sending the tx.

}

job.log(`Populated userOp: ${stringify(signedUserOp)}`);
Expand Down Expand Up @@ -322,12 +323,15 @@ const _sendTransaction = async (
maxPriorityFeePerGas: overrides?.maxPriorityFeePerGas,
},
});
} catch (e: unknown) {
return {
} catch (error: unknown) {
const errorMessage = wrapError(error, "RPC").message;
const erroredTransaction: ErroredTransaction = {
...queuedTransaction,
status: "errored",
errorMessage: wrapError(e, "RPC").message,
} satisfies ErroredTransaction;
errorMessage,
};
job.log(`Failed to populate transaction: ${errorMessage}`);
return erroredTransaction;
}

// Handle if `maxFeePerGas` is overridden.
Expand Down
Loading