Skip to content

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

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
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
Loading

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
Loading

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

buberdds commented Mar 24, 2022

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
Loading

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

@lukaw3d lukaw3d Apr 6, 2022

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

@lukaw3d lukaw3d Apr 6, 2022

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

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

@lukaw3d lukaw3d Apr 6, 2022

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
nitindudeja2107
nitindudeja2107 approved these changes Aug 13, 2022
View reviewed changes
Copy link

@nitindudeja2107 nitindudeja2107 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

#899 merge
@nitindudeja2107 nitindudeja2107 mentioned this pull request Aug 14, 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

Labels

None yet

6 participants

@matevz @buberdds @kostko @lukaw3d @nitindudeja2107