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

Minimal Viable Wallet Layer #43

Merged
merged 4 commits into from
Mar 12, 2019
Merged

Minimal Viable Wallet Layer #43

merged 4 commits into from
Mar 12, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Mar 8, 2019

Issue Number

#20

Overview

  • I have implemented a minimal viable wallet layer with prefiltering.
  • I have added some 'Buildable' instances to get nicely formatted outputs of things that hard to read through Show instances (like the UTxO, cf comments)
  • I have implement a few test using an excerpt of mainnet to validate applyBlock and compare it to a basic model.

Comments

UTxO through Buildable looks like the following:

    UTxO:
  - 1st fd545d05...da07612a ==>     3834435886614 @ DdzFFzCq...jve3ciMp
  - 2nd fd545d05...da07612a ==>        9999800000 @ DdzFFzCq...8N659Bm3

return $ if predicate then Just out else Nothing

forMaybe :: Monad m => [a] -> (a -> m (Maybe b)) -> m [b]
forMaybe xs = fmap catMaybes . for xs
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to Cardano/Wallet

@KtorZ KtorZ requested a review from paweljakubas March 8, 2019 16:32
@rvl rvl added this to the Support Wallet Creation milestone 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. Have some questions to understand the code better plus indicated minor typos.

, binary
, bytestring
, cborg
, containers
, cryptonite
, deepseq
, digest
, fmt
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet there are some strong argument in favor of using fmt vs formatting?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some arguments. See #sl-core (I'll send you a link in PM)

-- License: MIT
--
-- Here we find the "business logic" to manage a Cardano wallet. This is a
-- direct implementation of the model from the [Formal Specification for a Cardano Wallet](https://github.com/input-output-hk/cardano-wallet/blob/master/specifications/wallet/formal-specification-for-a-cardano-wallet.pdf)
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 to split it into two lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish but we can't split the link :(

-- k = 2160 is currently hard-coded here. In the short-long run, we do
-- want to get that as an argument or, leave that decision to the caller
-- though it is not trivial at all. If it shrinks, it's okay because we
-- have enough checkpoints, but if it does increase, then we have
Copy link
Contributor

Choose a reason for hiding this comment

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

you mean the problematic situation would be: the caller has an ability to set k in applyBlocks? and the wallet persists the state and then is called with higher k at some point, and for some period it misses checkpoints to be compliant with new k parameter. And then rollback comes that has depth covering those missing checkpoints? Maybe in that case k->k' , where k'>k some kind of "restotration" should take place to compensate it....

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I meant yes. So, there are different way to deal with this indeed, but it's not trivial, hence the warning.


-- | A polymorphic wrapper type with a custom show instance to display data
-- through 'Buildable' instances.
newtype ShowFmt a = ShowFmt a
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is because we do not want to deliver separate instances of Show for Utxo, Adresses, Block,... Instead we wrap them in ShowFmt and in one go use any type's Buildable instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is because we do not want to write custom show instances in practice. Though, because of the way QuickCheck, HSpec and some other libraries work, they use Show to output results and counterexamples which is very unpractical to read for some types (like the UTxO). For those, we may opt-in for a ShowFmt wrapper around a type to get a nice output. Though, we don't want to be changing the stock Show instance for those types.

{-------------------------------------------------------------------------------
Basic Model - See Wallet Specification, section 3

Our implementation of 'applyBlock' is a bit more complexe than the basic
Copy link
Contributor

Choose a reason for hiding this comment

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

complexe - typo

prefilterBlock b (Wallet !utxo _ !s) =
let
txs = transactions b
(ourOuts, s') = txOutsOurs txs s
Copy link
Contributor

Choose a reason for hiding this comment

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

in the previous code prefiltering acted on the whole set of transactions. now we make sure we intersect only on those belonging to us. Nice!

{-------------------------------------------------------------------------------
Test Data

In practice, we may want to generate arbitrary valid sequences of block.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe block sequences instead of sequences of block

test/unit/Cardano/WalletSpec.hs Show resolved Hide resolved
@KtorZ KtorZ merged commit d8b34e8 into master Mar 12, 2019
@KtorZ KtorZ deleted the KtorZ/#20/wallet-layer branch March 12, 2019 10:56
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

3 participants