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

Parameterize Tx data-type over the wallet core engine (allowing to work with different representations) #451

Merged
merged 3 commits into from
Jun 22, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jun 21, 2019

Issue Number

#219

Overview

  • Parameterize wallet core engine over the transaction data-type

This PR defines a new abstraction layer via the class

    class (NFData (Tx t), Show (Tx t), Ord (Tx t)) => DefineTx t where
        type Tx t :: *
        txId :: Tx t -> Hash "Tx"
        inputs :: Tx t -> [TxIn]
        outputs :: Tx t -> [TxOut]

This allows for leaving the transaction shape rather 'open' and
define by whoever is implementing the wallet layer, so long as
it provides a few accessors on the transaction type and some
mandatory type-classes used across the wallet.

This allows for supporting both cardano-http-bridge and jormungandr
at the same time (and likely, add support for shelley-haskell
without too much issues).

One point remains fairly hard to abstract at the moment and it's the
DBLayer, or more exactly, the SQLite implementation. Since it's defined
in cardano-wallet-core, we can't -- in theory -- assume anything
about the shape of the transaction data-type. This makes it hard to
prepare the shape of the database without any prior knowledge of the
data-type. We cheat a bit by defining an extra class in the SQLite
module:

    class DefineTx t => PersistTx t where
        resolvedInputs :: Tx t -> [(TxIn, Maybe Coin)]
        mkTx :: [(TxIn, Maybe Coin)] -> [TxOut] -> Tx t

This assumes a specific shape for the transaction but is still flexible
enough that this assumption isn't too pervasive in the SQLite code. In
practice, this is sufficient for our needs, so we may not bother any
further.

  • Define new modules to host the Tx data-types used by different targets

    • Cardano.Wallet.DummyTarget.Primitive.Types (used in tests for core:unit)
    • Cardano.Wallet.HttpBridge.Primitive.Types (Tx as we know it)
    • Cardano.Wallet.Jormungandr.Primitive.Types (Tx, but TxIn have an additional Coin attached to them, so they are closer to what we used to call ResolvedInputs).
  • Disabled Jormungandr golden tests for TxId
    The tests golden strings were not generated using jcli but using our own implementation, so they aren't testing anything. For now disabled, but will re-work them and generate strings from jcli.

Comments

@KtorZ KtorZ self-assigned this Jun 21, 2019
@@ -163,16 +163,18 @@ test-suite unit
exitcode-stdio-1.0
hs-source-dirs:
test/unit
test/shared
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've factored out common stuff related to DummyTarget in a shared module.

instance Buildable Tx where
build (Tx ins outs) = mempty
<> blockListF' "~>" build ins
<> blockListF' "<~" build outs
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 into http-bridge/src/Cardano/Wallet/HttpBridge/Primitive/Types.hs

}
, transactions = []
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Factored out into Moved into Cardano/Wallet/DummyTarget/Primitive/Types.hs

where
decodingError _ = TextDecodingError
"Unable to decode Address: expected Base16 encoding"

Copy link
Member Author

Choose a reason for hiding this comment

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

Factored out into Moved into Cardano/Wallet/DummyTarget/Primitive/Types.hs

, prevBlockHash = Hash "genesis"
}
, transactions = []
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Factored out into Moved into Cardano/Wallet/DummyTarget/Primitive/Types.hs

}

instance TxId DummyTarget where
txId = Hash . B8.pack . show
Copy link
Member Author

Choose a reason for hiding this comment

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

Factored out into Moved into Cardano/Wallet/DummyTarget/Primitive/Types.hs


instance KeyToAddress DummyTarget where
keyToAddress = Address . unXPub . getKey

Copy link
Member Author

Choose a reason for hiding this comment

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

Factored out into Moved into Cardano/Wallet/DummyTarget/Primitive/Types.hs

\ - ~> 1st c29d3ea0...13862214\n\
\ <~ 3823755953610 @ 82d81858...aebb3709\n\
\ <~ 19999800000 @ 82d81858...37ce9c60\n"
=== pretty @_ @Text block
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 into http-bridge unit tests

@KtorZ KtorZ force-pushed the KtorZ/parameterize-tx branch 2 times, most recently from 5fdf0ba to 849f5a1 Compare June 21, 2019 14:35
KtorZ added 2 commits June 21, 2019 20:57
This commit defines a new abstraction layer via the class

    class (NFData (Tx t), Show (Tx t), Ord (Tx t)) => DefineTx t where
        type Tx t :: *
        txId :: Tx t -> Hash "Tx"
        inputs :: Tx t -> [TxIn]
        outputs :: Tx t -> [TxOut]

This allows for leaving the transaction shape rather 'open' and
define by whoever is implementing the wallet layer, so long as
it provides a few accessors on the transaction type and some
mandatory type-classes used across the wallet.

This allows for supporting both cardano-http-bridge and jormungandr
at the same time (and likely, add support for shelley-haskell
without too much issues).

One point remains fairly hard to abstract at the moment and it's the
DBLayer, or more exactly, the SQLite implementation. Since it's defined
in `cardano-wallet-core`, we can't -- in theory -- assume anything
about the shape of the transaction data-type. This makes it hard to
prepare the shape of the database without any prior knowledge of the
data-type. We cheat a bit by defining an extra class in the SQLite
module:

    class DefineTx t => PersistTx t where
        resolvedInputs :: Tx t -> [(W.TxIn, Maybe W.Coin)]
        mkTx :: [(W.TxIn, Maybe W.Coin)] -> [W.TxOut] -> Tx t

This assumes a specific shape for the transaction but is still flexible
enough that this assumption isn't too pervasive in the SQLite code. In
practice, this is _sufficient_ for our needs, so we may not bother any
further.
@KtorZ KtorZ force-pushed the KtorZ/parameterize-tx branch from 849f5a1 to 1f74545 Compare June 21, 2019 18:58
@KtorZ KtorZ force-pushed the KtorZ/parameterize-tx branch from 1eebd1f to ad34c83 Compare June 21, 2019 21:15
Copy link
Contributor

@akegalj akegalj left a comment

Choose a reason for hiding this comment

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

lgtm

txInputTableOrder Int sql=order
txInputTableSourceTxId TxId sql=source_id
txInputTableSourceIndex Word32 sql=source_index
txInputTableSourceAmount W.Coin Maybe sql=source_amount default=NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

did this slip through? Looks like something for another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, unfortunately. The syntax is a bit weird but that's how we define nullable column with the persistent DSL. This allows for defining a single db schema for both http-bridge and jormungandr.

The difference is actually so small here that I thought this would be better than maintaining two distinct database tables.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

@KtorZ KtorZ merged commit 8664784 into master Jun 22, 2019
@KtorZ KtorZ deleted the KtorZ/parameterize-tx branch June 22, 2019 12:36
@KtorZ KtorZ mentioned this pull request Jun 22, 2019
12 tasks
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.

2 participants