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

SQL Schema #227

Merged
merged 12 commits into from
May 13, 2019
Merged

SQL Schema #227

merged 12 commits into from
May 13, 2019

Conversation

rvl
Copy link
Contributor

@rvl rvl commented May 7, 2019

Relates to #147

Overview

  • I have added schema for Wallet, TxInput, TxOutput, TxMeta, Utxo, Slot

Comments

@paweljakubas paweljakubas changed the title WIP SQL Schema SQL Schema May 7, 2019
@paweljakubas paweljakubas marked this pull request as ready for review May 7, 2019 13:25
KtorZ
KtorZ previously requested changes May 7, 2019
lib/core/cardano-wallet-core.cabal Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite.hs Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/SqliteTypes.hs Outdated Show resolved Hide resolved
stack.yaml Outdated Show resolved Hide resolved
@paweljakubas
Copy link
Contributor

just made compilation fix and rebased

@rvl rvl dismissed KtorZ’s stale review May 9, 2019 03:32

All comments ahve been addressed

Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

So far so good! 👍

I've added a few comments to the review.

Also, there are a few parts that should be fixed to adhere to the coding standards. I've highlighted these in the comments, but I also created a PR (#246) with the problems fixed, which you can use directly if desired.

lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs Outdated Show resolved Hide resolved
lib/text-class/test/unit/Data/Text/ClassSpec.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/SqliteTypes.hs Show resolved Hide resolved
lib/text-class/test/unit/Data/Text/ClassSpec.hs Outdated Show resolved Hide resolved
lib/text-class/test/unit/Data/Text/ClassSpec.hs Outdated Show resolved Hide resolved
instance PersistField AddressScheme where
toPersistValue = toPersistValue . schemeToBool
fromPersistValue pv = do
let err = T.pack $ "not a valid value: " <> show pv
Copy link
Member

Choose a reason for hiding this comment

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

You could probably write this as:

"not a valid value: " <> T.pack (show pv)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


schemeToBool :: AddressScheme -> Bool
schemeToBool Sequential = True
schemeToBool Random = False
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this can be renamed to isSchemeSequential. (Provide an equivalent isSchemeRandom if desired.)

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... Just seeing this bit from the PR, but I don't think a Bool is a wise choice for this one... We will likely introduce more schemes in the future (special ones for exchanges), and serializing this to Bool might bite us later. Also, we already have a 3rd scheme that we use in the benchmarks Any, and this will need to work with the DB as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used yet. We can turn it into an enum later.

Copy link
Contributor

Choose a reason for hiding this comment

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

changed to data AddressScheme = Sequential | Random | Any

schemeToBool Sequential = True
schemeToBool Random = False

schemeFromBool :: Bool -> AddressScheme
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this deserves a comment, to explain the exact mapping that will take place. (Important for someone reading the haddock documentation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@paweljakubas paweljakubas May 9, 2019

Choose a reason for hiding this comment

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

changed to data AddressScheme = Sequential | Random | Any

Copy link
Member

@jonathanknowles jonathanknowles May 9, 2019

Choose a reason for hiding this comment

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

Just to clarify. If I'm a casual user of this function, it currently isn't clear how a given Bool value will map onto an AddressScheme. There are two possible mappings:

  1. {True, False} -> {Sequential, Random}
  2. {True, False} -> {Random, Sequential}

To understand precisely which mapping is being used, I'd have to dig into the source code, which kind of defeats the point.

I would recommend either:

  1. Removing this function (as this mapping is somewhat arbitrary).
  2. Renaming it (so that the exact mapping performed is clear from the name).
  3. Adding a haddock documentation comment to make the mapping explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Introduced data AddressScheme = Sequential | Random | Any now relying on FromText, ToText - so basically opted for point 1 and 2 ;-)

@paweljakubas
Copy link
Contributor

@jonathanknowles @KtorZ if we are ok with the current state, I can rebase the branch and merge it

@KtorZ
Copy link
Member

KtorZ commented May 9, 2019

I haven't reviewed thoroughly any of this, just quick looks. I trust you guys on this :)

@paweljakubas paweljakubas force-pushed the rvl/147/sql-schema branch 2 times, most recently from d219095 to f2c65ad Compare May 10, 2019 13:33
-- blocks).
--
-- TxMeta is specific to a wallet because multiple wallets may have the same
-- transaction with different metdata values. The associated inputs and outputs
Copy link
Contributor

Choose a reason for hiding this comment

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

typo s/metdata/metadata/

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks! corrected

txMetaTableSlotId W.SlotId sql=slot_id
txMetaTableAmount W.Coin sql=amount

Primary txMetaTableTxId txMetaTableWalletId
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this can be shortend to only TxId as every TxId should be unique by itself and WalletId shouldn't be needed to persist uniqness.

Also I don't think we need to have WalletId in this table

Copy link
Contributor

Choose a reason for hiding this comment

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

addressed here #227 (comment)

-- of the transaction are in the TxIn and TxOut tables.
TxMeta
txMetaTableTxId TxId sql=tx_id
txMetaTableWalletId W.WalletId sql=wallet_id
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need reference to Wallet (ie, WalletId) in this table.

conceptually every TxMeta is part of some transaction Tx and every Tx is part of some or many Wallets. If someone needs to query TxMeta, like with he will most likely already have TxId of the transaction that he is working with.

Also conceptually every Tx might be part of multiple wallets: tx will show up in a wallet from which we sent the tx, and it might show up in a wallet to which we sent some tx. In a similar fashion - coresponding TxMeta (as it is part of Tx) might be part of multiple Wallets (sender and receiver wallet), not necessary one Wallet as specified with WalletId foreign key.

I might be on a wrong track but I would rather leave WalletId out for now as at least conceptually it doesn't make sense to have it here.

Because I thought Wallet and Tx are in many-to-many relationship. Every transaction can be in multiple wallets, and every wallet can have multiple transactions

But instead of having WalletId in all these tables related to Tx (like TxMeta, TxOut, TxIn, ...) why not create WalletTx table that will keep all pairs of WalletId and TxId:

WalletTx
  walletId WalletId
  txId TxId

Primary walletId txId

this way we can have this many-to-many relationship in one place without having to manage WalletId in every table that we have.

Maybe I am wrong :/ ? (I am still getting warmed up with sql)

txMetaTableAmount W.Coin sql=amount

Primary txMetaTableTxId txMetaTableWalletId
Foreign Wallet fk_wallet_tx_meta txMetaTableWalletId
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove WalletId from TxMeta - see comment above.

Copy link
Contributor

Choose a reason for hiding this comment

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

-- not the wallet. txInputTableTxId is referred to by TxMeta and PendingTx
TxIn
txInputTableTxId TxId sql=tx_id
txInputTableWalletId W.WalletId sql=wallet_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar answer as I said above - I think we should have WalletId reference here

-- not the wallet. txOutputTableTxId is referred to by TxMeta and PendingTx
TxOut
txOutputTableTxId TxId sql=tx_id
txOutputTableWalletId W.WalletId sql=wallet_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar answer as I said above - I think we should have WalletId reference here

-- not the wallet. txInputTableTxId is referred to by TxMeta and PendingTx
TxIn
txInputTableTxId TxId sql=tx_id
txInputTableWalletId W.WalletId sql=wallet_id
Copy link
Contributor

Choose a reason for hiding this comment

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

also I am not sure I understand why we have two TxIds here ?

@rvl
Copy link
Contributor Author

rvl commented May 10, 2019

@akegalj Hi Ante, the schema will become clearer when we implement the insert/query methods.

  • TxIn and TxOut are exactly the same regardless of which wallet they "belong" to. That's why they have no wallet_id.
  • The inputs and outputs of a transaction must be associated together. That's why the TxIn and TxOut tables have tx_ids.
  • The TxIn database also has a TxId for its source transaction. That's why the TxIn table has two tx_id columns.
  • TxMeta will be different depending on which wallet it is associated with. That's why TxMeta has a wallet_id. Suppose you have a database with two wallets. Funds are sent from one wallet to the other. The TxMeta associated with this transaction should say direction=outgoing for one wallet and direction=incoming for the other wallet.

Thanks for reviewing the substance of this PR rather than just its formatting.

@akegalj
Copy link
Contributor

akegalj commented May 13, 2019

@rvl

TxMeta will be different depending on which wallet it is associated with. That's why TxMeta has a wallet_id. Suppose you have a database with two wallets. Funds are sent from one wallet to the other. The TxMeta associated with this transaction should say direction=outgoing for one wallet and direction=incoming for the other wallet.

ah yes - didn't thought about this case

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.

Mostly looks good to me.

This implementation will sattle when we start implementing queries on the DB and with that collect more experience.

What confuses me currently is last point raised by @jonathanknowles . There are bits in this implementation that look like:

instance Read TxId where
    readsPrec _ = error "readsPrec stub needed for persistent"

and while I was going through persistent and trying to understnad I could find the explanation why and where Read will be used. Seems like this is a constrained from PersistentEntity class https://hackage.haskell.org/package/persistent-2.5/docs/Database-Persist-Class.html#t:PersistEntity .

I would need further to dig the library to understand what for this is used as I can't clearly understand where it is used.

We are going to issue a PR in future to fix this

@paweljakubas
Copy link
Contributor

@jonathanknowles your comments in the review are addressed. As you do not feel ok to check it and we want to have it merged finally I click on Dismiss review - just to proceed further

@paweljakubas paweljakubas dismissed jonathanknowles’s stale review May 13, 2019 10:00

comments addressed and we do not want to wait until next day

@paweljakubas paweljakubas merged commit 102e37d into master May 13, 2019
@KtorZ KtorZ deleted the rvl/147/sql-schema branch May 13, 2019 10:35
@KtorZ KtorZ removed this from the SQLite implementation for the DB Layer milestone Jun 5, 2019
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

5 participants