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

RangeError: j6.l: Malformed currency code ! #12856

Closed
sentry-io bot opened this issue Jan 8, 2025 · 5 comments · Fixed by #12851
Closed

RangeError: j6.l: Malformed currency code ! #12856

sentry-io bot opened this issue Jan 8, 2025 · 5 comments · Fixed by #12851
Labels
android Android specific issue area-UI regression-RC-7.38.0 Regression bug that was found in release candidate (RC) for release 7.38.0 release-7.39.0 Issue or pull request that will be included in release 7.39.0 release-blocker This bug is blocking the next release Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-stake type-bug Something isn't working

Comments

@sentry-io
Copy link

sentry-io bot commented Jan 8, 2025

Sentry Issue: METAMASK-MOBILE-39FF

RangeError: j6.l: Malformed currency code !
  at anonymous (app/selectors/multichain.ts:133:44)
  at Tokens (app/components/UI/Tokens/index.tsx:125:50)
  at ?anon_0_ (app/components/Nav/App/index.js:637:26)
...
(45 additional frame(s) were not displayed)
@sentry-io sentry-io bot added the android Android specific issue label Jan 8, 2025
@chrisleewilcox
Copy link
Contributor

I triggered this with my physical android device. App on my device is now in a state where wallet view is always showing the crash screen. I am able to navigate to other screens from the bottom nav panel. Even if I restart app it lands on the crash screen.
https://github.com/user-attachments/assets/47898bc8-91bb-4479-b291-ae07a692d0a8

@chrisleewilcox chrisleewilcox added area-UI release-blocker This bug is blocking the next release Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing Priority - High Task with high priority QA Regression Testing labels Jan 8, 2025
@chrisleewilcox
Copy link
Contributor

I think this all started when I changed the currency to USDT in Settings > General and just started tapping on tokens to view the charts and then backing out to wallet view and tapping another token when this crash occurred.

@chrisleewilcox chrisleewilcox added team-assets regression-RC-7.38.0 Regression bug that was found in release candidate (RC) for release 7.38.0 labels Jan 8, 2025
@chrisleewilcox
Copy link
Contributor

chrisleewilcox commented Jan 8, 2025

Ok, so workaround is to change the currency back to USD in Settings > General, restart app and crash screen goes away.

@Unik0rnMaggie
Copy link
Contributor

Reproduced on Android Pixel 6 Pro, v7.38.0 (1523) from here when selecting currency conversion the following tokens:

  • SC
  • SNGLS
  • SNT
  • STEEM
  • STORJ
  • TIME
  • TKN
  • TRST
  • USDC
  • USDT
  • WINGS
Malformed.currency.code.error.mov

@chrisleewilcox chrisleewilcox added type-bug Something isn't working and removed Priority - High Task with high priority QA Regression Testing labels Jan 8, 2025
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Jan 8, 2025
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Jan 8, 2025
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
## **Description**

1. What is the reason for the change?

There was a crash when clicking on the Staked Ethereum asset due to it
missing a key `nativeAsset` which is required in some component. This
was a regression from code refactoring.

Separately, there was a currency switching bug that was introduced with
the use of a different fiat conversion method. I have switched to a
method that does not throw error for currency switching and this has
been tested with Vince

Separately, there were some issues with the Token list asset balances
being slightly off. This was noticed when the balance we use for the
Asset Detail staked Ethereum asset showed up differently than in the
Token List. Some of this has been addressed, but there are still issues
that I've noticed where we lose precision when getting the fiat total of
a token asset or native asset. I left a comment in the code about this,
and maybe it could be revisited it later.

2. What is the improvement/solution?

We added the `nativeAsset` key back to the staked ethereum asset

We added the weiToFiat method to replace the Intl.NumberFormat method 

We removed precision loss from multichain.ts asset fiatBalance
calculations, however there is another place(s) where fiat balances are
recalculated again i.e.
[deriveBalanceFromAssetMarketDetails.ts](https://github.com/MetaMask/metamask-mobile/pull/12851/files#diff-283c109c8093bbb5d8e06be79bcbeac928b8353633e55dc7944594a0e6888b49)

## **Related issues**

Fixes: #12680
Fixes: #12856
Closes: https://consensyssoftware.atlassian.net/browse/STAKE-912

## **Manual testing steps**

- Import the staking test wallet in MM
- Click on the staked ethereum asset
- The link should not crash the application more info here on testing
this #12680
- Currency switching should not crash the application more info here on
testing this #12856

## **Screenshots/Recordings**

### **Before**

### **After**


https://github.com/user-attachments/assets/c2525b70-8d04-4606-861b-3206ebd557f4


https://github.com/user-attachments/assets/a8c4010e-3589-4171-803a-4d07a962e6a6

## **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
- [ ] 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**

- [ ] 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-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Jan 10, 2025
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Jan 10, 2025
@metamaskbot metamaskbot added the release-7.39.0 Issue or pull request that will be included in release 7.39.0 label Jan 10, 2025
runway-github bot added a commit that referenced this issue Jan 10, 2025
## **Description**

1. What is the reason for the change?

There was a crash when clicking on the Staked Ethereum asset due to it
missing a key `nativeAsset` which is required in some component. This
was a regression from code refactoring.

Separately, there was a currency switching bug that was introduced with
the use of a different fiat conversion method. I have switched to a
method that does not throw error for currency switching and this has
been tested with Vince

Separately, there were some issues with the Token list asset balances
being slightly off. This was noticed when the balance we use for the
Asset Detail staked Ethereum asset showed up differently than in the
Token List. Some of this has been addressed, but there are still issues
that I've noticed where we lose precision when getting the fiat total of
a token asset or native asset. I left a comment in the code about this,
and maybe it could be revisited it later.

2. What is the improvement/solution?

We added the `nativeAsset` key back to the staked ethereum asset

We added the weiToFiat method to replace the Intl.NumberFormat method 

We removed precision loss from multichain.ts asset fiatBalance
calculations, however there is another place(s) where fiat balances are
recalculated again i.e.
[deriveBalanceFromAssetMarketDetails.ts](https://github.com/MetaMask/metamask-mobile/pull/12851/files#diff-283c109c8093bbb5d8e06be79bcbeac928b8353633e55dc7944594a0e6888b49)

## **Related issues**

Fixes: #12680
Fixes: #12856
Closes: https://consensyssoftware.atlassian.net/browse/STAKE-912

## **Manual testing steps**

- Import the staking test wallet in MM
- Click on the staked ethereum asset
- The link should not crash the application more info here on testing
this #12680
- Currency switching should not crash the application more info here on
testing this #12856

## **Screenshots/Recordings**

### **Before**

### **After**


https://github.com/user-attachments/assets/c2525b70-8d04-4606-861b-3206ebd557f4


https://github.com/user-attachments/assets/a8c4010e-3589-4171-803a-4d07a962e6a6

## **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
- [ ] 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**

- [ ] 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.
runway-github bot added a commit that referenced this issue Jan 10, 2025
## **Description**

1. What is the reason for the change?

There was a crash when clicking on the Staked Ethereum asset due to it
missing a key `nativeAsset` which is required in some component. This
was a regression from code refactoring.

Separately, there was a currency switching bug that was introduced with
the use of a different fiat conversion method. I have switched to a
method that does not throw error for currency switching and this has
been tested with Vince

Separately, there were some issues with the Token list asset balances
being slightly off. This was noticed when the balance we use for the
Asset Detail staked Ethereum asset showed up differently than in the
Token List. Some of this has been addressed, but there are still issues
that I've noticed where we lose precision when getting the fiat total of
a token asset or native asset. I left a comment in the code about this,
and maybe it could be revisited it later.

2. What is the improvement/solution?

We added the `nativeAsset` key back to the staked ethereum asset

We added the weiToFiat method to replace the Intl.NumberFormat method

We removed precision loss from multichain.ts asset fiatBalance
calculations, however there is another place(s) where fiat balances are
recalculated again i.e.
[deriveBalanceFromAssetMarketDetails.ts](https://github.com/MetaMask/metamask-mobile/pull/12851/files#diff-283c109c8093bbb5d8e06be79bcbeac928b8353633e55dc7944594a0e6888b49)

## **Related issues**

Fixes: #12680
Fixes: #12856
Closes: https://consensyssoftware.atlassian.net/browse/STAKE-912

## **Manual testing steps**

- Import the staking test wallet in MM
- Click on the staked ethereum asset
- The link should not crash the application more info here on testing
this #12680
- Currency switching should not crash the application more info here on
testing this #12856

## **Screenshots/Recordings**

### **Before**

### **After**

https://github.com/user-attachments/assets/c2525b70-8d04-4606-861b-3206ebd557f4

https://github.com/user-attachments/assets/a8c4010e-3589-4171-803a-4d07a962e6a6

## **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
- [ ] 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**

- [ ] 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.
runway-github bot pushed a commit that referenced this issue Jan 10, 2025
…et multichain (#12851)

## **Description**

1. What is the reason for the change?

There was a crash when clicking on the Staked Ethereum asset due to it
missing a key `nativeAsset` which is required in some component. This
was a regression from code refactoring.

Separately, there was a currency switching bug that was introduced with
the use of a different fiat conversion method. I have switched to a
method that does not throw error for currency switching and this has
been tested with Vince

Separately, there were some issues with the Token list asset balances
being slightly off. This was noticed when the balance we use for the
Asset Detail staked Ethereum asset showed up differently than in the
Token List. Some of this has been addressed, but there are still issues
that I've noticed where we lose precision when getting the fiat total of
a token asset or native asset. I left a comment in the code about this,
and maybe it could be revisited it later.

2. What is the improvement/solution?

We added the `nativeAsset` key back to the staked ethereum asset

We added the weiToFiat method to replace the Intl.NumberFormat method 

We removed precision loss from multichain.ts asset fiatBalance
calculations, however there is another place(s) where fiat balances are
recalculated again i.e.
[deriveBalanceFromAssetMarketDetails.ts](https://github.com/MetaMask/metamask-mobile/pull/12851/files#diff-283c109c8093bbb5d8e06be79bcbeac928b8353633e55dc7944594a0e6888b49)

## **Related issues**

Fixes: #12680
Fixes: #12856
Closes: https://consensyssoftware.atlassian.net/browse/STAKE-912

## **Manual testing steps**

- Import the staking test wallet in MM
- Click on the staked ethereum asset
- The link should not crash the application more info here on testing
this #12680
- Currency switching should not crash the application more info here on
testing this #12856

## **Screenshots/Recordings**

### **Before**

### **After**


https://github.com/user-attachments/assets/c2525b70-8d04-4606-861b-3206ebd557f4


https://github.com/user-attachments/assets/a8c4010e-3589-4171-803a-4d07a962e6a6

## **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
- [ ] 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**

- [ ] 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.
sethkfman pushed a commit that referenced this issue Jan 13, 2025
…et multichain (#12930)

- fix: add nativeAsset key to staked ETH asset multichain (#12851)

## **Description**

1. What is the reason for the change?

There was a crash when clicking on the Staked Ethereum asset due to it
missing a key `nativeAsset` which is required in some component. This
was a regression from code refactoring.

Separately, there was a currency switching bug that was introduced with
the use of a different fiat conversion method. I have switched to a
method that does not throw error for currency switching and this has
been tested with Vince

Separately, there were some issues with the Token list asset balances
being slightly off. This was noticed when the balance we use for the
Asset Detail staked Ethereum asset showed up differently than in the
Token List. Some of this has been addressed, but there are still issues
that I've noticed where we lose precision when getting the fiat total of
a token asset or native asset. I left a comment in the code about this,
and maybe it could be revisited it later.

2. What is the improvement/solution?

We added the `nativeAsset` key back to the staked ethereum asset

We added the weiToFiat method to replace the Intl.NumberFormat method 

We removed precision loss from multichain.ts asset fiatBalance
calculations, however there is another place(s) where fiat balances are
recalculated again i.e.

[deriveBalanceFromAssetMarketDetails.ts](https://github.com/MetaMask/metamask-mobile/pull/12851/files#diff-283c109c8093bbb5d8e06be79bcbeac928b8353633e55dc7944594a0e6888b49)

## **Related issues**

Fixes: #12680
Fixes: #12856
Closes: https://consensyssoftware.atlassian.net/browse/STAKE-912

## **Manual testing steps**

- Import the staking test wallet in MM
- Click on the staked ethereum asset
- The link should not crash the application more info here on testing
this #12680
- Currency switching should not crash the application more info here on
testing this #12856

## **Screenshots/Recordings**

### **Before**

### **After**



https://github.com/user-attachments/assets/c2525b70-8d04-4606-861b-3206ebd557f4



https://github.com/user-attachments/assets/a8c4010e-3589-4171-803a-4d07a962e6a6

## **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
- [ ] 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**

- [ ] 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.
[40a999c](40a999c)

Co-authored-by: Nicholas Smith <nick.smith@consensys.net>
@nickewansmith
Copy link
Contributor

nickewansmith commented Jan 22, 2025

Root Cause Analysis

The root cause of this issue was that new changes to using Intl.NumberFormat for fiat conversions did not support some currencies such as STORJ and others mentioned above. There were also some other unexpected effects of using Intl.NumberFormat and so it was reverted back to how it was being used before. The fix that I created for a separate issue just so happened to address this issue as well. There were some inconsistencies in staking balance from using Intl.NumberFormat that led me to make the related change. The staking related issues found were all regressions from various recently merged multi-chain features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Android specific issue area-UI regression-RC-7.38.0 Regression bug that was found in release candidate (RC) for release 7.38.0 release-7.39.0 Issue or pull request that will be included in release 7.39.0 release-blocker This bug is blocking the next release Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-stake type-bug Something isn't working
Projects
Archived in project
Status: Fixed
Development

Successfully merging a pull request may close this issue.

5 participants