-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat: stacks ft fiat values from alex-sdk #5151
Conversation
2880d0a
to
e82f052
Compare
e82f052
to
3b72cbb
Compare
Also, only partially related, but how are we sorting tokens at the moment? I presume best to sort by total fiat value descending, then alphabetically ascending. cc @fabric-8 |
src/app/features/asset-list/components/stacks-balance-list-item.tsx
Outdated
Show resolved
Hide resolved
Nice work, this LGTM, I just added some suggestions but nothing critical |
I don't think the intention was to add all to the total balance, was it? I believe it is just Bitcoin and Stacks in that total? I think that would be an enhancement issue? |
We can tackle in a separate issue / PR if preferred, though it seems like an expected enhancement to me if we show fiat per Stacks FT. Otherwise we'll have an incongruence. |
I was more questioning, is that really what the total balance is supposed to mean? It isn't really set up in our code that way. Isn't that misleading when other tokens have value we just don't have market data for them? There will always be an |
Got it – perhaps @fabric-8 should chime in here. My sense is that it's best to optimistically include all known fiat values into the total account value, since it at least provides a floor value for the entire account. If we detect there's one or missing fiat values, we could theoretically enhance this approach by indicating the total account value with a lower-range e.g. If I see a smaller account total than the sum of the token values as listed, it's just going to seem broken (esp. since here's no way to understand as a user why only BTC and STX are included?). |
I generally agree with @markmhendrickson — If we display a fiat value for any given token, that value should also contribute to the total balance. Regarding sorting: +1 for sorting by fiat value in descending order as a default |
Agree that the total should sum all known balances, but @fbwoolf is right the code isn't really set up well for this. Suggest doing as a separate job, and rethinking the whole balance architecture. |
Made a new issue here: #5159 |
1917bb5
to
b1e0ab9
Compare
b1e0ab9
to
aa1f38b
Compare
This PR adds fiat balances for supported currencies from the alex-sdk. If no data is known, then the fiat balance will still show nothing. We should address hiding many of these airdropped tokens soon bc our asset list is getting quite long.
Also, I encountered some unnecessary code for the Fund page that I have refactored here.