Skip to content
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

fix: runes formatted balance, closes #5293 #5313

Merged
merged 3 commits into from
Apr 26, 2024
Merged

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Apr 24, 2024

Try out Leather build 4a67a60Extension build, Test report, Storybook, Chromatic

This PR fixes a UI bug when we formatted the runes (and BRC-20) token balance. We needed to convert the amount to the base unit first, my bad.

@fbwoolf fbwoolf linked an issue Apr 24, 2024 that may be closed by this pull request
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Apr 24, 2024

cc @brandonmarshall-tm this should be correct now?

Screenshot 2024-04-24 at 1 48 53 PM

Screenshot 2024-04-24 at 1 24 23 PM

EDIT: Previously...
Screenshot 2024-04-24 at 1 22 53 PM

@brandonmarshall-tm
Copy link

brandonmarshall-tm commented Apr 24, 2024

Looks great! The USD balances for BRC-20 look to be incorrect in this build, I thought I was rich for a second! 😂 But that's a separate issue.

What is the issue bc that was just released? Are they not correct? What are you seeing?

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Apr 24, 2024

@brandonmarshall-tm sorry, I think I edited your comment by mistake. Pretty confident I did a quote reply thou 🤷‍♀️

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Apr 24, 2024

Added a fix here for: #4408 (comment)

Screenshot 2024-04-24 at 4 29 34 PM

@fbwoolf fbwoolf force-pushed the fix/runes-formatted-balance branch from 6351442 to 79d94e9 Compare April 25, 2024 00:30
Copy link
Contributor

@pete-watters pete-watters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing Fara.

Some spring cleaning here that'd be great if not too much work: the issue with formatbalance not accepting Money.

Also, please take the time to write a test that ensures we display Rune and BRC 20 balances correctly.

Comment on lines 15 to 17
const balanceAsString = convertAmountToBaseUnit(rune.balance).toString();
const formattedBalance = formatBalance(balanceAsString);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic, why doesn't formatBalance accept Money?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I can change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really not sure I understand needing to change this when we need the string to display at this point? Why keep it as Money just to display it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questioning only bc balanceAsString is used in more than one place and is done this way in several components to handle the formatting w/ tooltip.

src/app/query/bitcoin/ordinals/brc20/brc20-tokens.hooks.ts Outdated Show resolved Hide resolved
src/app/query/bitcoin/ordinals/brc20/brc20-tokens.hooks.ts Outdated Show resolved Hide resolved
@314159265359879
Copy link
Contributor

When the number is very high I think we should ditch the "T" suffix and only show e+XX instead of e+_XX_T
image
...e+38 instead of ...e+26T

@fbwoolf fbwoolf force-pushed the fix/runes-formatted-balance branch from 79d94e9 to 4a67a60 Compare April 25, 2024 20:19
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Apr 25, 2024

Added a new issue to circle back to adding integrations tests for our asset list / token display. We need to add several new tokens to our test wallet as part of this effort. #5318

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Apr 25, 2024

Not confident my conversion here is accurate, or the price data is off from BiS? Coingecko is showing $152...

Screenshot 2024-04-25 at 5 34 27 PM

@brandonmarshall-tm
Copy link

I think you're good @fbwoolf! These numbers and prices are looking much more accurate to me. Prices can vary widly across exchanges (second screenshot), so depending on where BIS or Coingecko are pulling from, you might see some pretty big swings. But before it was like 10x the amount so this is much more accurate than before.

Screenshot 2024-04-25 at 7 46 30 PM

Screenshot 2024-04-25 at 7 49 45 PM

@fbwoolf fbwoolf added this pull request to the merge queue Apr 26, 2024
Merged via the queue into dev with commit d25741b Apr 26, 2024
28 checks passed
@fbwoolf fbwoolf deleted the fix/runes-formatted-balance branch April 26, 2024 12:31
@314159265359879
Copy link
Contributor

decimals and suffixes looking good now, this is going in the next release (after 6.36.0 that just went live), right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runes balance incorrect "M" prefix
5 participants