Skip to content

Add Hex Check For User Input #5668

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

rduttshukla
Copy link

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Added hex check for user input under Send Flow, the lack of which was causing the app to crash due to the toWei method not supporting hex strings. The reason for writing this PR are as below

  1. The app crashed when I accidentally pasted a wallet address into the amount text input
  2. Simply writing 0x1 also crashes the app

Screenshots/Recordings

Here is the screen recording demonstrating the issue.

WhatsApp.Video.2023-02-02.at.23.25.06.mp4

Issue

The issue is that the isDecimal check returns true a string like '0x1' but the resulting hex string is incompatible with the toWei method. While we can change the isDecimal to return false for hex strings, the solution I preferred is checking the string in the onInputChange method instead.

While we should depend on the keyboardType prop set as numeric, but as seen in the screen recording, it did not work as expected. Moreover, the prop didn't prevent pasting of non-numeric data at my end.

Added hex check for user input, the lack of which was causing the app to crash due to the `toWei` method not supporting hex strings
@rduttshukla rduttshukla requested a review from a team as a code owner February 2, 2023 18:07
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

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.

@rduttshukla
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@rduttshukla rduttshukla closed this Feb 2, 2023
@rduttshukla rduttshukla reopened this Feb 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2023
@legobeat
Copy link
Contributor

legobeat commented May 5, 2023

@MetaMask/mobile-devs PTAL

@NicholasEllul NicholasEllul added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 5, 2023
digiwand
digiwand previously approved these changes May 10, 2023
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

hey @rduttshukla, thanks for the catch and fix! LGTM

I think we can add a test for invalid inputs with or following this PR

@@ -85,6 +85,7 @@ import {
CURRENCY_SWITCH,
} from '../../../../../wdio/screen-objects/testIDs/Screens/AmountScreen.testIds.js';
import generateTestId from '../../../../../wdio/utils/generateTestId';
import { isHexString } from 'ethjs-util';
Copy link
Contributor

@legobeat legobeat May 11, 2023

Choose a reason for hiding this comment

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

This functionality is already imported at least through ethers.
See app/components/UI/PaymentRequest/index.js for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

@digiwand looks like this may be an inconsistent update-merge - could you take a look and rebase if necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch @legobeat. agreed using the ethers module would be preferred here too


I used the GitHub UI to resolve the merge conflict. I checked out the PR and the code looks good to me still

Copy link
Member

Choose a reason for hiding this comment

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

@digiwand looks like we're still importing from ethjs-util after the merge
could you retry or add a commit importing/using ethers? should fix the CI lint error too

Copy link
Member

Choose a reason for hiding this comment

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

🧹 as an aside could also follow-up separately to update below to ethers 🧹

isHexString,
addHexPrefix,
isValidChecksumAddress,
isHexPrefixed,
} from 'ethereumjs-util';

Copy link
Contributor

Choose a reason for hiding this comment

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

^ Dropping ethereumjs-util (deprecated) in favor of either ethers or @ethereumjs/util would be great I think

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals)
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

5 participants