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 wrongly computed insufficient funds errors #13347

Merged
merged 5 commits into from
May 17, 2022
Merged

Conversation

onyb
Copy link
Member

@onyb onyb commented May 16, 2022

This PR fixes multiple issues with insufficient funds checks in the transaction confirmation panel.

  1. ETHSwap transactions had wrong computation of fiat amounts. 92587af51ab70dd46974f2c19428bd9f38c772e0
  2. Prevent intermittent "insufficient funds" errors in the panel. It is possible these errors do not go away after balance is fetched. 70ed1b7f5ce4374abe67c58208eed2fe3c1cb132 and 89ac58d65fd995ba3e4c71b50c204739f7aa645b
  3. Differentiate between insufficient funds due to token balance vs gas fees. ffc835ff3c97edb6acb44de136e19a18e9c4323b
  4. Guarantee disable of the Confirm button in txn confirmation screen when balances are being fetched. 51f47c6f5325eb03c60608e1b05adf723d18cbca

Resolves brave/brave-browser#22877.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Demo:

Before

Wrong fiat amounts

  1. fiat amount of 0.00616164 ETH is too low
  2. insufficient balance $18.30 does not reflect send amount + gas fees.

Screenshot 2022-05-16 at 21 17 40

Disappearing "Insufficient funds error" text

cc: @srirambv I believe you had logged this at some point, but I can't find the issue.

Screen.Recording.2022-05-16.at.21.14.25.mov

After

No disappearing error text, and correct computation of fiat amount

Screen.Recording.2022-05-16.at.21.08.22.mov

Confirm button is disabled on page load while balances are being fetched

Screen.Recording.2022-05-16.at.22.13.59.mov

Note: This is not entirely new behaviour, but ensures that Confirm button is not disabled due to insufficientFundsError being false for redux state not being ready.

@onyb onyb requested review from josheleonard, Douglashdaniel, muliswilliam and a team May 16, 2022 20:19
@onyb onyb self-assigned this May 16, 2022
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label May 16, 2022
@@ -216,24 +222,48 @@ export default function useSwap ({ fromAsset: fromAssetProp, toAsset: toAssetPro

const isSwapButtonDisabled = React.useMemo(() => {
return (
// Prevent creating a swap transaction with stale parameters if fetching
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work on these comments

@onyb onyb force-pushed the f/wallet/insufficient-funds branch from 51f47c6 to d8bd5ec Compare May 16, 2022 22:19
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@onyb onyb added this to the 1.40.x - Nightly milestone May 17, 2022
@onyb onyb merged commit 60e56bf into master May 17, 2022
@onyb onyb deleted the f/wallet/insufficient-funds branch May 17, 2022 09:41
@srirambv
Copy link
Contributor

srirambv commented May 18, 2022

Verification passed on

Brave 1.40.64 Chromium: 102.0.5005.50 (Official Build) nightly (64-bit)
Revision f6e2cf8f59ec714bdcff8499991d55898875f287-refs/branch-heads/5005@{#648}
OS Windows 11 Version 22H2 (Build 22621.1)
  • Verified insufficient funds message is not shown when there is sufficient balance while doing a transaction
  • Verified error message is retained and confirm button doesn't get enabled
  • Verified closing and reopening panel doesn't allow the confirm button to be clicked when the error message is shown
13347.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix wrongly computed insufficient funds errors
5 participants