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 TxMeta type #114

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Add TxMeta type #114

merged 1 commit into from
Mar 26, 2019

Conversation

akegalj
Copy link
Contributor

@akegalj akegalj commented Mar 22, 2019 •
edited
Loading

Issue Number

#101

Overview

  • I have added initial version of TxMeta datatype

Comments

Nothing has been wired yet. This just creates datatype that will be used later on. Datatype is not set in stone as well and might change

akegalj
akegalj commented Mar 22, 2019
View reviewed changes
src/Cardano/Wallet/Primitive/Types.hs
{ metaId :: !(Hash "Tx")
-- FIXME: we might want to go for something more optimized than natural,
-- like Word64 ?
, depth :: !(Quantity "block" Natural)
Copy link
Contributor Author

@akegalj akegalj Mar 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

do we want to keep types in Primitives in their optimized form , thus opting for a bit more performant type like Word64?

Copy link
Member

Choose a reason for hiding this comment

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

Optimizations between Natural vs Word64 is pointless at this stage. Writing an algorithm that goes in O(n²) will cause us some trouble. Using Natural instead of Word64 won't probably have any noticeable effects on a wallet.

If so, we should measure it first and have a baseline to compare with.

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 will leave it to Natural then because its a safer type. If there will be some evidence that this should be optimized more we can then measure and optimize

src/Cardano/Wallet/Primitive/Types.hs Outdated
-- part alltogether. Node/core bits would still work optimally with
-- Coin/Word64
-- but wallet bits could go a bit more abstract (and less optimized)?
, amount :: !Coin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about something less performant like Quantity "lovelace" Natural for representing coin in wallet?

EDIT: in fact ^ is probably a bad idea because exchanges are also our clients and we do want to have performant types in wallet

Copy link
Member

Choose a reason for hiding this comment

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

I guess the amount is redundant with the transaction itself here. Not sure we do need to duplicate this information in the metadata.

Copy link
Contributor Author

@akegalj akegalj Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yes you are right - we can calculate this on demand. I thought this type will be used as a return type after calculation has been done already:

computeTxMeta :: Tx -> ... -> TxMeta

so in computeTxMeta we would calculate this amount. As I am not sure how this type will be used yet through the code - I can remove it untill its needed

src/Cardano/Wallet/Primitive/Types.hs
newtype Timestamp = Timestamp
-- NOTE: We could have gone for Microsecond from Data.Time.Units, but
-- it seems like an overkill for now
{ getTimestamp :: UTCTime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the old wallet we used Microsecond which at this point seems like not needed.

src/Cardano/Wallet/Primitive/Types.hs
@@ -1,6 +1,7 @@
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DerivingStrategies #-}
{-# LANGUAGE DuplicateRecordFields #-}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as proposed by @KtorZ , we are going to try to use generic-lens and light lensing as described here https://hackage.haskell.org/package/generic-lens with as little as possible:

  • ^.
  • .~
  • %~

This is experimental

@akegalj akegalj mentioned this pull request Mar 22, 2019
Closed
@akegalj akegalj marked this pull request as ready for review March 22, 2019 18:16
@akegalj akegalj requested review from KtorZ and paweljakubas March 22, 2019 18:16
@KtorZ KtorZ force-pushed the master branch 2 times, most recently from 049fddd to a8d7c7d Compare March 23, 2019 12:45
KtorZ
KtorZ approved these changes Mar 25, 2019
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.

Some minor changes but over looks good 👍

src/Cardano/Wallet/Primitive/Types.hs
{ metaId :: !(Hash "Tx")
-- FIXME: we might want to go for something more optimized than natural,
-- like Word64 ?
, depth :: !(Quantity "block" Natural)
Copy link
Member

Choose a reason for hiding this comment

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

Optimizations between Natural vs Word64 is pointless at this stage. Writing an algorithm that goes in O(n²) will cause us some trouble. Using Natural instead of Word64 won't probably have any noticeable effects on a wallet.

If so, we should measure it first and have a baseline to compare with.

src/Cardano/Wallet/Primitive/Types.hs Outdated
@@ -212,6 +220,30 @@ instance Buildable TxOut where
instance Buildable (TxIn, TxOut) where
build (txin, txout) = build txin <> " ==> " <> build txout

data TxMeta = TxMeta
{ metaId :: !(Hash "Tx")
Copy link
Member

Choose a reason for hiding this comment

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

What about txId here? That's what it is in the end, right ?

src/Cardano/Wallet/Primitive/Types.hs Outdated
-- part alltogether. Node/core bits would still work optimally with
-- Coin/Word64
-- but wallet bits could go a bit more abstract (and less optimized)?
, amount :: !Coin
Copy link
Member

Choose a reason for hiding this comment

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

I guess the amount is redundant with the transaction itself here. Not sure we do need to duplicate this information in the metadata.

@paweljakubas paweljakubas force-pushed the akegalj/101/tx-meta-type branch from b68f2e4 to bd6b33c Compare March 26, 2019 08:58
@akegalj @paweljakubas
Add TxMeta type
bd6b33c 
Remove comments

[101] Final polish of TxMeta type
paweljakubas
paweljakubas approved these changes Mar 26, 2019
View reviewed changes
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 KtorZ merged commit 6ccd314 into master Mar 26, 2019
@KtorZ KtorZ deleted the akegalj/101/tx-meta-type branch March 26, 2019 11:27
@paweljakubas paweljakubas mentioned this pull request Apr 2, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paweljakubas paweljakubas paweljakubas approved these changes

@KtorZ KtorZ KtorZ approved these changes

Assignees

@akegalj akegalj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akegalj @KtorZ @paweljakubas