feat(indexer): add coingecko price collection from tokens against usd #138

daywiss · 2024-12-18T19:51:00Z

motivation

we want to start collecting price data in the indexer

changes

This dynamically looks up prices based on fill events, passing them to a worker to then fetch and/or cache the price at the deposit time

linear · 2024-12-18T19:51:04Z

ACX-3505 Identify tokens to start indexing prices

amateima · 2024-12-19T09:14:52Z

...ges/indexer-database/packages/indexer-database/src/migrations/1733941169940-HistoricPrice.ts

+import { MigrationInterface, QueryRunner } from "typeorm";
+
+export class HistoricPrice1733941169940 implements MigrationInterface {
+    name = 'HistoricPrice1733941169940'
+
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`CREATE TABLE "historic_market_price" ("id" SERIAL NOT NULL, "baseCurrency" character varying NOT NULL, "quoteCurrency" character varying NOT NULL DEFAULT 'usd', "date" date NOT NULL, "price" numeric NOT NULL, "createdAt" TIMESTAMP NOT NULL DEFAULT now(), CONSTRAINT "UK_historic_price_baseCurrency_quoteCurrency_date" UNIQUE ("baseCurrency", "quoteCurrency", "date"), CONSTRAINT "PK_b0a22436b47e742187aa7408561" PRIMARY KEY ("id"))`);
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`DROP TABLE "historic_market_price"`);
+    }
+
+}


Not sure if these 2 migration files are different, but I guess one of them is not needed. The second migration will fail because the table will already be created by the first one

yea this was a mistake. wil fix

amateima · 2024-12-19T09:17:47Z

packages/indexer-database/src/entities/HistoricPrice.ts

+  @Column()
+  date: string;


This should definitely be a typed Date property and the DB column should be date. This way we can take advantage of special queries/functionalities that SQL provides for such column type.

i think the complexity is that we only care about 1 price per day, which means deterministically calculating the "day" based on a js Date or timestamp becomes more involved. Ill add the logic and u can see if thats what you want

amateima · 2024-12-19T09:33:09Z

packages/indexer/src/services/priceProcessor.ts

+          return;
+        }
+        await historicPriceRepository.insert({
+          date: dbFormattedDate,


I propose to not leak here the weird way that CoinGecko handles date format. Here we should pass a simple date instance as value

amateima · 2024-12-19T09:37:20Z

packages/indexer/src/services/priceProcessor.ts

+        }
+        await historicPriceRepository.insert({
+          date: dbFormattedDate,
+          baseCurrency: symbol,


In the past I experienced inconsistencies with the symbols returned by CoinGecko API, for example different letter capitalisation for the same token, so I propose to do a toLowerCase() on the symbol

this is currently an enum, so if anything is wrong we can fix it in the enum

amateima · 2024-12-19T10:11:52Z

packages/indexer/src/parseEnv.ts

@@ -193,6 +196,9 @@ export function envToConfig(env: Env): Config {
    enabledWebhookRequestWorkers: true,
    clients: parseWebhookClientsFromString(env.WEBHOOK_CLIENTS ?? "[]"),
  };
+  const coingeckoSymbols = parseArray(env.COINGECKO_SYMBOLS).map((symbol) =>


We should document this ENV value in the README and specify the format of it (list, object, etc)

amateima · 2024-12-30T12:48:49Z

packages/error-handling/src/utils/assert.ts

  try {
-    return assertModule.ok(value, message);
+    return assertModule.ok(value !== null && value !== undefined, message);


What was the edge case that was not handled gracefully by the current implementation?

i probably dont need to do this

amateima · 2024-12-30T13:05:31Z

packages/indexer/src/messaging/priceWorker.ts

+/**
+ * This worker listens to the `IntegratorId` queue and processes each job by:
+ * - Retrieving the deposit information from the database based on the provided relay hash.
+ * - Checking if the deposit record already has an integrator ID.
+ * - If the integrator ID is not set, the worker fetches it from the transaction data.
+ * - If found, the integrator ID is saved back into the deposit record in the database.
+ */


These comments should be updated or removed

amateima · 2024-12-30T13:09:17Z

packages/indexer/src/messaging/priceWorker.ts

+ * - If found, the integrator ID is saved back into the deposit record in the database.
+ */
+export class PriceWorker {
+  public worker: Worker;


Is this property needed to be public?

amateima · 2024-12-30T13:11:26Z

packages/indexer/src/messaging/priceWorker.ts

+    const relayHashInfoRepository = this.postgres.getRepository(
+      entities.RelayHashInfo,
+    );
+    const depositRepository = this.postgres.getRepository(
+      entities.V3FundsDeposited,
+    );
+    const historicPriceRepository = this.postgres.getRepository(
+      entities.HistoricPrice,
+    );


Would make sense to make the repository variables to be class properties and set them in the constructor? This way we would reduce the scope of run() function by not having it to deal with instantiating dependencies

amateima · 2024-12-30T13:30:09Z

packages/indexer/src/messaging/priceWorker.ts

+    const baseTokenInfo = findTokenByAddress(
+      relayHashInfo.fillEvent.outputToken,
+      relayHashInfo.destinationChainId,
+    );


I see that findTokenByAddress() function works on a list of tokens that filters out the tokens for which we don't have the CoinGecko symbol defined in the indexer. I don't think this is necessary.
The only condition we need to check is if the token is part of the supported tokens specified in the @across-protocol/constants. If we do this, we also don't need the CoinGecko env anymore

amateima · 2024-12-30T13:30:23Z

packages/indexer/src/messaging/priceWorker.ts

+    }
+    const priceTime = yesterday(blockTime);
+    const quoteCurrency = "usd";
+    const baseTokenInfo = findTokenByAddress(


We need to fetch price for input token too

amateima · 2024-12-30T13:52:17Z

packages/indexer/src/messaging/priceWorker.ts

+ * - If not, fetching the historic price from Coingecko and inserting it into the database.
+ * - Logging errors and information at various stages of the process.
+ */
+export class PriceWorker {


The way errors are handled in this worker should be reviewed and changed accordingly. The only situation in which the message should be discarded and execution should stop is the case when the deposit with a particular depositId and originChainId doesn't exist. Otherwise, we should log the error (as we do) and throw an error, so that the worker retries the message

amateima

I haven't looked at the code yet. I tried to test it locally, but I hit this error

amateima

I am not able to run the workers successfully, see screenshot for error.

I think the problem is at line 154 in the worker:

const deposit = await this.depositRepository.findOne({
      where: { depositId, originChainId },
    });

Is very possible for fill events to be fetched before the deposit events on the origin chain, so maybe in this case we should retry messages until the corresponding deposit event is found in the DB?

james-a-morris

Could be worth reviewing: https://github.com/across-protocol/sdk/tree/master/src/coingecko

We could probably leverage this heavily to avoid code duplication. This will be helpful when we have to worry about SVM.

packages/error-handling/src/utils/assert.ts

packages/indexer-database/src/entities/HistoricPrice.ts

james-a-morris · 2025-01-07T16:01:32Z

packages/indexer-database/src/entities/HistoricPrice.ts

+  // bear in mind we are using coingecko symbols directly here, for all intents and purposes this is coingecko historic market price
+  @Column()
+  baseCurrency: string;
+
+  @Column({ default: "usd" })
+  quoteCurrency: string;


Could be worth enumerated the expected values of these. We're unlikely to deviate from usd or eth or azero or matic as a base currency and the quote currency is unlikely to not be usd

packages/indexer-database/src/entities/RelayHashInfo.ts

packages/indexer/src/data-indexing/service/SpokePoolIndexerDataHandler.ts

packages/indexer/src/messaging/priceWorker.ts

james-a-morris · 2025-01-07T16:07:47Z

packages/indexer/src/parseEnv.ts

OOC why was this updated?

originally there was some env changes made for this pr, which are no longer needed, but this was accidentally left in

i think stylistically this is slightly better than before but i can revert if you like the other way better

james-a-morris · 2025-01-07T16:08:46Z

packages/indexer/src/utils/currencyUtils.ts

+// given an address and chain id, return the token data
+export function findTokenByAddress(address: string, chainId: number): Token {
+  const result = tokensList.find(
+    (token) =>
+      token.address.toLowerCase() === address.toLowerCase() &&
+      token.chainId === chainId,
+  );
+  if (!result) {
+    throw new Error(
+      `Token info not found for address: ${address} on chainId: ${chainId}`,
+    );
+  }
+  return result;
+}


We have a similar function in the SDK - maybe we should defer to this

+1 if such function exists

will need to tackle this when i get back, but was able to remove the coingecko library

Signed-off-by: david <david@umaproject.org>

… prices Signed-off-by: david <david@umaproject.org>

Signed-off-by: david <david@umaproject.org>

…output price Signed-off-by: david <david@umaproject.org>

Signed-off-by: david <david@umaproject.org>

james-a-morris

Curious about #138 (comment)

daywiss requested review from amateima and james-a-morris December 18, 2024 19:51

amateima requested changes Dec 19, 2024

View reviewed changes

daywiss requested a review from amateima December 19, 2024 14:31

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from 4bb3196 to bc2bf3d Compare December 24, 2024 16:51

amateima requested a review from melisaguevara December 30, 2024 12:47

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from bc2bf3d to a3283e9 Compare December 30, 2024 13:37

amateima requested changes Dec 30, 2024

View reviewed changes

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch 2 times, most recently from deecf87 to ae571cf Compare December 30, 2024 19:08

daywiss requested a review from amateima December 30, 2024 19:10

amateima force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from ae571cf to 8302d4a Compare January 3, 2025 13:30

amateima requested review from mrice32 and nicholaspai as code owners January 3, 2025 13:30

amateima requested changes Jan 3, 2025

View reviewed changes

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from 8183e68 to 4e3756e Compare January 3, 2025 18:58

amateima requested changes Jan 6, 2025

View reviewed changes

daywiss requested a review from amateima January 6, 2025 18:28

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from 58703e7 to fe7f7de Compare January 6, 2025 18:49

james-a-morris requested changes Jan 7, 2025

View reviewed changes

David and others added 8 commits January 7, 2025 13:28

feat(indexer): add coingecko price collection from tokens against usd

cc3e23c

Signed-off-by: david <david@umaproject.org>

fix migration, add docs, use date for database

9d073bd

Signed-off-by: david <david@umaproject.org>

refactor price lookups to use workers

aadbe43

Signed-off-by: david <david@umaproject.org>

remove config, include bridge fee calculation, input and output token…

9e8980a

… prices Signed-off-by: david <david@umaproject.org>

add relay hash info migration

2dee96b

Signed-off-by: david <david@umaproject.org>

add inputPriceUsd, outputPriceUsd to relay hash info

b7b2314

Signed-off-by: david <david@umaproject.org>

test bridge fee calc, use price symbols to store prices, store input/…

3d51725

…output price Signed-off-by: david <david@umaproject.org>

change saves to updates, handle invalid fills

e7d59ca

Signed-off-by: david <david@umaproject.org>

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from fe7f7de to f40359a Compare January 7, 2025 18:28

use sdk coingecko, use floats for prices

975f323

Signed-off-by: david <david@umaproject.org>

daywiss force-pushed the david/acx-3505-identify-tokens-to-start-indexing-prices branch from f40359a to 975f323 Compare January 7, 2025 18:33

daywiss requested a review from james-a-morris January 7, 2025 18:33

james-a-morris reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(indexer): add coingecko price collection from tokens against usd #138

feat(indexer): add coingecko price collection from tokens against usd #138

daywiss commented Dec 18, 2024 •

edited

Loading

linear bot commented Dec 18, 2024

amateima Dec 19, 2024

daywiss Dec 19, 2024

amateima Dec 19, 2024 •

edited

Loading

daywiss Dec 19, 2024

amateima Dec 19, 2024

amateima Dec 19, 2024

daywiss Dec 19, 2024

amateima Dec 19, 2024

amateima Dec 30, 2024

daywiss Dec 30, 2024

amateima Dec 30, 2024

amateima Dec 30, 2024

amateima Dec 30, 2024

amateima Dec 30, 2024

amateima Dec 30, 2024

amateima Dec 30, 2024

amateima left a comment

amateima left a comment

james-a-morris left a comment

james-a-morris Jan 7, 2025

james-a-morris Jan 7, 2025

daywiss Jan 7, 2025

daywiss Jan 7, 2025

james-a-morris Jan 7, 2025

amateima Jan 7, 2025

daywiss Jan 7, 2025

james-a-morris left a comment

feat(indexer): add coingecko price collection from tokens against usd #138

Are you sure you want to change the base?

feat(indexer): add coingecko price collection from tokens against usd #138

Conversation

daywiss commented Dec 18, 2024 • edited Loading

motivation

changes

linear bot commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amateima Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amateima left a comment

Choose a reason for hiding this comment

amateima left a comment

Choose a reason for hiding this comment

james-a-morris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-a-morris left a comment

Choose a reason for hiding this comment

daywiss commented Dec 18, 2024 •

edited

Loading

amateima Dec 19, 2024 •

edited

Loading