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

Address Discovery (Sequential) #51

Merged

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Mar 12, 2019

Issue Number

#22

Overview

  • I have ported the 'AddressPoolGap' and 'AddressPool' modules from cardano-wallet-legacy
  • I have merged both modules in once
  • I have reviewed the API for the AddressPool module according to the findings in the prototype, also making it a plain data-structure rather than accepting a function in its internal representation
  • I have re-worked quite a lot the property tests for this module to be a bit more useful and test actual things

Comments

@KtorZ KtorZ self-assigned this Mar 12, 2019
@KtorZ KtorZ changed the title Address Discovery Address Discovery (Sequential) Mar 12, 2019
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM!

AddressPoolGap (toEnum g)

-- | Smart constructor for 'AddressPoolGap'
mkAddressPoolGap :: Word8 -> Either MkAddressPoolGapError AddressPoolGap
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we want :

mkAddressPoolGap 
    :: Word8 
    -> Either MkAddressPoolGapError AddressPoolGap

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily. The line is still within 80 chars, so we don't necessarily have to wrap it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

-- In practice, we always have:
--
-- > mkAddressPool key g cc (addresses pool) == pool
addresses :: AddressPool -> [Address]
Copy link
Contributor

Choose a reason for hiding this comment

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

the same comment as above

| otherwise = pool
where
edge = Map.size (indexedAddresses pool)
isOnEdge = edge - fromEnum ix <= fromEnum (gap pool)
Copy link
Contributor

Choose a reason for hiding this comment

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

the definition of isOnEdge follows from the standard?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the specification is rather evasive about how this should be handled.

- scan addresses of the external chain; respect the gap limit described below
- if no transactions are found on the external chain, stop discovery

The approach above is how we've translated this, and works fairly well (cf property tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

newAddress = keyToAddress . deriveAddressPublicKey key cc

-- | Fails hard if an invariant does not hold, or proceed.
invariant
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it is good idea to extract invariant function in helper file or use the one already defined in src/Cardano/Wallet.hs :

173:invariant
174-    :: String -- ^ A title / message to throw in case of violation
175-    -> a
176-    -> (a -> Bool)
177-    -> a
178:invariant msg a predicate

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. My bad. I intended to use the one from src/Cardano/Wallet.hs indeed, though I hadn't rebase yet at this moment and just copy/pasted it quickly. Will remove 👍

@KtorZ KtorZ force-pushed the KtorZ/#21/ed25519-address-derivation branch from 35cb00e to 63e9070 Compare March 13, 2019 15:44
@KtorZ KtorZ force-pushed the KtorZ/#22/port-address-discovery branch from e89cd11 to 1a5805c Compare March 13, 2019 15:54
@KtorZ KtorZ force-pushed the KtorZ/#22/port-address-discovery branch from 1a5805c to d0b1c37 Compare March 13, 2019 16:00
@KtorZ KtorZ merged commit b554949 into KtorZ/#21/ed25519-address-derivation Mar 13, 2019
@KtorZ KtorZ deleted the KtorZ/#22/port-address-discovery branch March 13, 2019 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants