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

Set up database connection and create tables #247

Merged
merged 19 commits into from
May 17, 2019
Merged

Conversation

rvl
Copy link
Contributor

@rvl rvl commented May 9, 2019

Relates to issue #143 and #154

Overview

  • Added partial newDBLayer function which sets up the db connection and runs migrations.
  • Added a small test case to check that the connection works and tables exist.
  • Add database queries for wallet metadata.

@KtorZ KtorZ assigned rvl May 9, 2019
@paweljakubas paweljakubas force-pushed the rvl/147/sql-schema branch 2 times, most recently from 58834a6 to b424657 Compare May 9, 2019 16:40
@rvl rvl force-pushed the rvl/147/sql-conn branch 2 times, most recently from 7eebd26 to 5dfe82e Compare May 10, 2019 09:14
@paweljakubas paweljakubas force-pushed the rvl/147/sql-schema branch 3 times, most recently from d219095 to f2c65ad Compare May 10, 2019 13:33
@@ -46,6 +46,7 @@ library
, generic-lens
, http-api-data
, http-media
, lens
Copy link
Contributor

@paweljakubas paweljakubas May 10, 2019

Choose a reason for hiding this comment

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

we use generic-lens already in codebase. If I remember well, it was decided to use this lens library. probably it is good idea in the last commit to comply with the rest of the code (if possible)

Copy link
Contributor

Choose a reason for hiding this comment

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

done

, persistent-sqlite
, conduit
, monad-logger
, mtl
Copy link
Contributor

@paweljakubas paweljakubas May 10, 2019

Choose a reason for hiding this comment

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

we use transformers already. probably it is good idea in the last commit to comply with the rest of the code

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@paweljakubas paweljakubas force-pushed the rvl/147/sql-schema branch 2 times, most recently from a1efedb to ca6be91 Compare May 11, 2019 18:00
@paweljakubas paweljakubas force-pushed the rvl/147/sql-conn branch 2 times, most recently from 9522984 to 53739ab Compare May 11, 2019 19:13
@paweljakubas
Copy link
Contributor

@rvl Please notice that the branch was rebased against rvl/147/sql-schema (which was rebased before against master) and forced pushed, so if #227 is merged then you can put it against master and it should compile and be green (made needed fixes and also cleaned imports). I started addressing PrivateKey table and stumbled into problem how to really represent (Key 'RootK XPrv, W.Hash "encryption") . I tried to represent them as (Text, Text) via ByteString but then have problem to return from db to Key 'RootK XPrv

@rvl
Copy link
Contributor Author

rvl commented May 11, 2019

@paweljakubas Thanks 😄

@paweljakubas paweljakubas changed the base branch from rvl/147/sql-schema to master May 13, 2019 10:20
@paweljakubas paweljakubas force-pushed the rvl/147/sql-conn branch 4 times, most recently from 4acfcf9 to aba61e3 Compare May 13, 2019 20:17
@paweljakubas
Copy link
Contributor

@rvl Please read it before you continue working :

  1. rvl/147/sql-conn is rebased against master, compiles and should be green
  2. I ported all tests from MVarSpec adding SQL flavor (especially to wallet state which is represented as (UTxO, PendingTx))
  3. A number of tests does not pass - I denoted them as xit rather than it (at this moment 16 tests do not pass)
  4. I moved shared parts between MVar and SQL to DBSpec - still properties can also be moved to DBspec and shared as they are almost the same - maybe we can do it at the end together with lens refactoring

@paweljakubas paweljakubas self-assigned this May 13, 2019
-- ^ Database file location, or Nothing for in-memory database
-> IO (DBLayer IO s t)
newDBLayer fp = do
conn <- createSqliteBackend fp logStderr
Copy link
Contributor

Choose a reason for hiding this comment

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

wil this logStderr enable all logs (CREATE_TABLE, INSERT, ...) as a default or errors only? I would give my votes for the second option

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, it introduces logs like :

  Wallet table
Migrating: CREATE TABLE "wallet"("wallet_id" VARCHAR NOT NULL,"name" VARCHAR NOT NULL,"passphrase_last_updated_at" TIMESTAMP NULL,"status" INTEGER NOT NULL,"delegation" VARCHAR NULL, PRIMARY KEY ("wallet_id"))
CREATE TABLE "wallet"("wallet_id" VARCHAR NOT NULL,"name" VARCHAR NOT NULL,"passphrase_last_updated_at" TIMESTAMP NULL,"status" INTEGER NOT NULL,"delegation" VARCHAR NULL, PRIMARY KEY ("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.

added more customizable logging function (one can choose LogLevel)

W.lastUpdatedAt <$> meta ^. #passphraseInfo
, walTableStatus = meta ^. #status
, walTableDelegation = delegationToText $ meta ^. #delegation
, walTableAddressScheme = Sequential
Copy link
Contributor

@paweljakubas paweljakubas May 17, 2019

Choose a reason for hiding this comment

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

This probably has to be addressed in another PR. Either removed or detected whether we are Random or Sequential

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there is not yet reading/writing of the wallet state.
However we may even remove this field because the address scheme is fixed by a type parameter of DBLayer.

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly. This is in line what @KtorZ wrote on slack yesterday:

Then, we'll have one

- external seq wallet layer + seq db layer
- fully owned  seq wallet + seq db layer
- fully owned rnd wallet + rnd db layer

Address scheme is set for each db instance. Hence, walTableAddressScheme is probably superflouous

Copy link
Contributor

Choose a reason for hiding this comment

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

removed address scheme

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

@KtorZ
Copy link
Member

KtorZ commented May 17, 2019

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 17, 2019

Canceled

@paweljakubas paweljakubas merged commit e03bcc8 into master May 17, 2019
@iohk-bors iohk-bors bot deleted the rvl/147/sql-conn branch May 17, 2019 10:26
@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

4 participants