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

Retry mechanism #1078

Closed
digorithm opened this issue Jun 28, 2023 · 5 comments · Fixed by #1474 or #1495
Closed

Retry mechanism #1078

digorithm opened this issue Jun 28, 2023 · 5 comments · Fixed by #1474 or #1495
Assignees
Labels
feat Issue is a feature

Comments

@digorithm
Copy link
Member

We should support an optional, configurable parameter to enable retries when sending a transaction to the node. This mechanism's design and API are part of this work; nothing in mind yet!

@arboleya
Copy link
Member

arboleya commented Jun 28, 2023

Great idea; transient failures should indeed be accounted for. We could have a simple option like this (below) and have an internal retry pattern strategy with exponential back-off.

// if nothing is specified, we could have a default maxRetries=5
contract.functions.xyz(params).call({ maxRetries: 5 });

@digorithm Is there something like this on the Rust SDK already?

@digorithm
Copy link
Member Author

Nope, not yet. This API looks good to me!

@nedsalk
Copy link
Contributor

nedsalk commented Jun 30, 2023

What about a global fuels.config.ts file in addition to @arboleya's proposal? The user then wouldn't have to set it on every .call() but could still use @arboleya's solution to override the defaults they set in fuels.config.

{
 maxRetries: 5,
 txParams: {
  gasPrice: 2
 },
 contracts: {
  maxRetries: 2,
  txParams: {
   gasPrice: 1
  }
 }
}

Or if we don't go down this route, we could put an optional options = {...theAbove...} parameter in all of our program constructors. We could even provide all three ways of setting configurations:

  1. global via fuels.config.ts,
  2. program-specific via constructors / property setters,
  3. call-specific via @arboleya's proposal

@digorithm
Copy link
Member Author

That's a lot of complexity for very little gain. The JS ecosystem already suffers from way too many config files, I'd rather not contribute to that, haha.

And by very little gain I mean: gas price and many other transaction related configurations are mostly configured per call, usually being dynamically changed very quickly; often gas price being queried from external sources before a call happens. It isn't something that's set one time and the user rarely touches it. Retries does fit in this behaviour, though. But adding a whole new config file for just this value isn't justifiable.

@nedsalk
Copy link
Contributor

nedsalk commented Jun 30, 2023

Haha, yeah, makes sense. However, having to specify retries on a per-call basis might be cumbersome. A per-program config object passed via the constructor would alleviate some of that pain, so I think it's worth considering.

const myContract = new Contract(theAbi, ...whateverElse...,
 {
 maxRetries: 5, 
 retryStrategyCallback: (retryNumber, isLastRetry) => ({callAgain: true}),
 onCallFail: async () => await notifyTheWhiteHouse()
 })

The retryStrategyCallback and onCallFail are just examples of how easily extendable this config object would be. Functionality such as this would permit us to define sane defaults and also give the user the ability to override them if they choose to.

It could also be modified like below, although I'm not a fan because it's one more way for bugs to creep up:

myContract.configs.maxRetries = -1; // if negative, retry to infinity, etc.

@nedsalk nedsalk self-assigned this Aug 31, 2023
@arboleya arboleya added the feat Issue is a feature label Dec 7, 2023
@nedsalk nedsalk linked a pull request Dec 10, 2023 that will close this issue
@nedsalk nedsalk linked a pull request Jan 4, 2024 that will close this issue
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants