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

fix(contract-client): timeout & retry logic #919

Closed
wants to merge 1 commit into from

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Jan 30, 2024

no more TimeoutInfinite

Shaptic
Shaptic previously approved these changes Jan 30, 2024
@Shaptic Shaptic self-requested a review January 30, 2024 22:00
@Shaptic Shaptic dismissed their stale review January 30, 2024 22:01

didn't see it was in draft, my b

@Shaptic Shaptic removed their request for review January 30, 2024 22:01
@chadoh chadoh force-pushed the fix/timeout-and-retry-logic branch from f072c51 to ac5545b Compare January 31, 2024 17:28
@chadoh chadoh marked this pull request as ready for review January 31, 2024 20:48
@chadoh chadoh requested a review from Shaptic January 31, 2024 20:48
@chadoh
Copy link
Contributor Author

chadoh commented Jan 31, 2024

@Shaptic thanks! I'm not sure how exactly to clean up the retry logic, actually. Maybe you could help give some pointers? About a month ago, I learned from Tyler van der Hoeven that the rebuild/resign/resend logic is not so great, but I forget what the ideas were for improving it. Here's the current signAndSend logic:

  1. attempt sendTransaction
    • while attempt status !== PENDING
      • rebuild transaction, increment sequenceNumber & request new signature
      • 🔄 retry
      • 📈 exponential backoff – wait longer to retry each time
    • save all attempts as sendTransactionResponseAll
  2. if no sendTransaction status ever comes back as PENDING,
    • 🛑 throw error
  3. attempt getTransaction, using hash from pending sendTransaction
    • while getTransaction attempt status === NOT_FOUND
      • 🔄 retry
      • 📈 exponential backoff – wait longer to retry each time
  4. if final attempt still has status === NOT_FOUND
    • 🛑 throw error
  5. return SentTransaction

Or, as a flow chart:

graph TD;
    A[Start: Attempt sendTransaction] --> B{Status == PENDING?};
    B -- No --> C[Rebuild transaction, increment sequenceNumber & request new signature];
    C --> D[Retry with exponential backoff];
    D --> B;
    B -- Yes --> E[Save as sendTransactionResponseAll];
    E --> F{Did final sendTransaction have status == PENDING?};
    F -- No --> G[🛑 oh no];
    F -- Yes --> H[Attempt getTransaction using hash from sendTransactionResult];
    H --> I{Status == NOT_FOUND?};
    I -- Yes --> J[Retry with exponential backoff];
    J --> I;
    I -- No --> K[Continue];
    K --> L{Final attempt status == NOT_FOUND?};
    L -- Yes --> M[🛑 oh no];
    L -- No --> N[Return SentTransaction];
Loading

If GitHub refuses to render that, you can view it at https://www.mermaidchart.com/raw/10719cfd-0a10-45c4-b58b-5fe2e3cbdd38?theme=dark&version=v0.1&format=svg

@chadoh
Copy link
Contributor Author

chadoh commented Jan 31, 2024

If you want to merge this one as-is, I can clean up the retry logic in a follow-up

@Shaptic
Copy link
Contributor

Shaptic commented Feb 1, 2024

Thank you for the breakdown!! Both explanations are super helpful. As a prerequisite, I hope you've read the error-handling guide which covers a lot of nuances. With that in mind, here are some thoughts:

while != PENDING
rebuild transaction, increment sequenceNumber & request new signature

Do you differentiate between recoverable and non-recoverable errors? For example, fees vs. seqnum issues. Also let's try to keep the whole "signer-agnostic" thing in mind in the "request new signature" bit (like the generic signer in authorizeEntry) while we decide where this code belongs.

if no sendTransaction status ever comes back as PENDING

Is the amount of time or retries user-controlled?

exponential backoff – wait longer to retry each time

For getTransaction, linear back-off is probably better since! Exponential delays the status check unnecessarily, since we're polling vs. actually submitting.

return SentTransaction

Does this include the sendTransactionResponseAll?


also cc @kalepail for his thoughts!

/**
* The maximum amount of time to wait for the transaction to complete. Default: {@link DEFAULT_TIMEOUT}
*/
timeoutInSeconds?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this required rather than have a default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would way rather have a default! That way you don't need to include a second options object on every call. If we make it required, every function call from the generated Contract Clients would look like this:

await myContract.myMethod(
  { args: 'for', my: 'method', ... },
  { timeoutInSeconds: 10 }
)

Do you think this is important enough to bring to people's attention that we should uglify every method call in this way, or do you think there's a good-enough default?

@chadoh
Copy link
Contributor Author

chadoh commented Feb 8, 2024

I discussed the retry logic & API with @kalepail and came up with the approach in #921. Closing this one in favor of that.

@chadoh chadoh closed this Feb 8, 2024
@chadoh chadoh deleted the fix/timeout-and-retry-logic branch February 8, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants