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.

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

Add new address discovery schemes for testing and benchmarking #160

Merged
merged 4 commits into from
Apr 26, 2019
Merged

Add new address discovery schemes for testing and benchmarking #160

merged 4 commits into from
Apr 26, 2019

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Apr 8, 2019 •
edited
Loading

Relates to issue #100

Overview

This adds supplementary address discovery schemes for benchmarking.

Some things related to creating wallets needed to be moved around to accommodate.

@rvl rvl self-assigned this Apr 8, 2019
@rvl rvl force-pushed the rvl/100/address-discovery branch from b04b8cb to ea4d55b Compare April 9, 2019 09:20
@rvl rvl changed the title wip: Add new address discovery schemes for testing and benchmarking Add new address discovery schemes for testing and benchmarking Apr 9, 2019
@rvl rvl marked this pull request as ready for review April 9, 2019 09:24
@rvl rvl mentioned this pull request Apr 9, 2019
Closed
7 tasks
@KtorZ KtorZ force-pushed the rvl/100/bench branch 2 times, most recently from 97b16d1 to 714319b Compare April 9, 2019 12:13
@KtorZ KtorZ changed the base branch from rvl/100/bench to master April 9, 2019 12:32
@KtorZ KtorZ force-pushed the rvl/100/address-discovery branch from ea4d55b to 639e8c5 Compare April 9, 2019 12:46
@KtorZ
Copy link
Member

KtorZ commented Apr 9, 2019

@rvl I've changed the base the master and resolved conflicts.

KtorZ
KtorZ reviewed Apr 9, 2019
View reviewed changes
src/Cardano/Wallet.hs
} deriving (Show, Generic)
, addressDiscoveryConfig
:: AddressDiscoveryConfig s
}
Copy link
Member

@KtorZ KtorZ Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A suggestion here: let's remove the NewWallet type wrapper here and simply modify the wallet layer signature as follows:

        :: WalletName
        -> Key scheme 'Root0 XPrv
        -> Passphrase "encryption"
        -> s
        -> ExceptT (ErrWalletAlreadyExists "createWallet") IO WalletId

Leave the creation of the state to the caller 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK good idea. Turns out NewWallet is kind of helpful, so I have added a createWalletSeq :: WalletLayer SeqState -> NewWallet ExceptT ErrWalletAlreadyExists IO WalletId.

KtorZ
KtorZ reviewed Apr 9, 2019
View reviewed changes
src/Cardano/Wallet.hs Outdated
initState :: NewWallet s -> (WalletId, s)

instance InitState SeqState where
initState w = (wid, seqState)
Copy link
Member

Choose a reason for hiding this comment

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

We won't require a type-class here (such abstraction for the wallet scheme is actually hard to have in practice because the random cardano wallet and the sequential have rather different and not-so-compatible setup :(), but rather, let's make use of a helper function which yield a SeqState from a seed, generation passphrase and address pool gap.

initSeqState 
    :: (Passphrase "seed", Passphrase "generation") 
    -> AddressPoolGap 
    -> (Key 'Seq Root0 XPrv, SeqState)

This will play nicely with the previous comment and should be also flexible enough for testing 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK added, but the testing AD scheme(s) don't have any key. So I let the initSeqState function calculate the wallet ID.

@rvl rvl force-pushed the rvl/100/address-discovery branch 5 times, most recently from eadb5dc to b47eea2 Compare April 11, 2019 00:08
@KtorZ KtorZ force-pushed the rvl/100/address-discovery branch from b47eea2 to 854c571 Compare April 26, 2019 15:41
KtorZ
KtorZ approved these changes Apr 26, 2019
View reviewed changes
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.

reviewed & rebased on top of master

KtorZ and others added 4 commits April 26, 2019 17:52
@KtorZ
make the wallet layer state-agnostic (instead of requiring a 'SeqState')
c71257d
@KtorZ @rvl
Add stress restoration benchmarks
f63f8e4 
This creates a new 'special' address scheme that accept a given
proportion of addresses on the chain (e.g. 50%). This allows us to
artificially create very big wallets and observe how the wallet backend
survive and behave.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
@KtorZ
add extra property tests for the 'putPrivateKey' DB function
4bd416b
@KtorZ
slightly review README file (move testing bits to Wiki & document ben…
e3964f2 
…chmark)
@KtorZ KtorZ force-pushed the rvl/100/address-discovery branch from 854c571 to e3964f2 Compare April 26, 2019 15:52
@KtorZ KtorZ merged commit 9d81287 into master Apr 26, 2019
@KtorZ KtorZ deleted the rvl/100/address-discovery branch April 26, 2019 16:33
parsonsmatt added a commit to parsonsmatt/cardano-sl that referenced this pull request May 14, 2019
@iohk-bors @angerman
@parsonsmatt @KtorZ
Merge input-output-hk#3987
b8cae35 
3987: Parse required CLI params in cluster r=parsonsmatt a=parsonsmatt

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

## Linked issue

<!--- Put here the relevant issue from YouTrack -->

cardano-foundation/cardano-wallet#160



Co-authored-by: Moritz Angermann <moritz.angermann@gmail.com>
Co-authored-by: parsonsmatt <parsonsmatt@gmail.com>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ approved these changes

Assignees

@rvl rvl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rvl @KtorZ