-
Notifications
You must be signed in to change notification settings - Fork 145
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: brc-20 token balances as fiat #5179
Conversation
c06934b
to
0b840b3
Compare
0b840b3
to
abbe247
Compare
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.
A lot of the currency conversion code, here and from before, looks really complicated to me.
To calculate a fiat amount, we need an amount Money
and rate MarketData
. If these come from different sources, can't we first turn those sources into the desired type, and use some pure function to handle the math?
In these functions we:
- Formatting
- Could handle directly in the components rather than hooks
- Performing math
- Pure functions, not specific to a data source
- Handling
undefined
case- Use a loader component, and pass values in as prop so they're always defined
This LGTM in general. I can re-check once you update as per Kyrans suggestions |
I believe we may completely remove hiro api only if continue to work with BiS pro tier |
52f1a7a
to
245dd8d
Compare
Hoping this is a better implementation for fiat values ...not sure why all the hooks were used originally ...prob my fault. |
currency: CryptoCurrencies, | ||
price: Money, |
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.
Shouldn't we use MarketData
type here?
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.
The createMarketData
is used inside this function, so you are saying here better to do outside and pass in? Not sure the function is really needed then, was trying to create a shared function to createMarketData
and then return baseCurrencyAmountInQuote
...but can just repeat the code where needed?
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.
Forgive the essay, but I think this is an important architectural consideration worth illustrating.
The idea here is that, to keep the app consistent and predictable, values should always be passed around in their designated model/data format.
In this instance, why isn't the currency/price data already MarketData
? Is it because it comes from an API that looks different, e.g. AlexSDK? When we get external data in to the app, at the earliest possible convenience, we should turn it into our own data models, for interoperability with existing code.
Here, we're creating new code to that works better with an external data source format, than our own. It effectively introduces a new paradigm for handling market data. Suppose we introduce another API with a slightly different format. Do we just use that, and have 3 ways of using handling market data?
This type was introduced because we pull market data from 3 sources, and we want a uniform way of handling the data.
When new app code circumvents that abstraction we have this
Further, with this function, because MarketData
is created internally, what if I actually have the format we want, the app MarketData
type. It's not compatible with this fn (just because it happened to be created when dealing with another format). I reckon it's better to offer the best API for the case we do want, rather than a format we don't.
I'd say the same if someone wrote a function that accepted amount
and symbol
, used createMoney
internally then performed some operation on it. It would better accept Money
directly.
Can't we modify the AlexSDK response and turn this into MarketData
right away? If we have to createMarketData
in 5 places to accept it as the input to this function, that's a better imo as we don't deviate from a standard format.
Happy to help figure out how we might do that
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 didn't understand we wanted to convert it so early, that is it. Your request to implement it this way isn't complicated. I converted their price
to our Money type right away, but didn't realize it would be better to go all-in on MarketData
. Happy to change it and will keep this in mind moving fwd.
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.
@kyranjamie I refactored this so now we select the query response and add the MarketData
right away for fiat balances. Let me know if this looks good now? I relocated some code from the original swaps work that makes those assets more accessible.
src/app/pages/swap/components/swap-asset-select/swap-asset-select-quote.tsx
Outdated
Show resolved
Hide resolved
245dd8d
to
0a572fc
Compare
@markmhendrickson this is ready to merge but is using |
Haha it's not anyones fault 😄 I just mentioned that as I saw all of the place we have so many hooks. Getting rid of some of them will help us move to a more API based approach I think. Good work! |
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 🚀
@fbwoolf FYI we can now push usage of the |
d8a6385
to
4efcc2a
Compare
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.
Nice refactor! This is much tidier IMO
2c3a093
to
d14d318
Compare
7478945
to
e170ee7
Compare
e170ee7
to
a15ff27
Compare
a15ff27
to
96166e0
Compare
This PR changes to use
leatherapi
for BIS and implements fiat values for BRC-20 tokens. I did some refactoring here bc I had inadvertently made some functions for swapping with the alex-sdk too specific when they can be used by other apis for formatting fractional units and base units.See changes in
use-convert-to-fiat-amount.ts
, there are now hooks foruseFormattedFractionalUnitAsFiat
anduseFormattedBaseUnitAsFiat
for fiat.Note here, I am still unclear if I am supposed to completely remove our use of HiroApi? cc @alter-eggo