LW-12681 Coingecko token price API #1867

iccicci · 2025-05-13T10:22:58Z

Checklist

JIRA - LW-12681
Proper tests implemented
Screenshots added.

Proposed solution

Explain how does this PR solves the problem stated in JIRA ticket.
You can also enumerate different alternatives considered while approaching this task.

Testing

Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met

Screenshots

Attach screenshots here if implementation involves some UI changes

lace-bot · 2025-05-13T10:47:15Z

Allure Report

allure-report-publisher generated test report!

processReports: ✅ test report for 3174dc33

	passed	failed	skipped	flaky	total	result
Total	33	0	4	0	37	✅

szymonmaslowski

Great Job!

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

szymonmaslowski · 2025-05-19T10:49:44Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

    () => ({
+      getTokenPrice: (assetId: Wallet.Cardano.AssetId): TokenPrice | undefined => {
+        const tokenPrice = tokenPrices?.tokens?.get(assetId);


Suggested change

const tokenPrice = tokenPrices?.tokens?.get(assetId);

const tokenPrice = tokenPrices.tokens.get(assetId);

e101d0c removes the second question mark; removing the first one as well makes the extension to not load.

szymonmaslowski · 2025-05-19T10:49:55Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

+        const tokenPrice = tokenPrices?.tokens?.get(assetId);
+        // Actually track the price only for token in Cardano mainnet, otherwise just do nothing
+        const trackPrice = () =>
+          store && store.currentChain && store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet


Suggested change

store && store.currentChain && store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet

store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet

Addressing this comment I did a refactoring which changes the layout of the same check.
Anyway (as far as I understood) we still need to check currentChain to be defined as it could be not in case the wallet is in bitcoin mode.

szymonmaslowski · 2025-05-19T10:52:09Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

+        // Actually track the price only for token in Cardano mainnet, otherwise just do nothing
+        const trackPrice = () =>
+          store && store.currentChain && store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet
+            ? trackCardanoTokenPrice(assetId).catch(console.error)


Please use a logger instead of the console in all the occurences.
import { logger } from '@lace/common';

e101d0c should solve this

szymonmaslowski · 2025-05-19T10:53:58Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

+            : // eslint-disable-next-line @typescript-eslint/no-empty-function
+              () => {};


A nitpick. Feel free to ignore.

Suggested change

: // eslint-disable-next-line @typescript-eslint/no-empty-function

() => {};

: () => void 0;

Addressing this comment I did a little refactoring which makes this comment no longer required.

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

szymonmaslowski · 2025-05-19T11:18:02Z

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

+import { config } from '@src/config';
+
+/** Shortcut for the token prices subject type. */
+type TokenPriceSubject = Subject<{ tokens: TokenPrices } & Status>;


Suggested change

type TokenPriceSubject = Subject<{ tokens: TokenPrices } & Status>;

type TokenPriceSubject = BehaviorSubject<{ tokens: TokenPrices } & Status>;

This way you don't need the local priceData variable because you can access the last emited value: priceSubject$.value;

This is just an interface: the Observable used in the implementation is (ATM) actually a BehaviorSubject.
In case in the future we need to change the actual implementation, using the ancestor class Subject reduces the chances we need to change this interface type as well.

Sorry, I was not clear enough. I meant:

// have a local BehaviorSubject const price$ = new BehaviorSubject<{ tokens: TokenPrices } & Status>(); ... const updatePriceData = (assetId: string, data?: PriceData) => { const now = Date.now(); const nextPriceData = { ...price$.value, // access current value from the subject [assetId]: data ? [now, data] : [now], }; emitPrices(nextPriceData); ... const initCardanoTokenPrices = () => { storage.local .get(CACHE_KEY) .then((data) => { const initialPriceData = data[CACHE_KEY] || {}; emitPrices(initialPriceData); fetchPrices(); return price$; // expose local subject };

Then it could be assigned to coinPrices.tokenSubject$ in utilityService.ts

const coinPrices: CoinPrices = { tokenPrices$: initCardanoTokenPrices(), };

But again, this is just a suggestion. I am fine with either way.

Nice suggestion! Thank you.

5b33fc6 should solve this.

szymonmaslowski · 2025-05-19T11:20:01Z

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

+  updatePriceData(assetId, cachedData);
+
+  try {
+    const url = `https://coingecko.live-mainnet.eks.lw.iog.io/api/v3/onchain/networks/cardano/tokens/${assetId}/pools`;


Consider providing this URL via env variable

Suggested change

const url = `https://coingecko.live-mainnet.eks.lw.iog.io/api/v3/onchain/networks/cardano/tokens/${assetId}/pools`;

const url = `${process.env.TOKEN_PRICES_URL}/${assetId}/pools`;

e101d0c should solve this

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

szymonmaslowski · 2025-05-19T11:27:47Z

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

+  setTimeout(fetchPrices, TOKEN_PRICE_CHECK_INTERVAL);
+};
+
+export const initCardanoTokenPrices = (tokenPrices$: TokenPriceSubject) =>


Consider returning the local priceSubject$ instead of passing it from outside. This way the coinPrices.tokenPrices$ will get the proper types.

There is any locally defined priceSubject$, the only one defined is tokenSubject$ in utilityService.ts and it is defined as property of const coinPrices. The local variable priceSubject$ is just a reference to coinPrices.tokenSubject$ from utilityService.ts.
The reasons why I chosen this approach is to isolate the token price stuff in a dedicated file rather than implementing everything inside utilityService.ts which is already more than 300 lines.
We can evaluate importing the content of cardanoTokenPrices.ts into utilityService.ts, that would significantly simplify this point and some other; probably the type discussed in this comment will be completely removed as well.

przemyslaw-wlodek · 2025-05-19T11:39:07Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

+        const trackPrice = () =>
+          store && store.currentChain && store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet
+            ? trackCardanoTokenPrice(assetId).catch(console.error)
+            : // eslint-disable-next-line @typescript-eslint/no-empty-function
+              () => {};
+
+        // If the price for this token was never fetched, wee need to track it
+        if (!tokenPrice) {
+          trackPrice();
+
+          return undefined;
+        }


Just a suggestion. I think it would be more readable written as below. Currently you return an empty function or a promise - not sure why empty function instead of e.g. undefined (this empty/noop function is not getting called later).

edit: Part of this check will be even simpler networkId === Wallet.Cardano.NetworkId.Mainnet when my previous comment is applied

Suggested change

const trackPrice = () =>

store && store.currentChain && store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet

? trackCardanoTokenPrice(assetId).catch(console.error)

: // eslint-disable-next-line @typescript-eslint/no-empty-function

() => {};

// If the price for this token was never fetched, wee need to track it

if (!tokenPrice) {

trackPrice();

return undefined;

}

// If the price for this token was never fetched, wee need to track it

// but only for token in Cardano mainnet

if (!tokenPrice && store?.currentChain?.networkId === Wallet.Cardano.NetworkId.Mainnet) {

trackCardanoTokenPrice(assetId).catch(console.error)

return undefined;

}

The function trackPrice is called twice, next call is a few lines later.
I chosen this approach to reduce the code repetition as much as possible.

I know you call trackPrice, but not the function it returns here in line 60, so why do you need to return a function instead of e.g. undefined? Also this will be greatly simplified with #1867 (comment)

Mhh that is not correct.
trackPrice is a function which has trackCardanoTokenPrice(assetId).catch(console.error) as body if the wallet is in Cardano mainnet or an empty body in other networks: i.e. the function returned in line 60 is called (when the wallet is in preprod, preview or any bitcoin network)

Edit: please ignore this message, it is completely wrong

Thank you for insisting @przemyslaw-wlodek !
My previous message is probably a bias due to some previous implementations I did...

e101d0c should solve this

przemyslaw-wlodek · 2025-05-19T11:43:05Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

  const tokenPrices = useObservable(coinPrices.tokenPrices$);
  const adaPrices = useObservable(coinPrices.adaPrices$);
  const bitcoinPrices = useObservable(coinPrices.bitcoinPrices$);
+  const store = useWalletStore();


Please consider using a selector. You're passing whole store as a dependency for useMemo which will constantly recompute.

Suggested change

const store = useWalletStore();

const networkId = useWalletStore(state => state.currentChain?.networkId);

e101d0c should solve this

przemyslaw-wlodek · 2025-05-19T11:47:26Z

apps/browser-extension-wallet/src/hooks/useFetchCoinPrice.ts

      price: isAdaCurrency ? 1 : adaPrices?.prices?.[fiatCurrency.code.toLowerCase() as ADAPricesKeys],
      priceVariationPercentage24h:
        adaPrices?.prices?.[`${fiatCurrency.code.toLowerCase()}_24h_change` as ADAPricesKeys] || 0
    }),
-    [adaPrices?.prices, fiatCurrency.code, isAdaCurrency]
+    [adaPrices?.prices, fiatCurrency.code, isAdaCurrency, tokenPrices?.tokens, trackCardanoTokenPrice, store]


As described above, please pass just networkId to dependencies (instead of store

e101d0c should solve this

przemyslaw-wlodek · 2025-05-19T12:00:23Z

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

+  const [lastFetchTime, cachedData] = priceData[assetId] || [];
+
+  // If recently fetched, do nothing
+  if (typeof lastFetchTime === 'number' && lastFetchTime > Date.now() - TOKEN_PRICE_CHECK_INTERVAL) return;


Just a suggestion:

Suggested change

const [lastFetchTime, cachedData] = priceData[assetId] || [];

// If recently fetched, do nothing

if (typeof lastFetchTime === 'number' && lastFetchTime > Date.now() - TOKEN_PRICE_CHECK_INTERVAL) return;

const [lastFetchTime, cachedData] = priceData[assetId] || [0];

// If recently fetched, do nothing

if (lastFetchTime > Date.now() - TOKEN_PRICE_CHECK_INTERVAL) return;

e101d0c should solve this

przemyslaw-wlodek · 2025-05-19T12:02:44Z

apps/browser-extension-wallet/src/lib/scripts/background/services/cardanoTokenPrices.ts

+  updatePriceData(assetId, cachedData);
+
+  try {
+    const url = `https://coingecko.live-mainnet.eks.lw.iog.io/api/v3/onchain/networks/cardano/tokens/${assetId}/pools`;


I'd suggest moving core part of the URL to some constant + a function that accepts assetId and returns full URL.

Addressing this comment I did a little refactoring which could make this comment no longer required.

przemyslaw-wlodek

GJ! 🚀

szymonmaslowski

GJ!

sonarqubecloud · 2025-05-20T09:36:23Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.1% Duplication on New Code

See analysis details on SonarQube Cloud

iccicci force-pushed the feat/LW-12681-token-price branch 11 times, most recently from fcb3a0b to 8e077d9 Compare May 16, 2025 06:45

iccicci requested review from mchappell, szymonmaslowski, vetalcore and przemyslaw-wlodek May 16, 2025 06:46

iccicci marked this pull request as ready for review May 16, 2025 06:47

iccicci requested a review from a team as a code owner May 16, 2025 06:47

iccicci force-pushed the feat/LW-12681-token-price branch from 8e077d9 to f52263f Compare May 16, 2025 07:14

szymonmaslowski reviewed May 19, 2025

View reviewed changes

przemyslaw-wlodek reviewed May 19, 2025

View reviewed changes

przemyslaw-wlodek approved these changes May 19, 2025

View reviewed changes

szymonmaslowski approved these changes May 19, 2025

View reviewed changes

feat: coingecko token price API

3174dc3

iccicci force-pushed the feat/LW-12681-token-price branch from 5b33fc6 to 3174dc3 Compare May 20, 2025 09:34

iccicci merged commit 3174dc3 into main May 20, 2025
31 checks passed

iccicci deleted the feat/LW-12681-token-price branch May 20, 2025 09:57

	const tokenPrice = tokenPrices?.tokens?.get(assetId);
	const tokenPrice = tokenPrices.tokens.get(assetId);

	store && store.currentChain && store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet
	store.currentChain.networkId === Wallet.Cardano.NetworkId.Mainnet

		: // eslint-disable-next-line @typescript-eslint/no-empty-function
		() => {};

	: // eslint-disable-next-line @typescript-eslint/no-empty-function
	() => {};
	: () => void 0;

	type TokenPriceSubject = Subject<{ tokens: TokenPrices } & Status>;
	type TokenPriceSubject = BehaviorSubject<{ tokens: TokenPrices } & Status>;

	const url = `https://coingecko.live-mainnet.eks.lw.iog.io/api/v3/onchain/networks/cardano/tokens/${assetId}/pools`;
	const url = `${process.env.TOKEN_PRICES_URL}/${assetId}/pools`;

	const store = useWalletStore();
	const networkId = useWalletStore(state => state.currentChain?.networkId);

LW-12681 Coingecko token price API #1867

LW-12681 Coingecko token price API #1867

Conversation

iccicci commented May 13, 2025

Checklist

Proposed solution

Testing

Screenshots

Uh oh!

lace-bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Allure Report

Uh oh!

szymonmaslowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iccicci May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iccicci May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lace-bot commented May 13, 2025 •

edited

Loading

iccicci May 19, 2025 •

edited

Loading

iccicci May 19, 2025 •

edited

Loading