Skip to content
This repository was archived by the owner on Aug 18, 2020. It is now read-only.

Ktor z/chw 83/address path handler #2910

Closed
wants to merge 5 commits into from
Closed

Ktor z/chw 83/address path handler #2910

wants to merge 5 commits into from

Conversation

@KtorZ
Copy link
Contributor

@KtorZ KtorZ commented May 11, 2018

Description

Implement handler for creating new address path according to BIP-44 specs.

NOTE I started to work from CHW-37 branch which turns out not to be the ideal branch. @denisshevchenko is rebasing a more accurate branch on top of develop at the moment so ideally, the few commits added by this PR should be cherry-picked there and the base target should be changed accordingly!

NOTE #2 The linter currently fails in the integration tests because of code duplication. I know however that we've changed quite a lot of stuff in those files and therefore, this will most probably be fixed upon rebasing + adopting new style.

Linked issue

[CHW-83]

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

image

KtorZ added 2 commits May 10, 2018 19:14
@KtorZ
[CHW-83] Add extra AccountIndex to API for creating address path
df1fe89 
Even though hardware-wallet are likely to have only one account for now, we might prevent some future troubles by simply
making the account index explicit here.
@KtorZ
[CHW-83] Draft first implementation of address path handler
4b7817a 
This commits exposes some opaque structure from V1/Types. This is a trade-off between keeping the type definition
simple but still removing a whole category of errors. By exposing only smart constructors, we prevent
wrong AddressPath to be created. Likewise, we don't expose the internal of AddressLevel; in essence, they're just
Word32, but actually divided in two groups (normal for the first 2^31 bytes, and hardened for the other half).

In the current state, we _can't_ create AddressLevel on their own outside of the V1/Types module. To be created,
one necessarily has to use the mkAddresssPathBIP44 smart constructor.
@KtorZ KtorZ self-assigned this May 11, 2018
@KtorZ KtorZ requested a review from denisshevchenko May 11, 2018 12:14
@KtorZ KtorZ requested a review from adinapoli-iohk as a code owner May 11, 2018 12:14
@KtorZ KtorZ force-pushed the KtorZ/CHW-83/address-path-handler branch from 0e9b8ba to 7d653d1 Compare May 11, 2018 12:15
KtorZ added 3 commits May 11, 2018 14:23
@KtorZ
[CHW-83] Translate account index in the normal range
f70ca70 
We actually already use the hardened notation (2^31 + i) for our account indexes. However, this new implementation
for AddressPath make this expicit in the code while the API returns the expected key in the right notation
@KtorZ
[CHW-83] Add integration for new address-path endpoint
f15a231
@KtorZ
[CHW-83] Re-generate pkgs/hfsevents.nix
fa32173
@KtorZ KtorZ force-pushed the KtorZ/CHW-83/address-path-handler branch from 7d653d1 to fa32173 Compare May 11, 2018 12:24
@denisshevchenko denisshevchenko mentioned this pull request May 14, 2018
Closed
11 tasks
@denisshevchenko
Copy link

denisshevchenko commented May 14, 2018

Closed in favor of #2924.

@denisshevchenko denisshevchenko deleted the KtorZ/CHW-83/address-path-handler branch May 14, 2018 10:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@denisshevchenko denisshevchenko Awaiting requested review from denisshevchenko

@adinapoli-iohk adinapoli-iohk Awaiting requested review from adinapoli-iohk

Assignees

@KtorZ KtorZ

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KtorZ @denisshevchenko