-
Notifications
You must be signed in to change notification settings - Fork 315
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
Changes from all commits
98750d9
138bae7
04acd6f
5adf9d3
5656cfc
9a7a798
5bd0d05
83190a7
0999e74
166f80a
63386e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
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 |
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,..) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be a part of liveCommonConfig above, minor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnicoulaud-ledger
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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..)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
...There was a problem hiding this comment.
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 )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested again:
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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