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

Add tx metadata to sqlite database #2091

Merged
merged 5 commits into from
Sep 2, 2020
Merged

Add tx metadata to sqlite database #2091

merged 5 commits into from
Sep 2, 2020

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Aug 28, 2020

Issue Number

ADP-307 / #2072

Overview

Includes transaction metadata in the sqlite database transaction history.

Comments

@rvl rvl added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Aug 28, 2020
@rvl rvl added this to the (ADP-307) Transaction metadata milestone Aug 28, 2020
@rvl rvl self-assigned this Aug 28, 2020
@rvl rvl added this to In Progress in Adrestia Aug 28, 2020
@rvl rvl force-pushed the rvl/2072/sqlite-tx-metadata branch from ba83ded to 84fefa6 Compare August 31, 2020 15:02
@rvl rvl force-pushed the rvl/2072/sqlite-tx-metadata branch from 84fefa6 to d416430 Compare September 1, 2020 02:36
Base automatically changed from rvl/2077/tx-metadata to master September 1, 2020 04:12
@rvl rvl force-pushed the rvl/2072/sqlite-tx-metadata branch from d416430 to 8c98ca5 Compare September 1, 2020 05:37
KtorZ
KtorZ reviewed Sep 1, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated
@@ -128,13 +129,13 @@ import Data.Either
import Data.Generics.Internal.VL.Lens
( (^.) )
import Data.List
( nub, sortOn, unzip3 )
( nub, sortOn, unzip4 )
Copy link
Member

Choose a reason for hiding this comment

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

I like how it started with just unzip2 and gradually increased. We still have 3 left, after which we have to stop further development :)

lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated
dbChunked repsertMany
[ (TxInKey txInputTxId txInputSourceTxId txInputSourceIndex, i)
| i@TxIn{..} <- txins ]
dbChunked repsertMany
[ (TxOutKey txOutputTxId txOutputIndex, o)
| o@TxOut{..} <- txouts ]
forM_ (chunksOf chunkSize $ map txMetaTxId metas) $ \txids ->
deleteWhere [ TxMetadataTxId <-. txids ]
Copy link
Member

Choose a reason for hiding this comment

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

Why the delete? Isn't the point of repsert to "replace or insert"?

lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated
, concatMap (fst . (\(_,b,_,_) -> b)) entities
, concatMap (snd . (\(_,b,_,_) -> b)) entities
, concatMap (\(_,_,c,_) -> maybeToList c) entities
, concatMap (\(_,_,_,d) -> d) entities
)
Copy link
Member

@KtorZ KtorZ Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't be more efficient and simpler to store metadata with the rest of the static data (status, direction etc..)?

We have to resort to separate database tables for TxIn, TxOut and TxWithdrawals because they are unbounded (modulo tx size) lists. Metadata however can be treated as a bunch of bytes that are either present or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reason was just that sqlite tables don't cost much and it seemed like it would be confusing to have data in the TxMeta table which was actually a field of Tx.

lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs Outdated
txMetadataValue W.TxMetadata sql=value

Primary txMetadataTxId
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 am a bit doubtful about having this as a separate table (cf earlier comment).

@KtorZ KtorZ force-pushed the rvl/2072/sqlite-tx-metadata branch from 8c98ca5 to c43d093 Compare September 1, 2020 12:23
@KtorZ
store transaction metadata alongside other derived informations
4815f44 
  This is more efficient than storing them in separate tables like inputs and outputs and also a bit less awkward when it comes to fetching metadata corresponding to each transactions back.
@KtorZ
temporarily work-around jsonToMetadata limitations in state-machine t…
58d7913 
…ests
KtorZ
KtorZ approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Temporarily worked-around the roundtripping issues until #2098 is fixed. state-machine tests pass for now.

@rvl
Copy link
Contributor Author

rvl commented Sep 2, 2020

Thanks

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 2, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit ef704d7 into master Sep 2, 2020
@iohk-bors iohk-bors bot deleted the rvl/2072/sqlite-tx-metadata branch September 2, 2020 01:23
@piotr-iohk piotr-iohk moved this from In Progress to Closed in Adrestia Oct 6, 2020
@CharlesMorgan-IOHK CharlesMorgan-IOHK removed this from Closed in Adrestia Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ approved these changes

Assignees

@rvl rvl

Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Milestone
(ADP-307) Transaction metadata
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rvl @KtorZ