fix: toBignumber conversion error with high balance #12010

siibars · 2024-10-24T17:12:17Z

Description

There is a bignumber conversion attempt which is failing, this change fixes this.

Related issues

Fixes: STAKE-848

Manual testing steps

connect with an account with a high ETH balance
enable the native staking feature flag
click stake to get to the StakeinputView

Screenshots/Recordings

Before

View: Root
Error: [number-to-bn] while converting number "3.487400189415765903484e+21" to BN.js instance, error: invalid number value. Value must be an integer, hex string, BN or BigNumber instance. Note, decimals are not supported.

After

This issue doesn't happen.

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-10-24T17:12:33Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions · 2024-10-24T17:24:18Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: fc7623c
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/40b358b8-44bc-4313-bf09-f955ae877fa7

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

legobeat · 2024-10-24T17:40:35Z

Do you have a link (or would you be able to describe in PR description) the issue this fixes?

Considering if this is related at all to solving it:

fix(deps): unpin [email protected],x [email protected] #11972

Also, is there any possible way to add a regression test for this bugfix?

app/components/UI/Stake/hooks/useBalance.ts

Daniel-Cross

Check whether the useMemo is actually needed. This kind of hook allocates system memory to watch the dependancies.

Daniel-Cross

Looks good, once the tests are passing. Thanks

legobeat · 2024-10-25T22:38:19Z

Do you have a link (or would you be able to describe in PR description) the issue this fixes?

Considering if this is related at all to solving it:

fix(deps): unpin [email protected],x [email protected] #11972

@siibars Thanks for updating, much clearer! 👍

Looks like it is indeed related. bn.js v4 does accept numbers with scientific notation as input to the constructor while v5 throws an error. It seems likely that this was once working correctly when implemented towards bn.js@4 and then at some point it started failing as the library version used in that particular place got substituted with v5. Unfortunately, the way BN is imported from legacy ethereumjs-util makes these discrepancies currently not get caught in type checks.

> new (require('bnjs4'))('1.23e30')
BN { negative: 0, words: [ 824430 ], length: 1, red: null }

> new (require('bnjs5'))('1.23e30')
Uncaught Error: Invalid character
    at assert (/home/user/dev/mm/.worktrees/metamask-mobile/deps-ethereumjs-6/node_modules/bnjs5/lib/bn.js:6:21)
    at parseBase (/home/user/dev/mm/.worktrees/metamask-mobile/deps-ethereumjs-6/node_modules/bnjs5/lib/bn.js:274:7)
    at BN._parseBase (/home/user/dev/mm/.worktrees/metamask-mobile/deps-ethereumjs-6/node_modules/bnjs5/lib/bn.js:298:14)
    at BN.init [as _init] (/home/user/dev/mm/.worktrees/metamask-mobile/deps-ethereumjs-6/node_modules/bnjs5/lib/bn.js:105:14)
    at new BN (/home/user/dev/mm/.worktrees/metamask-mobile/deps-ethereumjs-6/node_modules/bnjs5/lib/bn.js:39:12)

First steps in sorting out this larger mess:

legobeat · 2024-10-25T23:17:05Z

@siibars I am trying to track down exactly where the stakedBalanceWei (assets) string gets constructed - do you have a reference handy? Is it just being piped unmodified from a server-side API?

https://github.com/MetaMask/metamask-mobile/actions/runs/11526358054/job/32090342121?pr=12010#step:5:31

FAIL app/components/UI/Stake/hooks/useBalance.test.tsx
  ● useBalance › returns balance and fiat values based on account and pooled stake data

    expect(received).toBe(expected) // Object.is equality

    Expected: "5791332670714232000"
    Received: "05791332670714232000"

      76 |     expect(result.current.balanceFiat).toBe('$39506172511.60'); // Fiat balance
      77 |     expect(result.current.balanceFiatNumber).toBe(39506172511.6); // Fiat number balance
    > 78 |     expect(result.current.stakedBalanceWei).toBe('5791332670714232000'); // No staked assets
         |                                             ^
      79 |     expect(result.current.formattedStakedBalanceETH).toBe('5.79133 ETH'); // Formatted ETH balance
      80 |     expect(result.current.stakedBalanceFiatNumber).toBe(18532.26454); // Staked balance in fiat number
      81 |     expect(result.current.formattedStakedBalanceFiat).toBe('$18532.26'); //

      at Object.toBe (app/components/UI/Stake/hooks/useBalance.test.tsx:78:45)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:17)
      at _next (node_modules/@babel/runtime/helpers/asyncToGenerator.js:17:9)
      at node_modules/@babel/runtime/helpers/asyncToGenerator.js:22:7
      at Object.<anonymous> (node_modules/@babel/runtime/helpers/asyncToGenerator.js:14:12)

amitabh94 · 2024-10-25T23:59:44Z

@siibars I am trying to track down exactly where the stakedBalanceWei (assets) string gets constructed - do you have a reference handy? Is it just being piped unmodified from a server-side API?

https://github.com/MetaMask/metamask-mobile/actions/runs/11526358054/job/32090342121?pr=12010#step:5:31

FAIL app/components/UI/Stake/hooks/useBalance.test.tsx
  ● useBalance › returns balance and fiat values based on account and pooled stake data

    expect(received).toBe(expected) // Object.is equality

    Expected: "5791332670714232000"
    Received: "05791332670714232000"

      76 |     expect(result.current.balanceFiat).toBe('$39506172511.60'); // Fiat balance
      77 |     expect(result.current.balanceFiatNumber).toBe(39506172511.6); // Fiat number balance
    > 78 |     expect(result.current.stakedBalanceWei).toBe('5791332670714232000'); // No staked assets
         |                                             ^
      79 |     expect(result.current.formattedStakedBalanceETH).toBe('5.79133 ETH'); // Formatted ETH balance
      80 |     expect(result.current.stakedBalanceFiatNumber).toBe(18532.26454); // Staked balance in fiat number
      81 |     expect(result.current.formattedStakedBalanceFiat).toBe('$18532.26'); //

      at Object.toBe (app/components/UI/Stake/hooks/useBalance.test.tsx:78:45)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:17)
      at _next (node_modules/@babel/runtime/helpers/asyncToGenerator.js:17:9)
      at node_modules/@babel/runtime/helpers/asyncToGenerator.js:22:7
      at Object.<anonymous> (node_modules/@babel/runtime/helpers/asyncToGenerator.js:14:12)

We get this as wei string from the server side API.
https://github.com/consensys-vertical-apps/va-mmcx-staking-api/blob/81718575f23d03763d8abb991a2379cf54d25041/src/metaMaskPooledStaking/metaMaskPooledStaking.controller.ts#L77

amitabh94 · 2024-10-26T00:00:47Z

using new BN(assets) instead of hexToBn(assets) fixes the issue.

siibars · 2024-10-28T10:46:01Z

hi @legobeat
to give you more details on where stakedBalanceWei comes from :
a. in the mobile it's populated here with 'pooledStakeData' which comes from an api server call,
b. this call is implemented in the stake-sdk here
c. The api server implementation makes call to a smart contract method 'getShares' here retrieve an share amount (uint256) and then converts it to a @ethersproject/bignumber to then converts it to a 'bignumber.js' (v9) here,
d. Finally it multiplies it here with conversion rate to get an asset amount
e. Finally Finally this server api implementation return d as a string here using BN.toString 😮

I think this hextobn util here make sense but Indeed the BN import should probably be aligned and this file should also be written in ts for more type safety at build time.

@amitabh94 I am curious about d. and e. working as expected with account with a high share amount / conversion rate with a lot of decimals, do we have tests for such cases ?

cheers

github-actions · 2024-10-28T12:39:55Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: e74f458
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ae02fb19-c3c0-4056-b404-8691299cde94

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

github-actions · 2024-10-28T13:50:34Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 6060b7d
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f6eb14ec-5783-4abe-ae44-cd100edc850d

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

sonarcloud · 2024-10-28T13:57:57Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
60.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

amitabh94 · 2024-10-28T14:25:19Z

hi @legobeat to give you more details on where stakedBalanceWei comes from : a. in the mobile it's populated here with 'pooledStakeData' which comes from an api server call, b. this call is implemented in the stake-sdk here c. The api server implementation makes call to a smart contract method 'getShares' here retrieve an share amount (uint256) and then converts it to a @ethersproject/bignumber to then converts it to a 'bignumber.js' (v9) here, d. Finally it multiplies it here with conversion rate to get an asset amount e. Finally Finally this server api implementation return d as a string here using BN.toString 😮

I think this hextobn util here make sense but Indeed the BN import should probably be aligned and this file should also be written in ts for more type safety at build time.

@amitabh94 I am curious about d. and e. working as expected with account with a high share amount / conversion rate with a lot of decimals, do we have tests for such cases ?

cheers

We will have to check whether if we have specific test cases for decimals but what I know is that we use bignumber.js in Staking api which is used to deal with big numbers with decimal places.

## **Description** There is a bignumber conversion attempt which is failing, this change fixes this.  ## **Related issues** Fixes: STAKE-848 ## **Manual testing steps** 1. connect with an account with a high ETH balance 2. enable the native staking feature flag 3. click stake to get to the StakeinputView ## **Screenshots/Recordings**  ### **Before**  ![image](https://github.com/user-attachments/assets/bd071444-a6b6-49ca-ae31-4339ebcc68e6) View: Root Error: [number-to-bn] while converting number "3.487400189415765903484e+21" to BN.js instance, error: invalid number value. Value must be an integer, hex string, BN or BigNumber instance. Note, decimals are not supported. ### **After** This issue doesn't happen.  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: legobeat <[email protected]>

fix: toBignumber conversion error with high balance

fc7623c

siibars requested a review from a team as a code owner October 24, 2024 17:12

Matt561 previously approved these changes Oct 24, 2024

View reviewed changes

siibars added team-stake Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 24, 2024

fix: unit test

9fd65c1

siibars dismissed Matt561’s stale review via 9fd65c1 October 24, 2024 17:36

chore: fix lint

3c7f559

legobeat reviewed Oct 24, 2024

View reviewed changes

app/components/UI/Stake/hooks/useBalance.ts Outdated Show resolved Hide resolved

Matt561 self-requested a review October 24, 2024 17:59

siibars added 2 commits October 25, 2024 12:55

chore: create an asset BN type and stringify it in hex format

7cb32b0

chore: lint

f4b33b3

amitabh94 previously approved these changes Oct 25, 2024

View reviewed changes

Merge branch 'main' into fix/bignumber-conversion-error

89f0858

siibars requested a review from legobeat October 25, 2024 13:33

siibars added the Priority - Medium Task with medium priority label Oct 25, 2024

Daniel-Cross reviewed Oct 25, 2024

View reviewed changes

app/components/UI/Stake/hooks/useBalance.ts Outdated Show resolved Hide resolved

Daniel-Cross requested changes Oct 25, 2024

View reviewed changes

siibars requested a review from Daniel-Cross October 25, 2024 17:00

chore: remove use memo

903bba0

siibars dismissed amitabh94’s stale review via 903bba0 October 25, 2024 17:09

Daniel-Cross previously approved these changes Oct 25, 2024

View reviewed changes

Merge branch 'main' into fix/bignumber-conversion-error

4843699

This was referenced Oct 28, 2024

fix(deps): unpin [email protected],x [email protected] #11972

Open

fix: remove usage of bn.js v4 #12043

Draft

fix: return hex string version of BN objects

9901a8e

siibars dismissed Daniel-Cross’s stale review via 9901a8e October 28, 2024 12:29

Merge branch 'main' into fix/bignumber-conversion-error

e74f458

siibars added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 28, 2024

Merge branch 'main' into fix/bignumber-conversion-error

b458ecd

siibars enabled auto-merge October 28, 2024 13:18

Merge branch 'main' into fix/bignumber-conversion-error

6060b7d

siibars added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 28, 2024

siibars requested review from amitabh94 and Daniel-Cross October 28, 2024 14:28

amitabh94 approved these changes Oct 28, 2024

View reviewed changes

siibars added this pull request to the merge queue Oct 28, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2024

siibars added this pull request to the merge queue Oct 28, 2024

Merged via the queue into main with commit e96777a Oct 28, 2024
43 checks passed

siibars deleted the fix/bignumber-conversion-error branch October 28, 2024 15:22

github-actions bot locked and limited conversation to collaborators Oct 28, 2024

metamaskbot added the release-7.35.0 Issue or pull request that will be included in release 7.35.0 label Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: toBignumber conversion error with high balance #12010

fix: toBignumber conversion error with high balance #12010

siibars commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 •

edited by metamaskbot

Loading

legobeat commented Oct 24, 2024 •

edited

Loading

Daniel-Cross left a comment

Daniel-Cross left a comment

legobeat commented Oct 25, 2024 •

edited

Loading

legobeat commented Oct 25, 2024 •

edited

Loading

amitabh94 commented Oct 25, 2024

amitabh94 commented Oct 26, 2024

siibars commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Oct 28, 2024 •

edited by metamaskbot

Loading

sonarcloud bot commented Oct 28, 2024

amitabh94 commented Oct 28, 2024

fix: toBignumber conversion error with high balance #12010

fix: toBignumber conversion error with high balance #12010

Conversation

siibars commented Oct 24, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 • edited by metamaskbot Loading

Bitrise

legobeat commented Oct 24, 2024 • edited Loading

Daniel-Cross left a comment

Choose a reason for hiding this comment

Daniel-Cross left a comment

Choose a reason for hiding this comment

legobeat commented Oct 25, 2024 • edited Loading

legobeat commented Oct 25, 2024 • edited Loading

amitabh94 commented Oct 25, 2024

amitabh94 commented Oct 26, 2024

siibars commented Oct 28, 2024 • edited Loading

github-actions bot commented Oct 28, 2024 • edited by metamaskbot Loading

Bitrise

github-actions bot commented Oct 28, 2024 • edited by metamaskbot Loading

Bitrise

sonarcloud bot commented Oct 28, 2024

Quality Gate passed

amitabh94 commented Oct 28, 2024

siibars commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited by metamaskbot

Loading

legobeat commented Oct 24, 2024 •

edited

Loading

legobeat commented Oct 25, 2024 •

edited

Loading

legobeat commented Oct 25, 2024 •

edited

Loading

siibars commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Oct 28, 2024 •

edited by metamaskbot

Loading