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.

Sign up for GitHub

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

Jump to bottom

Add validation to send amount input box. #6040

Merged
merged 14 commits into from
May 23, 2023
Merged

Add validation to send amount input box. #6040

merged 14 commits into from
May 23, 2023

Conversation

segun
Copy link
Contributor

@segun segun commented Mar 24, 2023 •
edited

Description

We are lacking input validation for the Send amount field, resulting into unexpected behaviour when we enter non-conventional chars.

If we enter 0x0 value, MetaMask crashes completly
If we enter negative values -0.001 you can proceed to the next page. When you land on the Confirmation page, then you get a warning

Screenshots/Recordings

Before

send-input-validation.mp4

After

Screen.Recording.2023-03-24.at.13.23.05.mov

Issue

Fixes #5589

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@segun segun requested a review from a team as a code owner March 24, 2023 13:25
@github-actions
Copy link
Contributor

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.

@bschorchit bschorchit mentioned this pull request Apr 18, 2023
Merged
8 tasks
@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from 3e93ea5 to 69b0526 Compare April 19, 2023 16:22
@blackdevelopa
Copy link
Contributor

Hey @segun it seems you have an unresolved conflict, can you check this out? TY

@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from 59a4752 to 7a08487 Compare April 27, 2023 16:08
blackdevelopa
blackdevelopa reviewed Apr 27, 2023
View reviewed changes
ios/Podfile.lock Outdated Show resolved Hide resolved
@jpuri
Copy link
Contributor

jpuri commented Apr 28, 2023

Hey @segun : did you had chance to discuss the required behaviour with product team, I just want to be sure that change is aligned with design guidelines for mobile app and consistent with other pages.

@segun segun added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels May 3, 2023
@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from 39ee698 to cb52ee3 Compare May 3, 2023 13:46
@segun segun requested review from jpuri and blackdevelopa May 5, 2023 18:06
@segun
Copy link
Contributor Author

segun commented May 8, 2023

Hey @segun : did you had chance to discuss the required behaviour with product team, I just want to be sure that change is aligned with design guidelines for mobile app and consistent with other pages.

Yes I did. The change is aligned.

blackdevelopa
blackdevelopa previously approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

LGTM. Check to see if yarn lint:fix can fix the failing lint

@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from cc5aa58 to 6adcb55 Compare May 12, 2023 01:22
jpuri
jpuri previously approved these changes May 12, 2023
View reviewed changes
@segun segun dismissed stale reviews from jpuri and blackdevelopa via 45354a0 May 15, 2023 17:28
@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from 6adcb55 to 45354a0 Compare May 15, 2023 17:28
@segun segun requested review from jpuri and blackdevelopa May 15, 2023 20:18
jpuri
jpuri previously approved these changes May 16, 2023
View reviewed changes
blackdevelopa
blackdevelopa previously approved these changes May 16, 2023
View reviewed changes
@blackdevelopa blackdevelopa removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 16, 2023
segun added 11 commits May 18, 2023 17:45
@segun
lint fixes
b31c49b 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
use check if has text
4a1d9ce 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Make sure tests passed
1b75f22 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Do not remove special characters...disable next.
10b6688 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Make sure error is shown
219dd67 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Check for negative value
506ff26 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
revert seedphrase
5acd8be 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Fix e2e after rebase
ddfccc2 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Remove SRP
9256cb1 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Lint fix
049e1e0 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun
Fix snapshot
aae751a 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@segun segun dismissed stale reviews from blackdevelopa and jpuri via aae751a May 18, 2023 14:53
@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from 45354a0 to aae751a Compare May 18, 2023 14:53
@segun
Copy link
Contributor Author

segun commented May 18, 2023

Hi @seaona I rebased to main.

Can you please QA again (I've re-added the label)

hi @segun I am seeing the following error when I load the app in android. Could you maybe update the branch with the last main?

image

@segun segun added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label May 18, 2023
@segun segun force-pushed the fix-send-amount-input-validation-5589 branch from 14ddb69 to aae751a Compare May 19, 2023 07:38
@segun
Fix snapshot
295de8b 
Signed-off-by: Olusegun Akintayo <akintayo.segun@gmail.com>
@seaona
Copy link
Contributor

seaona commented May 19, 2023

hi @segun !! Changes look good, thank you for the update!! The PR can be merged once we have x2 dev approvals 💯

@seaona seaona added QA Passed A successful QA run through has been done release-6.7.0 6.7.0 release tickets and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 19, 2023
@seaona seaona merged commit 698f0c5 into main May 23, 2023
10 checks passed
@seaona seaona deleted the fix-send-amount-input-validation-5589 branch May 23, 2023 09:16
@github-actions github-actions bot locked and limited conversation to collaborators May 23, 2023
@cortisiko cortisiko added release-7.1.0 and removed release-6.7.0 6.7.0 release tickets labels May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jpuri jpuri jpuri approved these changes

@blackdevelopa blackdevelopa blackdevelopa approved these changes

Assignees

@segun segun

Labels
QA Passed A successful QA run through has been done release-7.1.0 team-confirmations-secure-ux-PR PR from the confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Lacking Input Validation for Send amount, crashing MM when using certain values
5 participants
@segun @blackdevelopa @jpuri @seaona @cortisiko