feat: render dollar values #1227
Conversation
euharrison
requested review from
pedrorezende,
mateuszjasiuk,
jurevans and
emccorson
apps/namadillo/src/utils/registry.ts
Outdated
| * @param denom | ||
| * @returns The expoent, or zero if not found | ||
| */ | ||
| export const findExpoent = (asset: Asset, denom: string): number => |
There was a problem hiding this comment.
expoent -> exponent (also in other places)
| const asset = assetsByDenom?.[denom] ?? unknownAsset; | ||
| const display = asset.display; | ||
|
|
||
| const expoentInput = findExpoent(asset, denom); |
There was a problem hiding this comment.
This section is like a more general version of what toDisplayAmount in utils/index.ts does. The only difference is that toDisplayAmount assumes one of the exponents is 0. Maybe we could make that accept a second exponent and reuse it here? Then you could remove most of this code as well as findExponent.
| // For now, we are transforming the api returned value from `namnam` to `nam`. | ||
| return response.map((item) => | ||
| item.tokenAddress === namTokenAddressQuery.data ? | ||
| { ...item, balance: BigNumber(item.balance).shiftedBy(-6).toString() } |
There was a problem hiding this comment.
Is it possible to keep balance as a BigNumber here? I think it just gets converted back to a BigNumber later anyway.
| account: Account | undefined | ||
| ): Promise<Balance[]> => { | ||
| if (!account) return []; | ||
| const balancesResponse = await api.apiV1AccountAddressGet(account.address); |
There was a problem hiding this comment.
Maybe I'm being paranoid, but we could do the conversion to NAM in this function instead of the atom to make sure the namnam balances never escape anywhere 😄 It would be the same way as fetchNamAccountBalance does it.
There was a problem hiding this comment.
@emccorson I didn't update here because the current namadaAsset.base is namnam, and it'll create a cascade of issues. In case you prefer to change the namadaAsset.base to be nam, I can shift this value returned from the indexer api :)
| assets.assets.forEach((asset) => { | ||
| asset.denom_units.forEach((unit) => { | ||
| // only set the asset if it doesn't exists | ||
| // otherwise the testnet will override the mainnet |
There was a problem hiding this comment.
I just realised both this PR and #1228 are trying to decode assets from their IBC trace, but in different ways. Maybe we can use just one way, but we'll need to decide which way.
Like you said in this comment, if the asset exists on multiple chains, we will only use the first entry, but the problem with this is that different chains have different configurations for the same asset (and not just testnets).
I'll write it up a bit more in a message so we can discuss it.
|
|
||
| // TODO upgrade this function to be as smart as possible | ||
| // please refer to `tryCoinToIbcAsset` and how we could combine both | ||
| export const findAssetByToken = ( |
There was a problem hiding this comment.
It seems that this function takes an IBC trace such as transfer/channel-0/uatom and checks if there is any known asset with the same trace on any chain. But I think this only works by coincidence because there happens to be a channel-0 on Osmosis with counterparty Cosmoshub, and for other tokens we won't find anything. Or in other words, Namada does have a channel-0 (corresponding to Cosmoshub testnet) but this is a different thing from channel-0 on Osmosis (corresponding to Cosmoshub mainnet).
But I think we can leave this as is for now, and I can integrate the two different methods once #1228 is merged.
There was a problem hiding this comment.
Thanks! Exactly this! 🚀
Render dollar values on Masp Overview and the Account Overview
We are using the https://sqs.osmosis.zone/swagger/index.html api for the price conversion
Closes #1171
Ref #1172