improve(PriceClient): Permit retry on failed lookups #157
Conversation
Simplifies usage of the base HTTP class in advance of adding support for retries.
ACX-598 Add simply retry logic to PriceClient in Relayer
Today the client waits 5s before timing out, it also should retry |
| } | ||
| } while (tries <= this.retries); | ||
|
|
||
| throw new Error(`${this.name} price lookup failure (${errs.join(", ")})`); |
There was a problem hiding this comment.
FWIW, if we bump to TypeScript 4.9 then it should be possible to tack errs in under the cause argument to Error(). This would mean we don't need to concatenate errs into a string and might be desirable for nicer logging by upper-layer error handlers. Reference: microsoft/TypeScript#50583.
There was a problem hiding this comment.
Why not bump it? Does it break anything?
There was a problem hiding this comment.
Why not bump it? Does it break anything?
Fair question - was wary of a yak shaving exercise but it turned out to be a small diff, and all our tests pass: #159.
There was a problem hiding this comment.
FWIW I tried using Error(..., { cause }), but ended up in some kind of purgatory.
Per https://stackoverflow.com/a/73394582, it seems like we'd need at least es2022 support in our environment (i.e. specified via lib: [..., "es2022"] in tsconfig.json).
My env seems to support up to es2020 (tsc v4.9.5, nodejs v18.14.1)...but for the following code, I consistently get killed when I try to pass >1 arguments to Error().
>> 59 throw new Error(`${this.name} price lookup failure`, { cause: errs });
(typescript) Error: across-protocol/sdk-v2/src/priceClient/adapters/baseAdapter.ts(59,58): semantic error TS2554: Expected 0-1 arguments, but got 2.
Error: across-protocol/sdk-v2/src/priceClient/adapters/baseAdapter.ts(59,58): semantic error TS2554: Expected 0-1 arguments, but got 2
I've also noticed that I can't specify "es2022" for tsconfig.json compilerOptions.lib:
$ tsdx build && husky install
rpt2: config error TS6046: Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'es2019', 'es2020', 'esnext', 'dom', 'dom.iterable', 'webworker', 'webworker.importscripts', 'scripthost', 'es2015.core', 'es2015.collection', 'es2015.generator', 'es2015.iterable', 'es2015.promise', 'es2015.proxy', 'es2015.reflect', 'es2015.symbol', 'es2015.symbol.wellknown', 'es2016.array.include', 'es2017.object', 'es2017.sharedmemory', 'es2017.string', 'es2017.intl', 'es2017.typedarrays', 'es2018.asyncgenerator', 'es2018.asynciterable', 'es2018.intl', 'es2018.promise', 'es2018.regexp', 'es2019.array', 'es2019.object', 'es2019.string', 'es2019.symbol', 'es2020.bigint', 'es2020.promise', 'es2020.string', 'es2020.symbol.wellknown', 'esnext.array', 'esnext.symbol', 'esnext.asynciterable', 'esnext.intl', 'esnext.bigint', 'esnext.string', 'esnext.promise'.
Ultimately I was able to chase this down to a package named rollup-plugin-typescript2, which is a depdendency of tsdx. It seems like rpt2 doesn't yet permit es2022 to be explicitly specified in tsconfig.json 😞
There was a problem hiding this comment.
See also #157 (comment) for an intermediate conclusion on why we can't use Error(..., { cause}) yet.
src/priceClient/priceClient.e2e.ts
Outdated
|
|
||
| protected sleep = (ms: number) => { | ||
| ++this.nRetries; | ||
| return new Promise((r) => setTimeout(r, ms > 1 ? 1 : ms)); |
There was a problem hiding this comment.
Why not just always setTimeout(r, 0)?
There was a problem hiding this comment.
There were complaints about the value of ms being ignored so I just hacked that in. I've revisited it and realised it was just an eslint error that I could disable. Implemented here: 823a6a0.
src/priceClient/priceClient.e2e.ts
Outdated
| @@ -69,6 +87,17 @@ function validateTokenPrice(tokenPrice: TokenPrice, address: string, timestamp: | |||
| ); | |||
| } | |||
|
|
|||
| describe("PriceClient: BaseHTTPAdapter", function () { | |||
| test("Retry behaviour", async function () { | |||
There was a problem hiding this comment.
nit: it might be nice to have a test that ensures the retry process succeeds and returns the right result if some call before nRetries is successful. Basically the opposite of this test.
There was a problem hiding this comment.
This will require some introspection with axios but it's a good point - we should test this. I'll work on adding that.
There was a problem hiding this comment.
Test added here: 5c42d1b. It works by permitting the request to finally succeed on the final retry.
| } | ||
| } while (tries <= this.retries); | ||
|
|
||
| throw new Error(`${this.name} price lookup failure (${errs.join(", ")})`); |
There was a problem hiding this comment.
Why not bump it? Does it break anything?
Per https://stackoverflow.com/a/73394582, it seems like es2022 is required.
Prompted by Matt.
Fixes ACX-598.