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
3 changes: 2 additions & 1 deletion src/priceClient/adapters/acrossApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ export class PriceFeed extends BaseHTTPAdapter implements PriceFeedAdapter {
name = "Across API",
host = "across.to",
timeout = 5000, // ms
retries = 3,
}: AcrossApiArgs = {}) {
// Allow host to be overridden for test or alternative deployments.
super(name, host, { timeout });
super(name, host, { timeout, retries });
}

async getPriceByAddress(address: string, currency = "usd"): Promise<TokenPrice> {
Expand Down
42 changes: 35 additions & 7 deletions src/priceClient/adapters/baseAdapter.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
import assert from "assert";
import axios, { AxiosError } from "axios";
import axios from "axios";

export type BaseHTTPAdapterArgs = {
timeout?: number;
retries?: number;
};

export class BaseHTTPAdapter {
private _retries = 0;
private _timeout = 0;

get retries(): number {
return this._retries;
}

set retries(retries: number) {
assert(retries >= 0);
this._retries = retries;
}

get timeout(): number {
return this._timeout;
}
Expand All @@ -17,7 +28,12 @@ export class BaseHTTPAdapter {
this._timeout = timeout;
}

constructor(public readonly name: string, public readonly host: string, { timeout = 1000 }: BaseHTTPAdapterArgs) {
constructor(
public readonly name: string,
public readonly host: string,
{ timeout = 1000, retries = 1 }: BaseHTTPAdapterArgs
) {
this.retries = retries;
this.timeout = timeout; // ms
}

Expand All @@ -28,10 +44,22 @@ export class BaseHTTPAdapter {
params: urlArgs ?? {},
};

const result = await axios(url, args).catch((err) => {
const errMsg = err instanceof AxiosError ? err.message : err.toString();
throw new Error(`${this.name} price lookup failure (${errMsg})`);
});
return result.data;
const errs: string[] = [];
let tries = 0;
do {
try {
return (await axios(url, args)).data;
} catch (err) {
const errMsg = axios.isAxiosError(err) || err instanceof Error ? err.message : "unknown error";
errs.push(errMsg);
if (++tries <= this.retries) await this.sleep(Math.pow(1.5, tries) * 1000); // simple backoff
}
} 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.

}

protected async sleep(ms: number): Promise<void> {
return new Promise((r) => setTimeout(r, ms));
}
}
3 changes: 2 additions & 1 deletion src/priceClient/adapters/coingecko.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ type CoinGeckoArgs = BaseHTTPAdapterArgs & {
export class PriceFeed extends BaseHTTPAdapter implements PriceFeedAdapter {
private readonly apiKey: string | undefined = undefined;

constructor({ name, apiKey, timeout = 5000 }: CoinGeckoArgs = {}) {
constructor({ name, apiKey, timeout = 5000, retries = 3 }: CoinGeckoArgs = {}) {
super(name ?? apiKey ? "CoinGecko Pro" : "CoinGecko Free", apiKey ? "pro-api.coingecko.com" : "api.coingecko.com", {
timeout,
retries,
});
this.apiKey = apiKey;
}
Expand Down
3 changes: 2 additions & 1 deletion src/priceClient/adapters/defiLlama.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ export class PriceFeed extends BaseHTTPAdapter implements PriceFeedAdapter {
name = "DefiLlama",
host = "coins.llama.fi",
timeout = 5000,
retries = 2,
minConfidence = 0.9,
}: DefiLlamaArgs = {}) {
super(name, host, { timeout });
super(name, host, { timeout, retries });
assert(minConfidence >= 0.0 && minConfidence <= 1.0);
this._minConfidence = minConfidence;
}
Expand Down
68 changes: 61 additions & 7 deletions src/priceClient/priceClient.e2e.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,30 @@
import assert from "assert";
import axios from "axios";
import dotenv from "dotenv";
import winston from "winston";
import { Logger, msToS, PriceCache, PriceClient, PriceFeedAdapter, TokenPrice } from "./priceClient";
import { acrossApi, coingecko, defiLlama } from "./adapters";
import { BaseHTTPAdapter, BaseHTTPAdapterArgs } from "./adapters/baseAdapter";

dotenv.config();

class TestBaseHTTPAdapter extends BaseHTTPAdapter {
public nRetries = 0;

constructor(name: string, host: string, { timeout = 500, retries = 1 }: BaseHTTPAdapterArgs) {
super(name, host, { timeout, retries });
}

async _query(path: string, urlArgs?: object): Promise<unknown> {
return this.query(path, urlArgs);
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected override async sleep(_ms: number): Promise<void> {
++this.nRetries;
}
}

class TestPriceClient extends PriceClient {
constructor(logger: Logger, priceFeeds: PriceFeedAdapter[]) {
super(logger, priceFeeds);
Expand Down Expand Up @@ -69,6 +88,41 @@ function validateTokenPrice(tokenPrice: TokenPrice, address: string, timestamp:
);
}

describe("PriceClient: BaseHTTPAdapter", function () {
test("Retry behaviour: All failures", async function () {
for (const retries of [0, 1, 3, 5, 7, 9]) {
const name = `BaseHTTPAdapter test w/ ${retries} retries`;
const baseAdapter = new TestBaseHTTPAdapter(name, "127.0.0.1", { timeout: 1, retries });
expect(baseAdapter.nRetries).toBe(0);
await expect(baseAdapter._query("", { retries })).rejects.toThrow(`${name} price lookup failure`);
expect(baseAdapter.nRetries).toBe(retries);
}
});

test("Retry behaviour: Success on the final request", async function () {
for (const retries of [1, 3, 5, 7, 9]) {
const name = `BaseHTTPAdapter test w/ success on retry ${retries}`;
const baseAdapter = new TestBaseHTTPAdapter(name, "127.0.0.1", { timeout: 1, retries });
expect(baseAdapter.nRetries).toBe(0);

// Instantiate callback for HTTP response != 2xx.
const interceptor = axios.interceptors.response.use(
undefined, // HTTP 2xx.
function (error) {
const result = retries && baseAdapter.nRetries === retries ? Promise.resolve({}) : Promise.reject(error);
return result;
}
);

const response = baseAdapter._query("", { retries });
await expect(response).resolves.not.toThrow();
axios.interceptors.response.eject(interceptor); // Cleanup ASAP.

expect(baseAdapter.nRetries).toBe(retries);
}
});
});

// this requires e2e testing, should only test manually for now
describe("PriceClient", function () {
const dummyLogger: winston.Logger = winston.createLogger({
Expand Down Expand Up @@ -140,7 +194,7 @@ describe("PriceClient", function () {

test("getPriceByAddress: Across failover to Across", async function () {
pc = new PriceClient(dummyLogger, [
new acrossApi.PriceFeed({ name: "Across API (expect fail)", host: "127.0.0.1" }),
new acrossApi.PriceFeed({ name: "Across API (expect fail)", host: "127.0.0.1", retries: 0 }),
new acrossApi.PriceFeed({ name: "Across API (expect pass)" }),
]);

Expand All @@ -151,7 +205,7 @@ describe("PriceClient", function () {
test("getPriceByAddress: Coingecko failover to Across", async function () {
const _apiKey = "xxx-fake-apikey";
pc = new PriceClient(dummyLogger, [
new coingecko.PriceFeed({ name: "CoinGecko Pro (expect fail)", apiKey: _apiKey }),
new coingecko.PriceFeed({ name: "CoinGecko Pro (expect fail)", apiKey: _apiKey, retries: 0 }),
new acrossApi.PriceFeed({ name: "Across API (expect pass)" }),
]);

Expand All @@ -161,14 +215,14 @@ describe("PriceClient", function () {

test("getPriceByAddress: Complete price lookup failure", async function () {
pc = new PriceClient(dummyLogger, [
new acrossApi.PriceFeed({ name: "Across API #1 (expect fail)", host: "127.0.0.1" }),
new acrossApi.PriceFeed({ name: "Across API #2 (expect fail)", host: "127.0.0.1" }),
new acrossApi.PriceFeed({ name: "Across API #1 (expect fail)", host: "127.0.0.1", retries: 0 }),
new acrossApi.PriceFeed({ name: "Across API #2 (expect fail)", host: "127.0.0.1", retries: 0 }),
]);
await expect(pc.getPriceByAddress(testAddress)).rejects.toThrow();
});

test("getPriceByAddress: Across API timeout", async function () {
const acrossPriceFeed: acrossApi.PriceFeed = new acrossApi.PriceFeed({ name: "Across API (timeout)" });
const acrossPriceFeed: acrossApi.PriceFeed = new acrossApi.PriceFeed({ name: "Across API (timeout)", retries: 0 });
pc = new PriceClient(dummyLogger, [acrossPriceFeed]);

acrossPriceFeed.timeout = 1; // mS
Expand Down Expand Up @@ -206,7 +260,7 @@ describe("PriceClient", function () {
test("getPriceByAddress: Address case insensitivity", async function () {
// Instantiate a custom subclass of PriceClient.
const pc: TestPriceClient = new TestPriceClient(dummyLogger, [
new acrossApi.PriceFeed({ name: "Across API (expect fail)", host: "127.0.0.1" }),
new acrossApi.PriceFeed({ name: "Across API (expect fail)", host: "127.0.0.1", retries: 0 }),
]);

// Load the cache with lower-case addresses, then query with an upper-case address.
Expand Down Expand Up @@ -234,7 +288,7 @@ describe("PriceClient", function () {
test("getPriceByAddress: Validate price cache", async function () {
// Instantiate a custom subclass of PriceClient; load the cache and force price lookup failures.
const pc: TestPriceClient = new TestPriceClient(dummyLogger, [
new acrossApi.PriceFeed({ name: "Across API (expect fail)", host: "127.0.0.1" }),
new acrossApi.PriceFeed({ name: "Across API (expect fail)", host: "127.0.0.1", retries: 0 }),
]);

const priceCache: PriceCache = pc.getProtectedPriceCache(baseCurrency);
Expand Down
2 changes: 1 addition & 1 deletion src/priceClient/priceClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export class PriceClient implements PriceFeedAdapter {
priceFeeds: this.listPriceFeeds(),
tokens: addresses,
});
throw Error(`Price lookup failed against all price feeds (${this.listPriceFeeds().join(", ")})`);
throw new Error(`Price lookup failed against all price feeds (${this.listPriceFeeds().join(", ")})`);
}
}

Expand Down