17649 - Hardware wallet signing state

[flexa-rob]

Explanation

Issue explained via story:

1. User initiates a signing request with an unlocked Ledger
2. Confirmation Modal pops up, and no warning is present because the Ledger is unlocked
3. Ledger locks while the Confirmation is open (e.g. the user may be hesitating the confirmation for a bit, or may be playing around with fees etc.)
4. User hits "Sign" and the signing request fails silently as the Ledger is now locked

Fixes:
#17649
#17630
#17606

See related:
#17937
#17940

Screenshots/Screencaps

Before

Signing request was allowed to proceed with a locked device:

Resulting in a silent error and no result:

After

Signing requests now monitor HD wallet state and displays an appropriate error message when locked.

Personal Sign:

Sign Typed Data:

Sign Typed Data V3:

Sign Typed Data V4:

Sign in With Ethereum:

Send Transaction:

Manual Testing Steps

1. Initiate signing request with Ledger
2. While on the confirm screen, wait for the Ledger to lock. Signing request should be blocked with ledgerLocked warning message showing
3. Unlock Ledger. The ledgerLocked warning should no longer be present and the signing request should now be able to proceed
4. Sign request and proceed as normal

Pre-merge author checklist

• I've clearly explained:
  • What problem this PR is solving
  • How this problem was solved
  • How reviewers can test my changes
• Sufficient automated test coverage has been added

Pre-merge reviewer checklist

• Manual testing (e.g. pull and build branch, run in browser, test code being changed)
• PR is linked to the appropriate GitHub issue
• IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[georgewrmarshall]

Still need to do a comprehensive review and check out the branch but one suggestion. Great work on the story creation!

[metamaskbot]

Builds ready [1a2b103]

• builds: chrome, firefox
• builds (beta): chrome, firefox
• builds (flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4
  • common: 0, 1, 2, 3, 4, 5
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1798 ± 170 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	302	150	59	28
		domContentLoaded	1373	2514	1764	365	175
		load	1373	2515	1798	354	170
		domInteractive	1373	2514	1764	365	175

Bundle size diffs [🚨 Warning! Bundle size has increased!]

• background: 273 bytes
• ui: 5891 bytes
• common: 124 bytes

[metamaskbot]

Builds ready [36814e7]

• builds: chrome, firefox
• builds (beta): chrome, firefox
• builds (flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4
  • common: 0, 1, 2, 3, 4, 5
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1427 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	92	130	106	10	5
		domContentLoaded	1198	1564	1406	83	40
		load	1198	1565	1427	90	43
		domInteractive	1198	1564	1406	83	40

Bundle size diffs [🚨 Warning! Bundle size has increased!]

• background: 273 bytes
• ui: 5938 bytes
• common: 124 bytes

[codecov-commenter]

Codecov Report

Merging #17786 (e06f915) into develop (26d7520) will increase coverage by 1.01%.
The diff coverage is 84.68%.

❗ Current head e06f915 differs from pull request most recent head 9a64a71. Consider uploading reports for the commit 9a64a71 to get more accurate results

@@             Coverage Diff             @@
##           develop   #17786      +/-   ##
===========================================
+ Coverage    69.38%   70.38%   +1.01%     
===========================================
  Files          987      962      -25     
  Lines        37267    36950     -317     
  Branches     10008     9620     -388     
===========================================
+ Hits         25854    26006     +152     
+ Misses       11413    10944     -469     

Impacted Files	Coverage Δ
ui/pages/create-account/connect-hardware/index.js	42.47% <0.00%> (-0.29%)	⬇️
...p/signature-request-siwe/signature-request-siwe.js	63.27% <50.00%> (-8.66%)	⬇️
...p/signature-request/signature-request.component.js	77.22% <60.00%> (ø)
...saction-base/confirm-transaction-base.component.js	48.16% <60.00%> (-9.18%)	⬇️
...t-original/signature-request-original.component.js	54.72% <66.67%> (-5.28%)	⬇️
ui/ducks/app/app.ts	64.75% <66.67%> (+2.25%)	⬆️
app/scripts/metamask-controller.js	67.84% <76.92%> (+1.87%)	⬆️
...app/hardware-wallet-state/hardware-wallet-state.js	96.77% <96.77%> (ø)
shared/constants/hardware-wallets.ts	100.00% <100.00%> (ø)
...dger-instruction-field/ledger-instruction-field.js	60.53% <100.00%> (+1.62%)	⬆️
... and 6 more

... and 354 files with indirect coverage changes

[metamaskbot]

Builds ready [330639c]

• builds: chrome, firefox
• builds (beta): chrome, firefox
• builds (flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4
  • common: 0, 1, 2, 3, 4, 5
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1597 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	91	154	119	19	9
		domContentLoaded	1253	1745	1564	102	49
		load	1253	1745	1597	105	50
		domInteractive	1253	1745	1564	102	49

Bundle size diffs [🚨 Warning! Bundle size has increased!]

• background: 273 bytes
• ui: 5938 bytes
• common: 124 bytes

[georgewrmarshall]

Looking great! Left a few minor suggestions. Travelling right now so don't have a hardware wallet on me to test locally so will need another approval to test funcitonality. I think it's out of the scope of this ticket but it would be great to update this notification to use BannerAlert as well. Might need some design attention 😅

[metamaskbot]

Builds ready [22522eb]

• builds: chrome, firefox
• builds (beta): chrome, firefox
• builds (flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4
  • common: 0, 1, 2, 3, 4, 5
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1890 ± 279 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	94	352	152	81	39
		domContentLoaded	1264	3034	1860	573	275
		load	1347	3045	1890	580	279
		domInteractive	1264	3034	1860	573	275

[flexa-rob]

Looking great! Left a few minor suggestions. Travelling right now so don't have a hardware wallet on me to test locally so will need another approval to test funcitonality. I think it's out of the scope of this ticket but it would be great to update this notification to use BannerAlert as well. Might need some design attention sweat_smile

Thanks @georgewrmarshall - I just looked into updating the ledger-instruction-field too.

I'd be happy to do this, though would recommend that it be implemented in a separate PR - it uses Typography under the covers with some Button functionality in certain cases. So, there's probably enough effort (and testing) to warrant a separate PR/issue, if that's ok with you?

[georgewrmarshall]

@flexa-rob absolutely! Created another ticket here #17873

[Silvrbckw]

Commit and push

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

[github-actions]

This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions.