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 loss of precision releasegold:withdraw CLI command #7748

Merged
merged 4 commits into from
Apr 23, 2021

Conversation

oneeman
Copy link
Contributor

@oneeman oneeman commented Apr 22, 2021

Description

This bug fixes the loss of precision (due to converting the BigNumber to number) in the releasegold:withdraw CLI Command. This was discovered by @zviadm when he tried to use the command to withdraw a contract's full balance. Due to the loss of precision, the value sent to the contract was higher than the actual balance, leading to the following error:

Error: Your request got reverted with the following reason string: Requested amount is greater than available released funds

Note that this loss of precision would still constitute a bug even when it doesn't lead to the transaction reverting (since the amount withdrawn is not exactly that requested by the user).

The PR also updates one of the tests for releasegold:withdraw to use a more precise assertion which would have caught this bug.

Tested

Confirmed that the updated test passes, and that before the fix the updated test fails as expected:

 FAIL  src/commands/releasegold/withdraw.test.ts (9.881 s)
  releasegold:withdraw cmd
    ✕ can withdraw released gold to beneficiary (3206 ms)
    ✓ can't withdraw the whole balance if there is a cUSD balance (1736 ms)

  ● releasegold:withdraw cmd › can withdraw released gold to beneficiary

    expected 1e+22 to equal 1.0000000000000000000005e+22

      39 |     const balanceAfter = await (await kit.getTotalBalance(beneficiary)).CELO!
      40 |     const difference = balanceAfter.minus(balanceBefore)
    > 41 |     expect(difference).toEqBigNumber(new BigNumber(withdrawalAmount))
         |                        ^
      42 |   })
      43 |
      44 |   test("can't withdraw the whole balance if there is a cUSD balance", async () => {

      at src/commands/releasegold/withdraw.test.ts:41:24
      at fulfilled (src/commands/releasegold/withdraw.test.ts:5:58)

Backwards compatibility

No change in the interface, the PR simply makes the command's behavior match the expected behavior (withdrawing the exact amount specified)

@oneeman oneeman requested review from gastonponti and a team April 22, 2021 22:00
@oneeman oneeman added the automerge Have PR merge automatically when checks pass label Apr 22, 2021
Copy link
Contributor

@eelanagaraj eelanagaraj left a comment

Choose a reason for hiding this comment

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

a nit but other than that looks good!

packages/cli/src/commands/releasegold/withdraw.test.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/releasegold/withdraw.test.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/releasegold/withdraw.test.ts Outdated Show resolved Hide resolved
Or Neeman and others added 2 commits April 23, 2021 08:10
Co-authored-by: Eela Nagaraj <7308464+eelanagaraj@users.noreply.github.com>
Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit da677b6 into master Apr 23, 2021
@mergify mergify bot deleted the oneeman/releasegold-withdraw-fix branch April 23, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants