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

Refactor: LowGasAlert errors on EVM #7782

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Conversation

CremaFR
Copy link
Member

@CremaFR CremaFR commented Sep 9, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • Fixes error management of GasPrice errors.

πŸ“ Description

The previous PR #5780 introduced changes to how errors were managed in the SendAmountField component from EVM. It assumed that only Send was using the SendAmountField, which led to issues when the component was reused in other flows, such as Swap and Sign. This caused multiple errors to be displayed inconsistently, as seen in the attached screenshots.

Screenshot 2024-09-09 at 15 48 55 Screenshot 2024-09-09 at 15 20 58

Fixes in this PR:

  • Refactored the way gasPriceError and related errors are handled in the Swap and Send flows.
  • Removed redundant error management inside the form component, and delegated error handling to the appropriate wrappers (e.g., SwapFeeDrawer, Send, Sign).
  • Removed old BUY ETH buttons as per feat(LLM/send): New gas fees parent account errorΒ #5780 did. Also remove dead code/checks related to this
  • Ensured that errors like NotEnoughGas and GasPrice are consistently handled across different flows without duplicating logic of buy click and redirection
  • Added a new LowGasAlertBuyMore component to simplify the logic and centralize the error handling and Buy action
  • Allows Drawer that uses the SendAmountFields to be closed like modals

The changes aim to improve consistency in error management across different flows allowing more flexibility in usage, features and tracking

Screenshots of the Sign flow fixed on the triple error. Now consistent with every other flow and tracking correctly the redirection

Screenshot 2024-09-09 at 23 28 12

❓ Context

  • JIRA or GitHub link:

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@CremaFR CremaFR requested review from a team as code owners September 9, 2024 17:00
Copy link

vercel bot commented Sep 9, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
ledger-live-github-bot βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Sep 13, 2024 7:35am
web-tools ❌ Failed (Inspect) Sep 13, 2024 7:35am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Sep 13, 2024 7:35am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 13, 2024 7:35am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 13, 2024 7:35am

@live-github-bot live-github-bot bot added the desktop Has changes in LLD label Sep 9, 2024
@CremaFR CremaFR merged commit 06a6ce1 into develop Sep 13, 2024
40 of 42 checks passed
@CremaFR CremaFR deleted the feat/LowGasAlertBuyMore branch September 13, 2024 08:46
@VicAlbr VicAlbr mentioned this pull request Sep 17, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants