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

LIVE-2641 Prevent emitting an event when device descriptor is null #1070

Merged
merged 4 commits into from
Aug 29, 2022

Conversation

grsoares21
Copy link
Contributor

@grsoares21 grsoares21 commented Aug 26, 2022

📝 Description

This is a fix for an error that was found in Sentry.
While scanning devices, if there was a device for which the scan failed, we emitted is as a null device. However this broke on React end since we were trying to access infos from this null value as if it were an object.
See associated JIRA task for more info on the error.

The PR prevents the emission of an event in this case, fixing the issue directly in the scanning logic. This could have been prevented if we had a stricter typing, so I've also added some typing to the logic.

❓ Context

  • Impacted projects: ledgerjs/react-native-hw-transport-ble, ledger-live-mobile
  • Linked resource(s): LIVE-2641

✅ Checklist

  • [N/A] Test coverage -- this was more a question of proper typing rather than tests
  • Atomic delivery
  • No breaking changes

📸 Demo

N/A

@vercel
Copy link

vercel bot commented Aug 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
live-common-tools ✅ Ready (Inspect) Visit Preview Aug 29, 2022 at 1:35PM (UTC)
3 Ignored Deployments
Name Status Preview Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Aug 29, 2022 at 1:35PM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Aug 29, 2022 at 1:35PM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Aug 29, 2022 at 1:35PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2022

🦋 Changeset detected

Latest commit: e9f0cc6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
live-mobile Patch
@ledgerhq/react-native-hw-transport-ble Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM labels Aug 26, 2022
@github-actions
Copy link

github-actions bot commented Aug 26, 2022

@grsoares21

Screenshots: ✅

There are no changes in the screenshots for this PR. If this is expected, you are good to go.

Copy link
Contributor

@gre gre left a comment

Choose a reason for hiding this comment

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

lint issues (according to CI)

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #1070 (6c9bef8) into develop (9dff5cb) will decrease coverage by 47.57%.
The diff coverage is n/a.

❗ Current head 6c9bef8 differs from pull request most recent head e9f0cc6. Consider uploading reports for the commit e9f0cc6 to get more accurate results

@@             Coverage Diff             @@
##           develop   #1070       +/-   ##
===========================================
- Coverage    47.57%       0   -47.58%     
===========================================
  Files          647       0      -647     
  Lines        28899       0    -28899     
  Branches      7456       0     -7456     
===========================================
- Hits         13750       0    -13750     
+ Misses       15091       0    -15091     
+ Partials        58       0       -58     
Flag Coverage Δ
test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/ledger-live-common/src/account/importAccounts.ts
libs/ledger-live-common/src/range.ts
...ive-common/src/families/ethereum/hw-signMessage.ts
...edger-live-common/src/exchange/swap/hooks/index.ts
libs/ledger-live-common/src/countervalues/mock.ts
...js/packages/hw-app-btc/src/newops/psbtFinalizer.ts
...edger-live-common/src/transaction/signOperation.ts
...bs/ledgerjs/packages/hw-app-btc/src/signMessage.ts
...er-live-common/src/currencies/CurrencyURIScheme.ts
...on/src/families/osmosis/deviceTransactionConfig.ts
... and 637 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gre gre merged commit 533e658 into develop Aug 29, 2022
@gre gre deleted the bugfix/LIVE-2641-fix-null-ble-device branch August 29, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants