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: unresponsive ledger device #474

Merged
merged 2 commits into from
Jan 9, 2024
Merged

fix: unresponsive ledger device #474

merged 2 commits into from
Jan 9, 2024

Conversation

r4mmer
Copy link
Member

@r4mmer r4mmer commented Jan 4, 2024

Summary

Fix the issues described in #473

Acceptance Criteria

  • Update ledger transport dependency
  • Update hathor wallet lib dependency
  • Do not redirect to locked screen when starting a hardware wallet
  • During preload, clean any previous hardware wallet access data.
  • Add better logs to debug issues with ledger communication

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@r4mmer r4mmer self-assigned this Jan 4, 2024
@r4mmer r4mmer requested a review from pedroferreira1 as a code owner January 4, 2024 15:50
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (cbecaf1) 9.34% compared to head (a66063e) 9.34%.

Files Patch % Lines
src/storage.js 0.00% 5 Missing ⚠️
src/App.js 0.00% 0 Missing and 2 partials ⚠️
src/screens/StartHardwareWallet.js 0.00% 1 Missing ⚠️
src/utils/wallet.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             dev    #474      +/-   ##
========================================
- Coverage   9.34%   9.34%   -0.01%     
========================================
  Files        112     112              
  Lines       5166    5167       +1     
  Branches     698     698              
========================================
  Hits         483     483              
- Misses      4020    4021       +1     
  Partials     663     663              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

// This ensures that a previously closed hardware wallet will not be loaded again
// unless the device is connected
const accessData = JSON.parse(accessDataRaw);
if ((accessData?.walletFlags & 0x02) > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

0x02 is the hardware wallet flag (ref).
We can expose WALLET_FLAGS from the lib on the next version and import it here but I wish to release this fix asap so I hardcoded the value here for now.

Copy link
Member

Choose a reason for hiding this comment

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

Can you create the issue for that, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

@r4mmer r4mmer requested a review from tuliomir January 4, 2024 16:21
package.json Outdated
"@hathor/wallet-lib": "1.0.2",
"@ledgerhq/hw-transport-node-hid": "6.27.1",
"@hathor/wallet-lib": "1.0.4",
"@ledgerhq/hw-transport-node-hid": "^6.28",
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Could we use a pinned version for this dependency? This has been the default since #422

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@r4mmer r4mmer requested a review from tuliomir January 8, 2024 11:48
@r4mmer r4mmer force-pushed the fix/ledger-unresponsive branch from 860e40f to 416a4db Compare January 8, 2024 15:31
@r4mmer r4mmer changed the base branch from master to dev January 8, 2024 15:32
Comment on lines +140 to +141
console.log('Error communicating with device');
console.log(error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: Maybe switch these error messages to console.error instead?

@r4mmer r4mmer linked an issue Jan 8, 2024 that may be closed by this pull request
// This ensures that a previously closed hardware wallet will not be loaded again
// unless the device is connected
const accessData = JSON.parse(accessDataRaw);
if ((accessData?.walletFlags & 0x02) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you create the issue for that, please?

@pedroferreira1 pedroferreira1 self-requested a review January 9, 2024 15:06
@r4mmer r4mmer merged commit c2c5504 into dev Jan 9, 2024
3 checks passed
@r4mmer r4mmer deleted the fix/ledger-unresponsive branch January 9, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Ledger unresponsive for api calls
3 participants