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

SIWE | Sign-in With Ethereum - Add Ledger Instructions #18589

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Apr 14, 2023

Explanation

Fixes #18552

The original implementation was missing the Ledger instructions. This PR adds the supporting component.

Screenshots/Screencaps

Before

(see Issue related #18552)

After

Screen Shot 2023-04-14 at 12 35 19 AM

Manual Testing Steps

  1. Connect with Ledger
  2. Create sign-in with ethereum request
  3. Observe ledger instructions UI

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.

@digiwand digiwand requested a review from a team as a code owner April 14, 2023 03:40
@digiwand digiwand requested a review from hmalik88 April 14, 2023 03:40
@github-actions
Copy link
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
Collaborator

Builds ready [f2834fc]
Page Load Metrics (1967 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1031901372211
domContentLoaded15622291195219895
load15622336196720799
domInteractive15622291195219895
Bundle size diffs
  • background: 0 bytes
  • ui: 488 bytes
  • common: 0 bytes

Copy link
Contributor

@NiranjanaBinoy NiranjanaBinoy left a comment

Choose a reason for hiding this comment

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

I don't have a ledger, so I haven't tested this. But the code changes look good, so I am approving this one.

@digiwand
Copy link
Contributor Author

thanks for the reviews!

@digiwand digiwand merged commit 1406885 into develop Apr 15, 2023
@digiwand digiwand deleted the siwe-add-ledger-instructions branch April 15, 2023 00:10
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2023
@PeterYinusa PeterYinusa added the release-10.29.0 Issue or pull request that will be included in release 10.29.0 label Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signatures rc-cherry-picked release-10.29.0 Issue or pull request that will be included in release 10.29.0 team-confirmations-secure-ux-PR PRs from the confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: SIWE does not show Ledger dialog
6 participants