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

CVS live-countervalues performance evolutions #6771

Merged
merged 11 commits into from
May 6, 2024
Merged

CVS live-countervalues performance evolutions #6771

merged 11 commits into from
May 6, 2024

Conversation

gre
Copy link
Contributor

@gre gre commented Apr 29, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests. for most of them
  • Impact of the changes:
    • countervalues

πŸ“ Description

Implements https://ledgerhq.atlassian.net/browse/LIVE-11861 tasks
notably:

  • LIVE-11865: Add live-config fields
    • they are provided as new fields in CountervaluesSettings so the live-countervalues lib don't directly depend on live-config but on LLD/LLM we will inject them from countervalues_*
  • switch refreshRate to 1min default (remote config)
    • that way, the live price will be way more often up to date
  • LIVE-11864 introduce hybrid batch fetching mechanism
    • that will improve the performance of frontend<>backend by reducing the amount of Cloudflare cache efficiency
  • switch from /v3/currencies/supported to /v3/supported/crypto
  • closes chore(cvs): upgrade to v3 API for fetchSupportedFiatsTokens()Β #6768

❓ Context

  • JIRA or GitHub link: LIVE-11865 LIVE-11864 LIVE-11862

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@gre gre requested review from a team as code owners April 29, 2024 10:43
Copy link

vercel bot commented Apr 29, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
web-tools βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback May 2, 2024 3:48pm
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview May 2, 2024 3:48pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview May 2, 2024 3:48pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 2, 2024 3:48pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 2, 2024 3:48pm

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common cli labels Apr 29, 2024
@gre gre marked this pull request as draft April 29, 2024 13:28
@gre gre marked this pull request as ready for review April 29, 2024 14:20
@@ -72,7 +71,7 @@ async function initializeUserSupportedFiats() {

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

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

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.

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

@live-github-bot live-github-bot bot added the ledgerjs Has changes in the ledgerjs open source libs label May 2, 2024
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)

@gre gre merged commit 6c35cc5 into develop May 6, 2024
57 of 58 checks passed
@gre gre deleted the live-11861 branch May 6, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants