Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

fix: fix surplus calculation for very big numbers #515

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

shoom3301
Copy link
Contributor

@shoom3301 shoom3301 commented Jun 9, 2023

Summary

For some reason, the surplus is wrongly displayed for very big numbers.

Example: https://explorer.cow.fi/orders/0xe05b6a805656fdf2a4c404be0756f360e0297c53d00371da637a44e133676d2e6cd22a1d90eeba4bfd0080ad9f121d4a33b1fda96459d692?tab=overview

Background

bignumber.js wrongly round very small numbers - when you divide something small to something very big

Actual result:

const minimumBuyAmount = new BigNumber('4000000000000000000000000000').multipliedBy(
      (new BigNumber('7152000000'))
        .dividedBy(new BigNumber('4000000000000000000000000000'))
    )
// 7160000000
image

Expected result:

const minimumBuyAmount = (4000000000000000000000000000) * (7152000000 / 4000000000000000000000000000 )
// 7152000000
image

To test

The displayed surplus should look the same for any other values:

  1. Very small
  2. Normal
  3. Very big

@vercel
Copy link

vercel bot commented Jun 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

📖 Storybook ↗︎

@shoom3301 shoom3301 requested review from a team June 9, 2023 10:26
@shoom3301 shoom3301 self-assigned this Jun 9, 2023
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

Pull Request Test Coverage Report for Build 5220880806

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 61.585%

Totals Coverage Status
Change from base Build 5188962808: 0.0%
Covered Lines: 872
Relevant Lines: 1231

💛 - Coveralls

@anxolin anxolin changed the base branch from develop to release/2.23 June 12, 2023 12:08
@anxolin anxolin mentioned this pull request Jun 12, 2023
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Didn't quite work
image

Also the unit tests are broken, probably due to the same reason?

@shoom3301 shoom3301 force-pushed the fix/surplus-formula branch from 2a5d317 to 2940230 Compare June 12, 2023 13:50
@shoom3301
Copy link
Contributor Author

@alfetopito sorry, it was an attempt to use Anxo's suggestion. Now it works.
@elena-zh fyi

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Changes LGTM.
Still, we have an issue with a surplus percentage, but it is reported in #462 and can be addressed later.
image

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Nice, looks great now!

One gentle reminder that you should do that for the buySurplus as well

@shoom3301 shoom3301 force-pushed the fix/surplus-formula branch from 2940230 to dbd5a7e Compare June 13, 2023 08:45
@alfetopito alfetopito merged commit 0e1408e into release/2.23 Jun 13, 2023
@alfetopito alfetopito deleted the fix/surplus-formula branch June 13, 2023 09:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants