-
Notifications
You must be signed in to change notification settings - Fork 15
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
Refactor selections/balances to use DenomMetadata
, and to render a ValueViewComponent when possible
#440
Conversation
22db3d0
to
9693bb5
Compare
export const displayDenom = (res?: AssetsResponse): { display: string; exponent: number } => { | ||
const display = res?.denomMetadata?.display; | ||
if (!display) return { display: 'unknown', exponent: 0 }; | ||
|
||
const match = res.denomMetadata?.denomUnits.find(d => d.denom === display); | ||
if (!match) return { display, exponent: 0 }; | ||
|
||
return { display, exponent: match.exponent }; | ||
}; |
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.
Eliminated this helper function entirely, as it just takes an AssetsResponse
(a thin wrapper around DenomMetadata
) and returns just the display denom's exponent.
Going forward, we want to use the ValueViewComponent
to display balances, which requires an entire DenomMetadata
object. So we don't want to just strip a few properties out of that object here.
f0efaad
to
72a046c
Compare
9693bb5
to
abca983
Compare
d144bd8
to
4f46a9f
Compare
DenomMetadata
, and to render a ValueViewComponent when possible
@@ -17,7 +18,7 @@ export const ValueViewComponent = ({ view }: ValueViewPrpos) => { | |||
const encodedAssetId = bech32AssetId(value.assetId!); | |||
return ( | |||
<div className='flex font-mono'> | |||
<p className='text-[15px] leading-[22px]'>{fromBaseUnitAmount(amount, 1).toFormat()}</p> | |||
<p className='text-[15px] leading-[22px]'>{fromBaseUnitAmount(amount, 0).toFormat()}</p> |
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'm like... 90% sure that using 1
for the default exponent for an unknown denomination is a mistake. An exponent of 1
means we're multiplying amount
by "10 to the power of 1" — i.e., we're multiplying it by 10.
We should (right?!) be multiplying amount
"10 to the power of 0" (i.e., multiplying it by 1
) when we don't know the denomination.
Reviewers, please check my logic!
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 believe this is correct, but i would confirm by viewing a value through this and then comparing it with the same value viewed with pcli
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.
Ah good catch!
@@ -36,8 +37,8 @@ export const ValueViewComponent = ({ view }: ValueViewPrpos) => { | |||
const value = view.valueView.value; | |||
const amount = value.amount ?? new Amount(); | |||
const display_denom = value.denom?.display ?? ''; | |||
// The first denom unit in the list is the display denom, according to cosmos practice | |||
const exponent = value.denom?.denomUnits[0]?.exponent ?? 1; | |||
const exponent = value.denom ? getDisplayDenomExponent(value.denom) : 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.
Same issue here, changing the exponent from 1 to 0.
Also, the comment which I deleted says:
The first denom unit in the list is the display denom, according to cosmos practice
However, I'm not sure this is correct. Cosmos's own ADR docs contain an example asset where the first denom is not the display denom. So, to be safe, I'm calling getDisplayDenomExponent
to make sure we're getting the right exponent.
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 agree it's preferable to certainly identify it rather than assume, especially since it doesn't seem to be an expensive search
|
||
export const joinLoHiAmount = (amount: Amount): bigint => { | ||
return joinLoHi(amount.lo, amount.hi); | ||
}; | ||
|
||
export const fromBaseUnitAmount = (amount: Amount, exponent: number): BigNumber => { | ||
export const fromBaseUnitAmount = (amount: Amount, exponent = 0): BigNumber => { |
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.
Since we're occasionally passing in an empty DenomMetadata
upstream of this call, I'll leave the default exponent as 0
, so that we're just multiplying by 1
.
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.
If we do error guards, I'm not sure we'll ever run into the default case 🤔 . If we don't have the denomData that corresponds to the amount, we probably should just throw right? (given it means something went wrong?)
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.
@grod220 hmm yeah, possibly. I think the case where we'd have missing denomMetadata is when we have a transaction but we don't have denom metadata for its asset type — which, if I understand correctly how Penumbra works, should never happen, right? In which case you're right that we should throw (somewhere in fetchers/balances.ts
).
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.
This comment might be clarifying: cosmos/cosmos-sdk#19298 (comment)
It's possible that finer information about the denom might be missing. The UX design challenge here is figuring out what the right thing to do in that case is -- I think we should think of the extension as being the user's "user agent", acting on their behalf to help them understand tokens, and showing unknown tokens as "Unknown".
@@ -76,7 +76,7 @@ export const fromBaseUnit = (lo = 0n, hi = 0n, exponent: number): BigNumber => { | |||
* @param {number} exponent - The exponent to be applied. | |||
* @returns {LoHi} An object with properties `lo` and `hi`, representing the low and high 64 bits of the multiplied value. | |||
*/ | |||
export const toBaseUnit = (value: BigNumber, exponent: number): LoHi => { | |||
export const toBaseUnit = (value: BigNumber, exponent = 0): LoHi => { |
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.
Since we're occasionally passing in an empty DenomMetadata
upstream of this call, I'll leave the default exponent as 0
, so that we're just multiplying by 1
.
@@ -17,7 +18,7 @@ export const ValueViewComponent = ({ view }: ValueViewPrpos) => { | |||
const encodedAssetId = bech32AssetId(value.assetId!); | |||
return ( | |||
<div className='flex font-mono'> | |||
<p className='text-[15px] leading-[22px]'>{fromBaseUnitAmount(amount, 1).toFormat()}</p> | |||
<p className='text-[15px] leading-[22px]'>{fromBaseUnitAmount(amount, 0).toFormat()}</p> |
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 believe this is correct, but i would confirm by viewing a value through this and then comparing it with the same value viewed with pcli
@@ -36,8 +37,8 @@ export const ValueViewComponent = ({ view }: ValueViewPrpos) => { | |||
const value = view.valueView.value; | |||
const amount = value.amount ?? new Amount(); | |||
const display_denom = value.denom?.display ?? ''; | |||
// The first denom unit in the list is the display denom, according to cosmos practice | |||
const exponent = value.denom?.denomUnits[0]?.exponent ?? 1; | |||
const exponent = value.denom ? getDisplayDenomExponent(value.denom) : 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.
i agree it's preferable to certainly identify it rather than assume, especially since it doesn't seem to be an expensive search
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 job! Left some comments about taking this further in #427. I'd love to integrate this work and build off of it. I'm going to do something I never do approve+merge 😱 (maybe it should be the standard? haha) and rebase this against my other branch and continue it from there.
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.
Some overlap with: #427. But no worries! I'll definitely make use of your work. I think the end state probably looks like this:
export interface AccountBalance {
value: ValueView;
address: AddressView;
usdcValue: number;
}
export const getDisplayDenomExponent = (denomMetadata: DenomMetadata): number | undefined => | ||
denomMetadata.denomUnits.find(denomUnit => denomUnit.denom === denomMetadata.display)?.exponent; |
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.
Display denom should always be in the denomUnits array right? Can we guard with an error so it's a guaranteed output?
export const getDisplayDenomExponent = (denomMetadata: DenomMetadata): number => {
const match = denomMetadata.denomUnits.find(
denomUnit => denomUnit.denom === denomMetadata.display,
);
if (!match)
throw new Error(
`Could not find display denom ${denomMetadata.display} in the denomUnits array`,
);
return match.exponent;
};
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.
Good call. If it's OK, I'll leave that up to you in your PR.
|
||
export const joinLoHiAmount = (amount: Amount): bigint => { | ||
return joinLoHi(amount.lo, amount.hi); | ||
}; | ||
|
||
export const fromBaseUnitAmount = (amount: Amount, exponent: number): BigNumber => { | ||
export const fromBaseUnitAmount = (amount: Amount, exponent = 0): BigNumber => { |
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.
If we do error guards, I'm not sure we'll ever run into the default case 🤔 . If we don't have the denomData that corresponds to the amount, we probably should just throw right? (given it means something went wrong?)
{fromBaseUnitAmountAndDenomMetadata( | ||
asset.amount, | ||
asset.denomMetadata, | ||
).toFormat()} |
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.
These are places we'd want to use ValueViewComponent
as well. However, I'll pick up this work in: #427. This would require more refactoring that is currently in progress in balances.ts.
const getCurrentBalanceValueView = (assetBalance: AssetBalance | undefined): ValueView => { | ||
if (assetBalance?.denomMetadata) | ||
return new ValueView({ | ||
valueView: { | ||
case: 'knownDenom', | ||
value: { amount: assetBalance.amount, denom: assetBalance.denomMetadata }, | ||
}, | ||
}); | ||
else if (assetBalance?.assetId) | ||
return new ValueView({ | ||
valueView: { | ||
case: 'unknownDenom', | ||
value: { amount: assetBalance.amount, assetId: assetBalance.assetId }, | ||
}, | ||
}); | ||
else return new ValueView(); | ||
}; |
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.
This is also something we'll migrate to balances.ts so components won't need to have to do this.
@@ -17,7 +18,7 @@ export const ValueViewComponent = ({ view }: ValueViewPrpos) => { | |||
const encodedAssetId = bech32AssetId(value.assetId!); | |||
return ( | |||
<div className='flex font-mono'> | |||
<p className='text-[15px] leading-[22px]'>{fromBaseUnitAmount(amount, 1).toFormat()}</p> | |||
<p className='text-[15px] leading-[22px]'>{fromBaseUnitAmount(amount, 0).toFormat()}</p> |
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.
Ah good catch!
This started as a change to how we render a user's balance on the Send form. @grod220 pointed out that we should be using a
ValueViewComponent
, as that is the standard for how we renderValueView
s throughout the app.As I worked on this, though, I realized this change wasn't possible without also changing the form of data that we pass around in
apps/webapp/src/fetchers/balances.ts
. So I refactored the helpers in that file so that we could work with fullDenomMetadata
objects, rather than just a few properties pulled off of those objects.When I did this, I also realized there were a few places where we were making incorrect assumptions and calculations about how to render financial values! I'll comment on those in the code so you can check my work, but I believe I corrected them.
In this PR
AssetBalance
type to convert thedenom
property to a fulldenomMetadata
property containing all metadata about the asset.ValueViewComponent
where the default exponent was assumed to be1
, when it should be0
. (Reviewers, please double-check this logic — I'll leave a comment where I'm referring to).ValueViewComponent
to ensure we're rendering financial figures correctly.