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

Trim trailing zeroes #11068

Merged
merged 16 commits into from
Oct 16, 2024
Merged

Trim trailing zeroes #11068

merged 16 commits into from
Oct 16, 2024

Conversation

uDaiko
Copy link
Contributor

@uDaiko uDaiko commented Oct 1, 2024

Thank you for your contribution to the Koda - Generative Art Marketplace.
After I stumpled upon this, as discussed in #10230

👇 __ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Needs QA check

  • @kodadot/qa-guild please review

Context

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

firefox_XHgWUfXpNE
firefox_kQGteIoil3

@uDaiko uDaiko requested a review from a team as a code owner October 1, 2024 14:27
@uDaiko uDaiko requested review from Jarsen136 and hassnian and removed request for a team October 1, 2024 14:27
Copy link

netlify bot commented Oct 1, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 33f14b0
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/670fb7edf041720008f3f35d
😎 Deploy Preview https://deploy-preview-11068--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Jarsen136
Copy link
Contributor

I just rechecked #10230 and found that it happened when you listed some NFT. Could you also fix it there?

image

@uDaiko
Copy link
Contributor Author

uDaiko commented Oct 3, 2024

@Jarsen136 good catch. Aside from MakingOfferSingleItem and with your Input I found ListingCartSingleItemCart and ListingCartItem where that issue appears. Given that all these basically have the same requirement I thought it would be better to just refactor it and put the parse Code into the balance util. Please have a look.

@vikiival
Copy link
Member

vikiival commented Oct 7, 2024

cc @uDaiko

@uDaiko
Copy link
Contributor Author

uDaiko commented Oct 7, 2024

@hassnian I used the function you mentioned, please have a look. It's working however I noticed that leaving the roundBy parameter blank as you suggested results in unreliable rounds from 0.3999 to 0.4. Would probably make sense to just add 4 round to the function calls

@hassnian
Copy link
Contributor

hassnian commented Oct 8, 2024

I noticed that leaving the roundBy parameter blank as you suggested results in unreliable rounds from 0.3999 to 0.4.

@uDaiko that's fine, if for some reason you don't need the rounding and you need to show the exact number

you can add a optional logic to roundTo by adding minimumFractionDigits

@uDaiko
Copy link
Contributor Author

uDaiko commented Oct 8, 2024

I noticed that leaving the roundBy parameter blank as you suggested results in unreliable rounds from 0.3999 to 0.4.

@uDaiko that's fine, if for some reason you don't need the rounding and you need to show the exact number

you can add a optional logic to roundTo by adding minimumFractionDigits

Well the rounding is already in place in the composable. I was thinking of just passing computed(() => 4)
useAmount(computed(() => props.nft.collection.floor), decimals, chainSymbol, computed(() => 4)) for these 3 instances
alternatively you can have roundTo to have a default of 4 unless stated otherwise but then existing calls of this function would have to be checked and modified so I think it's easier to just pass it where needed here.

Copy link
Contributor

@hassnian hassnian left a comment

Choose a reason for hiding this comment

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

almost there!

Copy link
Contributor

@hassnian hassnian left a comment

Choose a reason for hiding this comment

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

there are a couple of edge cases that need covering , but this is working fine

let's keep it simple, reverted to a prev version 405b0f7

@Jarsen136 code changed a bit pls re-review ty

@hassnian hassnian requested a review from Jarsen136 October 15, 2024 06:06
@hassnian hassnian added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Oct 16, 2024
@vikiival vikiival enabled auto-merge October 16, 2024 12:56
@vikiival
Copy link
Member

Thank u @uDaiko !

@vikiival vikiival added this pull request to the merge queue Oct 16, 2024
Merged via the queue into kodadot:main with commit e6751b1 Oct 16, 2024
18 of 20 checks passed
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicated zeros in decimals
4 participants