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

improve(PriceClient): Permit retry on failed lookups #157

Merged
merged 11 commits into from
Mar 7, 2023

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Mar 1, 2023

Fixes ACX-598.

pxrl added 2 commits March 1, 2023 03:58
Simplifies usage of the base HTTP class in advance of adding support for
retries.
@linear
Copy link

linear bot commented Mar 1, 2023

ACX-598 Add simply retry logic to PriceClient in Relayer

Today the client waits 5s before timing out, it also should retry

@pxrl pxrl changed the base branch from master to pxrl/priceclient-tidy March 1, 2023 03:09
@james-a-morris
Copy link
Contributor

@pxrl This looks good to me, but there are some linting issues that should be resolved prior to our next version. Linked here: #158

Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

Base automatically changed from pxrl/priceclient-tidy to master March 2, 2023 02:07
}
} while (tries <= this.retries);

throw new Error(`${this.name} price lookup failure (${errs.join(", ")})`);
Copy link
Contributor Author

@pxrl pxrl Mar 2, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not bump it? Does it break anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been bumped now 👍.

Copy link
Contributor Author

@pxrl pxrl Mar 4, 2023

Choose a reason for hiding this comment

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

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 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also #157 (comment) for an intermediate conclusion on why we can't use Error(..., { cause}) yet.

@pxrl
Copy link
Contributor Author

pxrl commented Mar 2, 2023

@pxrl This looks good to me, but there are some linting issues that should be resolved prior to our next version. Linked here: #158

Well spotted. Looks like they are unrelated to this change, but I agree that it should be resolved before we bump next, so we can make them fatal :).

Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!


protected sleep = (ms: number) => {
++this.nRetries;
return new Promise((r) => setTimeout(r, ms > 1 ? 1 : ms));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just always setTimeout(r, 0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -69,6 +87,17 @@ function validateTokenPrice(tokenPrice: TokenPrice, address: string, timestamp:
);
}

describe("PriceClient: BaseHTTPAdapter", function () {
test("Retry behaviour", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will require some introspection with axios but it's a good point - we should test this. I'll work on adding that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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(", ")})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not bump it? Does it break anything?

@pxrl pxrl requested a review from mrice32 March 4, 2023 13:46
@pxrl pxrl merged commit 4466b75 into master Mar 7, 2023
@pxrl pxrl deleted the pxrl/priceclient-retries branch March 7, 2023 07:57
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.

3 participants