Skip to content

Include error string from ledger errors#9911

Merged
tmashuang merged 3 commits intodevelopfrom
hardware-wallet-error-message
Nov 19, 2020
Merged

Include error string from ledger errors#9911
tmashuang merged 3 commits intodevelopfrom
hardware-wallet-error-message

Conversation

@tmashuang
Copy link
Copy Markdown
Contributor

@tmashuang tmashuang commented Nov 18, 2020

Also remove the componentMount to check hardware status. This would constantly check the status of the hardware wallet upon mounting, even after the wallet is unplugged.

Manual testing steps on Win10/Firefox

  1. Do not plug in ledger
  2. Select Ledger and attempt to connect even though one is not connected.
    2.1 Win10 users will need to cancel/close(x) the Windows Security window for usb.
  3. After some time, the error TypeError: t is undefined will log to the console, and the error message won't render,
    renderError() {
    if (this.state.error === U2F_ERROR) {
    return (
    <p className="hw-connect__error">
    {this.context.t('troubleConnectingToWallet', [
    this.state.device,
    // eslint-disable-next-line react/jsx-key
    <a
    href="https://metamask.zendesk.com/hc/en-us/articles/360020394612-How-to-connect-a-Trezor-or-Ledger-Hardware-Wallet"
    key="hardware-connection-guide"
    target="_blank"
    rel="noopener noreferrer"
    className="hw-connect__link"
    style={{ marginLeft: '5px', marginRight: '5px' }}
    >
    {this.context.t('walletConnectionGuide')}
    </a>,
    ])}
    </p>
    )
    }

Also remove the componentMount to check hardware status. This would constantly check the status of the hardware wallet upon mounting, even after the wallet is unplugged.
@tmashuang tmashuang requested a review from a team as a code owner November 18, 2020 22:12
@github-actions
Copy link
Copy Markdown
Contributor

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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [60d3d98]
Page Load Metrics (357 ± 34 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint329947199
domContentLoaded2625633557134
load2635643577134
domInteractive2615623557134

@tmashuang
Copy link
Copy Markdown
Contributor Author

tmashuang commented Nov 18, 2020

I removed the entire componentDidMount that calls checkIfUnlocked since it seemed a little extraneous. It would constantly attempt to connect after a Ledger account has already been imported, and after refreshing the page it would attempt to connect as well. If this is extraneous to the PR, I can separate it out.

// eslint-disable-next-line react/jsx-key
<a
href="https://metamask.zendesk.com/hc/en-us/articles/360020394612-How-to-connect-a-Trezor-or-Ledger-Hardware-Wallet"
key="hardware-connection-guide"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe this fixes the react warning for a unique key.

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Nov 19, 2020

I removed the entire componentDidMount that calls checkIfUnlocked since it seemed a little extraneous. It would constantly attempt to connect after a Ledger account has already been imported, and after refreshing the page it would attempt to connect as well. If this is extraneous to the PR, I can separate it out.

This was added in #4851. I'm guessing that the purpose of checking whether the device is locked upon mount was to know whether to call showTemporaryAlert later on. This function shows an auto-hiding alert that informs the user that the device has connected. Without this check upon mount, this will happen upon every page load, instead of just in cases where the user unlocks the device while still on the page.

As janky as this is, it might be better to leave it alone until we're ready to delete this "temporary alert" altogether, or verify it still works as intended. Certainly seems better to leave it for another PR.

tmashuang and others added 2 commits November 18, 2020 22:27
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [10064ce]
Page Load Metrics (353 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint309039136
domContentLoaded24460135210349
load24660235310349
domInteractive24460135110349

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@tmashuang tmashuang merged commit 75de908 into develop Nov 19, 2020
@tmashuang tmashuang deleted the hardware-wallet-error-message branch November 19, 2020 15:35
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants