fix: Prevent try on some transaction methods and errors #5554
Conversation
…ryables. It prevents retries in the SmartProvider by parsing the transaction method and error response.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5554 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 103 103
Lines 2110 2110
Branches 190 190
=======================================
Hits 1636 1636
Misses 453 453
Partials 21 21
|
🦋 Changeset detectedLatest commit: 0b2dcb0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| } | ||
| export function hasNonRetryableError(params: { method: string; error: Error }) { | ||
| const { method, error } = params; | ||
| const TX_METHODS = ['sendRawTransaction', 'sendTransaction']; |
There was a problem hiding this comment.
I would've thought these would need to include the eth_ prefix, e.g. eth_sendRawTransaction, eth_call, etc
| }); | ||
|
|
||
| pIndex += nonRetryable | ||
| ? providers.length // Setting this to providers.length will force exit the loop and not retry with the next provider |
There was a problem hiding this comment.
feels a little bit like a hack, wonder if there's a nicer way to do it?
worried that it's so dependent on the if (pIndex < providers.length) { and if that were to change, it's not obvious to change it here too
There was a problem hiding this comment.
good point. I've changed this to instead throw
| : 1; | ||
|
|
||
| this.logger.debug( | ||
| isLastProvider || nonRetryable ? '' : 'Triggering next provider.', |
There was a problem hiding this comment.
Should we log something about it being the last time it's attempted and why? feels useful to know if it's non retryable
| @@ -90,13 +90,18 @@ export async function retryAsync<T>( | |||
| runner: () => T, | |||
| attempts = 5, | |||
| baseRetryMs = 50, | |||
| dontRetry: string[] = [], | |||
There was a problem hiding this comment.
I think this would be more flexible as an optional function that's passed in, where if it's specified and returns true (or false, whatever you think is more natural) then it decides to retry or not
| @@ -207,6 +237,7 @@ export class HyperlaneSmartProvider | |||
| () => this.performWithFallback(method, params, supportedProviders, reqId), | |||
| this.options?.maxRetries || DEFAULT_MAX_RETRIES, | |||
| this.options?.baseRetryDelayMs || DEFAULT_BASE_RETRY_DELAY_MS, | |||
| Object.values(TX_ERROR_MESSAGE_PHRASES), | |||
There was a problem hiding this comment.
for my own understanding: this is is needed because we retry within performWithFallback and if we decide to stop retrying there we also want to not do a retryAsync retry right?
A bit of an awkward setup imo the condition for not retrying that we pass in here is slightly different from the condition that we use for retrying in performWithFallback. Just an idea but I wonder if we should have a version of retryAsync that would maybe allow the runner to decide whether it should get retried at the higher level? Something like this maybe, but maybe isn't super compatible with performWithFallback
interface Retryable<T> {
result: T | undefined,
retry: bool,
}
async function retryRetryable<T>(
runner: () => Promise<Retryable<T>>,
// ..
// ..
)
Description
This PR stops retries in the SmartProvider by parsing the transaction method and error response. It prevents them when:
retryAsyncDrive-by changes
Related issues
Backward compatibility
Testing