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.

Sign up for GitHub

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

Jump to bottom

feature: Add ADR 0008 option for opening Ledger wallet #761

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feature: Add ADR 0008 option for opening Ledger wallet #761

wants to merge 2 commits into from

Conversation

matevz
Copy link
Member

@matevz matevz commented Mar 18, 2022 •
edited

Fixes #760

Merge after #792 is implemented and show ADR8 option only when the user enables some "advanced" mode.

before after
Screenshot_20220328_164459 Peek 2022-03-30 10-40

Depends on Ledger docs update oasisprotocol/docs#57

@github-actions
Copy link

github-actions bot commented Mar 18, 2022 •
edited

MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 16 0 0.77s
✅ GIT git_diff yes no 0.04s
✅ JSON eslint-plugin-jsonc 3 0 0 0.98s
✅ JSON jsonlint 3 0 0.75s
✅ JSON prettier 3 0 0 0.67s
✅ JSON v8r 3 0 2.56s
✅ TSX eslint 2 0 0 6.06s
✅ TYPESCRIPT eslint 9 0 0 4.41s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

@matevz matevz force-pushed the matevz/feature/ledger-adr8 branch 2 times, most recently from 68ea3e5 to 0271de2 Compare March 24, 2022 13:10
@matevz matevz marked this pull request as ready for review March 24, 2022 14:38
@matevz matevz force-pushed the matevz/feature/ledger-adr8 branch from 166ec41 to ab06b8c Compare March 24, 2022 14:51
@buberdds
Copy link
Contributor

you need to bump snapshots and a few saga tests are failing

@matevz matevz force-pushed the matevz/feature/ledger-adr8 branch 3 times, most recently from 16505a3 to f2f6bca Compare March 29, 2022 10:52
@codecov
Copy link

codecov bot commented Mar 29, 2022 •
edited

Codecov Report

Merging #761 (fdca15d) into master (316d9b7) will decrease coverage by 0.35%.
The diff coverage is 67.74%.

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

@@            Coverage Diff             @@
##           master     #761      +/-   ##
==========================================
- Coverage   88.24%   87.88%   -0.36%     
==========================================
  Files         100      100              
  Lines        1693     1717      +24     
  Branches      338      341       +3     
==========================================
+ Hits         1494     1509      +15     
- Misses        199      208       +9
Flag Coverage Δ
cypress 57.29% <16.12%> (-0.60%) ⬇️
jest 74.72% <67.74%> (-0.18%) ⬇️

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

Impacted Files Coverage Δ
src/app/state/ledger/index.ts 50.00% <0.00%> (-10.87%) ⬇️
src/types/errors.ts 100.00% <ø> (ø)
...pages/OpenWalletPage/Features/FromLedger/index.tsx 69.56% <75.00%> (-0.44%) ⬇️
src/app/state/ledger/saga.ts 97.82% <75.00%> (-2.18%) ⬇️
src/app/lib/ledger.ts 97.95% <90.90%> (-2.05%) ⬇️
...ponents/Toolbar/Features/AccountSelector/index.tsx 100.00% <100.00%> (ø)
src/app/state/ledger/selectors.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a29bd1a...9f3bb9c. Read the comment docs.

@matevz matevz force-pushed the matevz/feature/ledger-adr8 branch from dd0747c to fdca15d Compare March 30, 2022 08:12
@matevz matevz force-pushed the matevz/feature/ledger-adr8 branch from fdca15d to 9f3bb9c Compare March 30, 2022 08:13
@matevz matevz changed the title feature: Add ADR8 option for opening Ledger wallet feature: Add ADR 0008 option for opening Ledger wallet Mar 30, 2022
@kostko
Copy link
Member

kostko commented Mar 30, 2022

Task linked: CU-2gf4peq Add support to Oasis Wallet - Web

lukaw3d
lukaw3d reviewed Apr 6, 2022
View reviewed changes
src/locales/fr/translation.json
Comment on lines +96 to +100
"send": "Envoyer",
"confirmSendingToValidator": {
"title": "",
"description": ""
}
Copy link
Member

Choose a reason for hiding this comment

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

Empty translations show up nothing. Missing translations fallback to English.

lukaw3d
lukaw3d reviewed Apr 6, 2022
View reviewed changes
src/app/lib/ledger.ts
Comment on lines +40 to +46
public static mustGetPath(pathType: string, i: number) {
switch (pathType) {
case DerivationPathTypeAdr8:
return [44, 474, i]
case DerivationPathTypeLegacy:
return [44, 474, 0, 0, i]
}
Copy link
Member

Choose a reason for hiding this comment

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

Enum would be a better type

export enum DerivationPathType {
  Adr8 = 'adr8',
  Legacy = 'legacy',
}

  public static mustGetPath(pathType: DerivationPathType, i: number) {
    switch (pathType) {
      case DerivationPathType.Adr8:
        return [44, 474, i]
      case DerivationPathType.Legacy:
        return [44, 474, 0, 0, i]
    }

src/app/lib/ledger.ts
@@ -34,7 +37,18 @@ const successOrThrow = (response: Response, message: string) => {
}

export class Ledger {
public static async enumerateAccounts(transport: Transport, count = 5) {
public static mustGetPath(pathType: string, i: number) {
Copy link
Member

Choose a reason for hiding this comment

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

"must" is a bit odd. I'd just name it getPath

@matevz matevz mentioned this pull request Jul 8, 2022
Closed
@matevz matevz mentioned this pull request Aug 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaw3d lukaw3d lukaw3d left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for ADR8 derivation path on Ledger
4 participants
@matevz @buberdds @kostko @lukaw3d