-
Notifications
You must be signed in to change notification settings - Fork 5
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: balances service, closes LEA-1707 LEA-1793 LEA-1761 #671
Conversation
dc9405a
to
052f083
Compare
return useQueries({ queries, combine }); | ||
const { isLoading, data } = useBtcBalanceQuery(descriptors); | ||
return { | ||
availableBalance: !isLoading && data ? data.balanceBtc.availableBalance : createMoney(0, 'BTC'), |
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.
Can we return null
/ undefined
here if !data
?
We need to get rid of these 0 balance fallbacks and handle them more gracefully, only showing 0 if we have data and it is 0
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.
@pete-watters Because the output of this query is already being used in several places, all of which would need to be refactored to accept and propogate Money | null
, maybe this is better-suited for a follow-on where we can rethink how to propogate loading states (or nulls) into balance components?
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.
Sounds fine with me, just wanted to mention it. It can be tackled in https://linear.app/leather-io/issue/LEA-1726/handle-balance-loading-state-elegantly
protectedBalanceBtc, | ||
uneconomicalBalanceBtc | ||
), | ||
balanceUsd: createBtcCryptoAssetBalance( |
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.
Do we know for sure that we always want the fiat balance in USD? I know we have a settings pref for conversion unit, so are we intending for this to be customizable?
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.
In the future we want to support multi currencies but that setting is not connected yet and I started deliberately labelling balances with USD
to make this clearer as in i18nFormatCurrency
we have it hardcoded to only accept USD anyway:
export function i18nFormatCurrency(quantity: Money, decimals: number = 2) {
if (quantity.symbol !== 'USD') throw new Error('Cannot format non-USD amounts');
const currencyFormatter = new Intl.NumberFormat('en-US', {
I agree it should be more scalable and offer alternative currencies but that's a whole other initiative and for now I think balanceUsd
is more apt as it can't be anything else
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.
@alexp3y - is the market data currently only supporting USD anyway?
Just thinking we could add a project for updating both extension
+ mobile
to support multi currency in the future. (Probably far in the future)
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.
@fbwoolf @pete-watters At the moment, yeah. USD is the default (and sometimes only) option returned by the pricing APIs used by MarketDataService
, so I didn't add support for customisation.
When I go through the rest of the balances work I'll investigate and spec out what it will take to accommodate the fiat preference setting in @leather.io/services
(i.e. balances and market data).
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.
Great work here @alexp3y 🎉
It would be good to add some unit tests while things are fresh in your mind but I'm happy to merge as is and add those when migrating Stacks balances here.
For now this is a big improvement so I think we should merge include it to a release candidate for Leatherhood
Implements a BTC balance service and applies it in
mobile
.CryptoAssetBalance
model.LeatherApiClient
We will continue to extend service-based balances functionality from here, including the incorporation of STX and other asset balances, and additional UTXO filtering logic.
A modest reorganization of the
CryptoAssetBalance
model is included to move globally shared sub-balance fields (total, inbound, outbound, pending, available) to the base entity, and keep only asset-specific sub-balances on child entities e.g. (BTC -> protected, uneconomical) and (STX -> locked, unlocked, availableUnlocked).Issue LEA-1793 has been corrected incidentally by refactoring to use the new BTC balance service.
This also fixes the Market Data decimal precision issue LEA-1761. We represent all balance amounts in the smallest unit of the currency (sats, cents, etc..). The fix ensures that native asset pricing information fetched by third-party API (usually represented in USD) are faithfully converted to cents in the
MarketDataService
.