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

SQL Schema #227

Merged
merged 12 commits into from
May 13, 2019
Merged

SQL Schema #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
24e5426
Add flatSlot/fromFlatSlot to Cardano.Wallet.Primitive.Types
rvl May 8, 2019
522b470
Add Data.Text.Class.fromTextMaybe
rvl May 8, 2019
0e6d475
Add TxStatus support and improve on schema
paweljakubas May 8, 2019
8556f53
First refactoring of review comments
paweljakubas May 7, 2019
a7c47ad
Initial persistent SQL schema
rvl May 9, 2019
82ee336
Add a test for flatSlot/fromFlatSlot
rvl May 9, 2019
6df5cd4
Add another test for fromText/fromTextMaybe
rvl May 9, 2019
a161b14
Tidy up some of the formatting within Cardano.Wallet.DB.Sqlite to meet
jonathanknowles May 9, 2019
69ffbbd
Wrap code in Cardano.Wallet.DB.SqliteSpec to meet our coding standard.
jonathanknowles May 9, 2019
85fca9d
Extend AddressSchema
paweljakubas May 9, 2019
25781fb
Fix AddressScheme PersistField instance
rvl May 10, 2019
a3ec3a6
Last estetic bits
paweljakubas May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/core/cardano-wallet-core.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,30 @@ library
, http-api-data
, http-media
, memory
, path-pieces
, persistent
, persistent-sqlite
, persistent-template
, servant
, servant-server
, text
, text-class
, time
, transformers
, vector

hs-source-dirs:
src
exposed-modules:
Cardano.Environment
Cardano.Wallet
Cardano.Wallet.Api
Cardano.Wallet.Api.Server
Cardano.Wallet.Api.Types
Cardano.Wallet.DB
Cardano.Wallet.DB.MVar
Cardano.Environment
Cardano.Wallet.DB.SqliteTypes
Cardano.Wallet.DB.Sqlite
Cardano.Wallet.Network
Cardano.Wallet.Primitive.AddressDerivation
Cardano.Wallet.Primitive.AddressDiscovery
Expand Down Expand Up @@ -120,6 +127,11 @@ test-suite unit
, text-class
, transformers
, yaml
, persistent
, persistent-sqlite
, conduit
, monad-logger
, mtl
type:
exitcode-stdio-1.0
hs-source-dirs:
Expand All @@ -130,6 +142,7 @@ test-suite unit
Cardano.Wallet.Api.TypesSpec
Cardano.Wallet.ApiSpec
Cardano.Wallet.DB.MVarSpec
Cardano.Wallet.DB.SqliteSpec
Cardano.Wallet.DBSpec
Cardano.EnvironmentSpec
Cardano.Wallet.NetworkSpec
Expand Down
153 changes: 153 additions & 0 deletions lib/core/src/Cardano/Wallet/DB/Sqlite.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE QuasiQuotes #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE TypeApplications #-}
{-# LANGUAGE TypeFamilies #-}

module Cardano.Wallet.DB.Sqlite where

import Prelude

import Cardano.Wallet.DB.SqliteTypes
( AddressScheme, TxId )
import Data.Text
( Text )
import Data.Time.Clock
( UTCTime )
import Data.Word
( Word32 )
import Database.Persist.TH
Copy link
Member

Choose a reason for hiding this comment

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

This is imported implicitly, let's not.

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 remark as above -> done

( mkMigrate
, mkPersist
, mpsPrefixFields
, persistLowerCase
, share
, sqlSettings
)
import GHC.Generics
( Generic (..) )

import qualified Cardano.Wallet.Primitive.Types as W

-- fixme: need tables for wallet AddressPool

share
[ mkPersist sqlSettings { mpsPrefixFields = False }
, mkMigrate "migrateAll"
]
[persistLowerCase|

-- Wallet IDs, address discovery state, and metadata.
Wallet
walTableId W.WalletId sql=wallet_id
walTableName Text sql=name
walTablePassphraseLastUpdatedAt UTCTime sql=passphrase_last_updated_at
walTableStatus W.WalletState sql=status
walTableDelegation Text Maybe sql=delegation
walTableAddressScheme AddressScheme sql=address_discovery

Primary walTableId
deriving Show Generic

Copy link
Member

Choose a reason for hiding this comment

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

I have a question about the naming here.

Is there a good reason to use the word Table in every field name?

Why not just make sure that each field name starts with the name of the table as a prefix, but in lower case.

So:

Foo
    fooX
    fooY

For example:

Wallet
    walletId                 W.WalletId     sql=wallet_id
    walletName               Text           sql=name
    walletPassLastUpdatedAt  UTCTime        sql=passphrase_last_updated_at
    walletStatus             W.WalletState  sql=status
    walletDelegation         Text Maybe     sql=delegation
    walletAddressScheme      AddressScheme  sql=address_discovery
TxInput
    txInputTxId         TxId        sql=tx_id
    txInputWalletId     W.WalletId  sql=wallet_id
    txInputSourceTxId   TxId        sql=source_id
    txInputSourceIndex  Word32      sql=source_index

Copy link
Contributor

Choose a reason for hiding this comment

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

I like putting Table especially in the context of writing encoders/decoders. Then, it is immediately visible to whether field belongs to the generated type from template haskell or to other modules. That's the rationale behind introducing it. Nevertheless, I am not about to fight for this ;-)

-- The private key for each wallet. This is in a separate table simply so that
-- "SELECT * FROM wallet" won't print keys.
PrivateKey sql=private_key
privateKeyTableWalletId W.WalletId sql=wallet_id
privateKeyTableKey Text sql=private_key

Primary privateKeyTableWalletId
Foreign Wallet fk_wallet_private_key privateKeyTableWalletId

deriving Show Generic

-- Maps a transaction ID to its metadata (which is calculated when applying
-- blocks).
--
-- TxMeta is specific to a wallet because multiple wallets may have the same
-- transaction with different metadata values. The associated inputs and outputs
-- 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 wonder if we really needs here walletId

Copy link
Contributor

Choose a reason for hiding this comment

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

probably yes - if we remove wallet then all pertinent TxMeta should also be removed

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - my comment above is wrong in many ways. I didn't took into account that TxMeta will be different depending on the Wallet it bellongs to (as @rvl pointed out)

txMetaTableStatus W.TxStatus sql=status
txMetaTableDirection W.Direction sql=direction
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)

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.

#227 (comment)

deriving Show Generic

-- A transaction input associated with TxMeta.
--
-- There is no wallet ID because these values depend only on the transaction,
-- 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.

if above answer to above question is no then this is not needed

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rvl addressed these questions #227 (comment)

txInputTableSourceTxId TxId sql=source_id
txInputTableSourceIndex Word32 sql=source_index

Primary txInputTableTxId txInputTableSourceTxId txInputTableSourceIndex
Foreign TxMeta fk_tx_meta_tx_in txInputTableTxId txInputTableWalletId
deriving Show Generic

-- A transaction output associated with TxMeta.
--
-- There is no wallet ID because these values depend only on the transaction,
-- 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.

if above answer to above question is no then this is not needed

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

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)

txOutputTableIndex Word32 sql=index
txOutputTableAddress Text sql=address
txOutputTableAmount W.Coin sql=amount

Primary txOutputTableTxId txOutputTableIndex
Foreign TxMeta fk_tx_meta_tx_out txOutputTableTxId txOutputTableWalletId
deriving Show Generic

-- A checkpoint for a given wallet is referred to by (wallet_id, slot_id).
-- Checkpoint data such as UTxO will refer to this table.
Checkpoint
checkpointTableWalletId W.WalletId sql=wallet_id
checkpointTableWalletSlot W.SlotId sql=slot_id

Primary checkpointTableWalletId checkpointTableWalletSlot
Foreign Wallet fk_wallet_checkpoint checkpointTableWalletId

deriving Show Generic

-- The UTxO for a given wallet checkpoint is a one-to-one mapping from TxIn ->
-- TxOut. This table does not need to refer to the TxIn or TxOut tables. All
-- necessary information for the UTxO is in this table.
UTxO sql=utxo

-- The wallet checkpoint (wallet_id, slot_id)
utxoTableWalletId W.WalletId sql=wallet_id
utxoTableWalletSlot W.SlotId sql=slot_id

-- TxIn
utxoTableInputId TxId sql=input_tx_id
utxoTableInputIndex Word32 sql=input_index

-- TxOut
utxoTableOutputId TxId sql=output_tx_id
utxoTableOutputIndex Word32 sql=output_index

Primary
utxoTableWalletId
utxoTableWalletSlot
utxoTableInputId
utxoTableInputIndex
utxoTableOutputId
utxoTableOutputIndex

Foreign Checkpoint fk_checkpoint_utxo utxoTableWalletId utxoTableWalletSlot
deriving Show Generic
Copy link
Member

Choose a reason for hiding this comment

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

What's the need for this since we already have a transaction table?

Copy link
Contributor

Choose a reason for hiding this comment

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

removed PendingTx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's needed to associate pending transactions with a checkpoint. But we'll find out later when implementing putCheckpoint and readCheckpoint.

|]
rvl marked this conversation as resolved.
Show resolved Hide resolved
Loading