-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
Conversation
| server.setErrorHandler( | ||
| (error: Error | CustomError | ZodError, request, reply) => { | ||
| (error: string | Error | CustomError | ZodError, request, reply) => { | ||
| if (typeof error === "string") { |
There was a problem hiding this comment.
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.
| transaction = { | ||
| ...{ | ||
| ...transaction, | ||
| nonce: undefined, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
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.
PR-Codex overview
This PR focuses on improving error handling in the transaction processing logic and refining the retry mechanism for failed transactions.
Detailed summary
withErrorHandlerto support string errors.sendTransactionWorkerby omitting unnecessary fields.