Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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.

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

Open
matevz wants to merge 2 commits into master
base: master
Choose a base branch
Loading
from matevz/feature/ledger-adr8

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

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
Copy link
Contributor

you need to bump snapshots and a few saga tests are failing
matevz reacted with thumbs up emoji

@matevz matevz force-pushed the matevz/feature/ledger-adr8 branch 3 times, most recently from 16505a3 to f2f6bca Compare March 29, 2022 10:52
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
Copy link
Member

kostko commented Mar 30, 2022

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.

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]
 }


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

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

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

+1 more reviewer

@nitindudeja2107 nitindudeja2107 nitindudeja2107 approved these changes

Reviewers whose approvals may not affect merge requirements

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

AltStyle によって変換されたページ (->オリジナル) /