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

feat: display incompatibility message for LNS swap, LIVE-14891, LIVE-14838, LIVE-14923 #8433

Merged
merged 14 commits into from
Dec 6, 2024

Conversation

andreicovaciu
Copy link
Contributor

@andreicovaciu andreicovaciu commented Nov 21, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

πŸ“ Description

Adds LNS incompatibility banner for TON, ADA and SPL Tokens.
image (6)

❓ Context

  • JIRA or GitHub link: LIVE-14838

🧐 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)

@andreicovaciu andreicovaciu requested a review from a team as a code owner November 21, 2024 14:30
Copy link

vercel bot commented Nov 21, 2024

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

4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2024 9:38am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2024 9:38am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2024 9:38am
web-tools ⬜️ Ignored (Inspect) Visit Preview Dec 6, 2024 9:38am

@live-github-bot live-github-bot bot added desktop Has changes in LLD translations Translation files have been touched labels Nov 21, 2024
@andreicovaciu andreicovaciu force-pushed the feat/LIVE-14838-adds-lns-swap-error branch from d0f694f to 44841be Compare November 22, 2024 09:41
@andreicovaciu andreicovaciu changed the title feat: display incompatibility message for LNS swap feat: display incompatibility message for LNS swap, LIVE-14888, LIVE-14838, LIVE-14923 Nov 22, 2024
@andreicovaciu andreicovaciu changed the title feat: display incompatibility message for LNS swap, LIVE-14888, LIVE-14838, LIVE-14923 feat: display incompatibility message for LNS swap, LIVE-14891, LIVE-14838, LIVE-14923 Nov 25, 2024
Copy link
Contributor

@Justkant Justkant left a comment

Choose a reason for hiding this comment

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

I think we might want to handle the renderHardwareUpdate before the DeviceAction component to avoid sending APDUs to the device while showing the message

@andreicovaciu
Copy link
Contributor Author

I think we might want to handle the renderHardwareUpdate before the DeviceAction component to avoid sending APDUs to the device while showing the message

What is your suggestion?

@Justkant
Copy link
Contributor

Justkant commented Dec 5, 2024

What is your suggestion?

Hey you'll want to move the renderHardwareUpdate outside the DeviceAction component

I think we only need to target the new swap flows as the old native swap should not be used in prod anymore and the old platform startExchange modal is not used anymore by the wallet-api

For this you can edit this file: https://github.com/LedgerHQ/ledger-live/blob/develop/apps/ledger-live-desktop/src/renderer/components/LiveAppDrawer.tsx#L162-L178
And to get the device in the wrapper component you can use this selector
const device = useSelector(getCurrentDevice);

The main idea is that we don't want to call the useHook here: https://github.com/LedgerHQ/ledger-live/blob/develop/apps/ledger-live-desktop/src/renderer/components/DeviceAction/index.tsx#L640

@andreicovaciu andreicovaciu requested a review from a team as a code owner December 5, 2024 13:58
Justkant
Justkant previously approved these changes Dec 6, 2024
Copy link
Contributor

@Justkant Justkant left a comment

Choose a reason for hiding this comment

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

I've tested a lot of cases with multiple devices and it now works perfectly, thx for the updates

@andreicovaciu andreicovaciu merged commit b4ae38f into develop Dec 6, 2024
39 of 41 checks passed
@andreicovaciu andreicovaciu deleted the feat/LIVE-14838-adds-lns-swap-error branch December 6, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants