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

Add derivation path to api address #2598

Merged

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Apr 2, 2021

Issue Number

adp-766

Overview

  • changed the definition of ApiAddress, and handled core code adjustments
  • aligned unit tests
  • aligned integration testing, added some tests

Comments

@paweljakubas paweljakubas requested a review from KtorZ April 2, 2021 09:48
@paweljakubas paweljakubas self-assigned this Apr 2, 2021
@paweljakubas paweljakubas added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Apr 2, 2021
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

LGTM. Some minor suggestions left.

w <- fixture ctx
r <- request @[ApiAddress n] ctx (Link.listAddresses @'Byron w) Default Empty
verify r [ expectResponseCode HTTP.status200 ]
let n = length $ getFromResponse id r
forM_ [0..n-1] $ \addrIx -> do
expectListField addrIx #state (`shouldBe` ApiT Unused) r
expectListField addrIx #derivationPath (\derPath -> NE.length derPath `shouldBe` derPathSize) r
Copy link
Member

Choose a reason for hiding this comment

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

👍

toDerivationPath ix = NE.fromList $ map DerivationIndex
[ getIndex purposeBIP44
, getIndex purposeCIP1852
, 0
Copy link
Member

Choose a reason for hiding this comment

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

Okay-ish because we assume the first account. I'd however rather leave a note as comment explaining this hard-coded value.

constructAddrWithPath path (addr,state) acc =
(addr, state, toDerivationIndexes path):acc
retrieveAddrsWithPaPath =
Map.foldrWithKey constructAddrWithPath []
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion to avoid the extra traversal on pending addresses and also, make the function a bit more symmetric to more clearly see how the output is constructed:

instance KnownAddresses (RndState n) where
    knownAddresses s = mconcat
        [ toListWithPath (\path (addr, state) -> (addr, state, path))
            (discoveredAddresses s)
        , toListWithPath (\path addr -> (addr, Unused, path))
            (pendingAddresses s)
        ]
      where
        toListWithPath
            :: (NonEmpty DerivationIndex -> v -> result)
            -> Map DerivationPath v
            -> [result]
        toListWithPath mk =
            Map.foldrWithKey
                (\path v result -> mk (toDerivationIndexes path) v : result)
                []

@paweljakubas
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 2, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 95bbd84 into master Apr 2, 2021
@iohk-bors iohk-bors bot deleted the paweljakubas/ADP-766/add-derivation-path-to-api-address branch April 2, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants