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.

Sign up for GitHub

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

Jump to bottom

CVS live-countervalues performance evolutions #6771

Merged
merged 11 commits into from
May 6, 2024
Merged

CVS live-countervalues performance evolutions #6771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
98750d9
chore(countervalues): Add live-config fields
gre Apr 29, 2024
138bae7
chore(countervalues): switch refreshRate to 1min default (remote conf)
gre Apr 29, 2024
04acd6f
chore(countervalues): introduce hybrid batch fetching mecanism
gre Apr 29, 2024
5adf9d3
chore(countervalues): switch to /v3/supported/crypto
gre Apr 29, 2024
5656cfc
chore: changeset
gre Apr 29, 2024
9a7a798
chore(cvs): upgrade to v3 API for fetchSupportedFiatsTokens()
gre Apr 26, 2024
5bd0d05
chore: remove useless log
gre Apr 30, 2024
83190a7
chore: add all missing fiats (and apply fixes from currency changes)
gre May 2, 2024
0999e74
Merge pull request #6798 from LedgerHQ/live-12456
gre May 2, 2024
166f80a
chore: fix tests
gre May 2, 2024
63386e4
chore: fix fiat loading logic to handle new coins that aren't in loca…
gre May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changeset/beige-starfishes-guess.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@ledgerhq/cryptoassets": patch
"@ledgerhq/live-countervalues-react": patch
"ledger-live-desktop": patch
"live-mobile": patch
"@ledgerhq/live-common": patch
"@ledgerhq/live-countervalues": patch
"@ledgerhq/coin-framework": patch
"@ledgerhq/live-cli": patch
---

LL's preferred countervalues will now have all the possible fiats that our CVS api supports.
8 changes: 8 additions & 0 deletions .changeset/green-bags-hunt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@ledgerhq/live-countervalues-react": patch
"ledger-live-desktop": patch
"@ledgerhq/live-countervalues": patch
"@ledgerhq/coin-framework": patch
---

Countervalues API: upgrade to v3 for fetching supported fiats
10 changes: 10 additions & 0 deletions .changeset/swift-penguins-act.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"@ledgerhq/live-countervalues-react": patch
"ledger-live-desktop": patch
"live-mobile": patch
"@ledgerhq/live-common": patch
"@ledgerhq/live-countervalues": patch
"@ledgerhq/live-cli": patch
---

Countervalues performance evolutions. (8min -> 1min refresh rate, more efficient http calls caching,..)
2 changes: 2 additions & 0 deletions apps/cli/src/commands/blockchain/botTransfer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ export default {
const countervaluesState = await loadCountervalues(initialState, {
trackingPairs: inferTrackingPairForAccounts(accounts, countervalue),
autofillGaps: true,
refreshRate: 60000,
marketCapBatchingAfterRank: 20,
});

await promiseAllBatched(CONCURRENT, accounts, async account => {
Expand Down
2 changes: 2 additions & 0 deletions apps/cli/src/commands/live/countervalues.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ export default {
})),
),
autofillGaps: !opts.disableAutofillGaps,
refreshRate: 60000,
marketCapBatchingAfterRank: 20,
});
// eslint-disable-next-line no-console
if (opts.verbose) console.log(cvs);
Expand Down
2 changes: 2 additions & 0 deletions apps/cli/src/commands/live/portfolio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ export default {
loadCountervalues(initialState, {
trackingPairs: inferTrackingPairForAccounts(accounts, countervalue as Currency),
autofillGaps: !opts.disableAutofillGaps,
refreshRate: 60000,
marketCapBatchingAfterRank: 20,
}),
).pipe(
map(state => {
Expand Down
5 changes: 5 additions & 0 deletions apps/ledger-live-desktop/src/renderer/actions/general.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
import { resolveTrackingPairs } from "@ledgerhq/live-countervalues/logic";
import { useExtraSessionTrackingPair } from "./deprecated/ondemand-countervalues";
import { useMarketPerformanceTrackingPairs } from "./marketperformance";
import { LiveConfig } from "@ledgerhq/live-config/LiveConfig";

// provide redux states via custom hook wrapper

Expand Down Expand Up @@ -130,6 +131,10 @@ export function useCalculateCountervaluesUserSettings(): CountervaluesSettings {
() => ({
trackingPairs,
autofillGaps: true,
refreshRate: LiveConfig.getValueByKey("countervalues_refreshRate"),
marketCapBatchingAfterRank: LiveConfig.getValueByKey(
"countervalues_marketCapBatchingAfterRank",
),
}),
[trackingPairs],
);
Expand Down
124 changes: 62 additions & 62 deletions apps/ledger-live-desktop/tests/specs/market/market.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,71 +52,71 @@ test("Market", async ({ page }) => {
await expect.soft(page).toHaveScreenshot("market-page-no-scrollbar.png", maskItems);
});

await page.route(`${getEnv("LEDGER_COUNTERVALUES_API")}/v2/supported-to`, async route => {
await page.route(`${getEnv("LEDGER_COUNTERVALUES_API")}/v3/supported/fiat`, async route => {
route.fulfill({
headers: { teststatus: "mocked" },
body: JSON.stringify([
"aed",
"ars",
"aud",
"bch",
"bdt",
"bhd",
"bits",
"bmd",
"bnb",
"brl",
"btc",
"cad",
"chf",
"clp",
"cny",
"czk",
"dkk",
"dot",
"eos",
"eth",
"eur",
"gbp",
"hkd",
"huf",
"idr",
"ils",
"inr",
"jpy",
"krw",
"kwd",
"link",
"lkr",
"ltc",
"mmk",
"mxn",
"myr",
"ngn",
"nok",
"nzd",
"php",
"pkr",
"pln",
"rub",
"sar",
"sats",
"sek",
"sgd",
"thb",
"try",
"twd",
"uah",
"usd",
"vef",
"vnd",
"xag",
"xau",
"xdr",
"xlm",
"xrp",
"yfi",
"zar",
"AED",
"ARS",
"AUD",
"BCH",
"BDT",
"BHD",
"BITS",
"BMD",
"BNB",
"BRL",
"BTC",
"CAD",
"CHF",
"CLP",
"CNY",
"CZK",
"DKK",
"DOT",
"EOS",
"ETH",
"EUR",
"GBP",
"HKD",
"HUF",
"IDR",
"ILS",
"INR",
"JPY",
"KRW",
"KWD",
"LINK",
"LKR",
"LTC",
"MMK",
"MXN",
"MYR",
"NGN",
"NOK",
"NZD",
"PHP",
"PKR",
"PLN",
"RUB",
"SAR",
"SATS",
"SEK",
"SGD",
"THB",
"TRY",
"TWD",
"UAH",
"USD",
"VEF",
"VND",
"XAG",
"XAU",
"XDR",
"XLM",
"XRP",
"YFI",
"ZAR",
]),
});
});
Expand Down
5 changes: 5 additions & 0 deletions apps/ledger-live-mobile/src/actions/general.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { accountsSelector } from "../reducers/accounts";
import { counterValueCurrencySelector, orderAccountsSelector } from "../reducers/settings";
import { clearBridgeCache } from "../bridge/cache";
import { flushAll } from "../components/DBSave";
import { LiveConfig } from "@ledgerhq/live-config/LiveConfig";

const extraSessionTrackingPairsChanges: BehaviorSubject<TrackingPair[]> = new BehaviorSubject<
TrackingPair[]
Expand Down Expand Up @@ -102,6 +103,10 @@ export function useUserSettings() {
() => ({
trackingPairs,
autofillGaps: true,
refreshRate: LiveConfig.getValueByKey("countervalues_refreshRate"),
marketCapBatchingAfterRank: LiveConfig.getValueByKey(
"countervalues_marketCapBatchingAfterRank",
),
}),
[trackingPairs],
);
Expand Down
12 changes: 5 additions & 7 deletions libs/coin-framework/src/currencies/support.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
getFiatCurrencyByTicker,
getCryptoCurrencyById,
hasCryptoCurrencyId,
hasFiatCurrencyTicker,
} from "@ledgerhq/cryptoassets";
import { CryptoCurrency, CryptoCurrencyId, FiatCurrency } from "@ledgerhq/types-cryptoassets";
import { getEnv } from "@ledgerhq/live-env";
Expand Down Expand Up @@ -56,23 +57,20 @@ async function initializeUserSupportedFiats() {
let supportedTokens = [] as string[];

if (remoteSupportedTokens.length !== 0) {
remoteSupportedTokens.forEach(id => {
const token = id.toUpperCase();
if (locallySupportedFiats.includes(token)) {
remoteSupportedTokens.forEach(token => {
if (hasFiatCurrencyTicker(token)) {
supportedTokens.push(token);
}
});
} else {
supportedTokens = locallySupportedFiats;
}
userSupportedFiats = supportedTokens.map(id => {
return getFiatCurrencyByTicker(id);
});
userSupportedFiats = supportedTokens.map(getFiatCurrencyByTicker);
}

export async function fetchSupportedFiatsTokens(): Promise<string[]> {
try {
const response = await fetch(`${getEnv("LEDGER_COUNTERVALUES_API")}/v2/supported-to`, {
const response = await fetch(`${getEnv("LEDGER_COUNTERVALUES_API")}/v3/supported/fiat`, {
Copy link
Contributor

@jnicoulaud-ledger jnicoulaud-ledger May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I thought we can't do this move yet because the market page does not support all the extra fiats until moved to /v3/markets API.

But when I tried on a build of this PR I still see the "short" list in the currency selection menu, so maybe I am missing something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jnicoulaud-ledger

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha OK, that explains. But if my understanding is correct, market inherits the globally set currency so we are limited by what market supports.

For the harcoded data, I guess we could expose it in an API, but it's ISO-4217 standard, there are libraries that provide this metadata: https://www.npmjs.com/search?q=keywords:iso4217

Copy link
Contributor Author

@gre gre May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jnicoulaud-ledger #6798
for now, i think we can survive with this, especially we have some local modification needed for LL (e.g. using a shorter version for some fiat name to fit in the UI..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've merged the additions of fiats so we can test more. i seems to have a subset of fiat that all works on market. do you have a specific fiat that works on the new api but not on the old?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's the diff between https://countervalues.live.ledger.com/v2/supported-to and https://countervalues.live.ledger.com/v3/supported/fiat, so for instance: AFN, ALL, AMD...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, after testing, i can confirm that market uses a totally different list , so we are fine with these changes. however, I had to do extra fix to actually make Live instance support the new list.

note for ourself: coin-framework also hardcode a locallySupportedFiats list, which i suspect market uses, so i'm not touching this, but in future we will need to move this out of coin-framework. ( cc @sprohaszka-ledger fyi )

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested again:

  • On prod build global currency and market are always synced
  • On this PR, market currency is always USD even if I set a global currency that is supported (EUR)

I would at least expect market currency to follow the global one when possible.

But also I remember last time this was discussed, it was considered not acceptable to have a different set of supported currencies globally and for market, so we might want to double check this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would at least expect market currency to follow the global one when possible.

it doesn't seem it's the case today, and this PR didn't have the scope to address this. therefore if things are not connected i'm not sure if it's going to be a problem to have different list on both sides. indeed it's not ideal but i'm not seeing it a blocker to do this first step.
I'll ask product on monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seen with Anthony, we're good to have this evolution even if market is disconnected for the time being

method: "GET",
headers: {
accept: "application/json",
Expand Down
2 changes: 2 additions & 0 deletions libs/ledger-live-common/src/__tests__/csvExport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ test("export CSV", async () => {
startDate: new Date(Date.now() - 200 * 24 * 60 * 60 * 1000),
})),
autofillGaps: false,
refreshRate: 60000,
marketCapBatchingAfterRank: 20,
});
const accounts = currencies.map(currency =>
genAccount(`${currency.id}_export`, {
Expand Down
2 changes: 2 additions & 0 deletions libs/ledger-live-common/src/bot/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ export async function bot({ disabled, filter }: Arg = {}): Promise<void> {
const countervaluesState = await loadCountervalues(initialState, {
trackingPairs: inferTrackingPairForAccounts(allAccountsAfter, usd),
autofillGaps: true,
refreshRate: 60000,
marketCapBatchingAfterRank: 20,
}).catch(e => {
if (process.env.CI) console.error(e);
countervaluesError = e;
Expand Down
12 changes: 12 additions & 0 deletions libs/ledger-live-common/src/config/sharedConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,23 @@ import { tronConfig } from "../families/tron/config";
import { vechainConfig } from "../families/vechain/config";
import { appConfig } from "../apps/config";

const countervaluesConfig: ConfigSchema = {
countervalues_refreshRate: {
type: "number",
default: 60 * 1000,
},
countervalues_marketCapBatchingAfterRank: {
type: "number",
default: 20,
},
};

const liveCommonConfig: ConfigSchema = {
...appConfig,
};

export const liveConfig: ConfigSchema = {
...countervaluesConfig,
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be a part of liveCommonConfig above, minor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reasoning behind this is that "countervalues" is a dedicated library and not part of "live-common"

(ultimately the existence of this file that glue things together manually is problematic imo but it's another topic)

...liveCommonConfig,
...algorandConfig,
...bitcoinConfig,
Expand Down
Loading
Loading