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

Implement jörmungandr txId #436

Merged
merged 1 commit into from
Jun 20, 2019
Merged

Implement jörmungandr txId #436

merged 1 commit into from
Jun 20, 2019

Conversation

Anviking
Copy link
Collaborator

@Anviking Anviking commented Jun 19, 2019 •
edited
Loading

Issue Number

#219

Overview

  • I have implemented txId = blake2b256 . putTokenTransfer
  • A golden test
  • Auxiliary: I have renamed putTransaction to putSignedTransaction

Comments

  • I have not verified that the golden matches jcli

@Anviking Anviking self-assigned this Jun 19, 2019
Anviking
Anviking commented Jun 19, 2019
View reviewed changes
lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/CompatibilitySpec.hs
largeTx :: Tx
largeTx = Tx {inputs = [], outputs = [TxOut {address = Address {unAddress = "\131\&3$\195xi\193\"h\154\&5\145}\245:O\"\148\163\165/h^\ENQ\245\248\229;\135\231\234E/"}, coin = Coin {getCoin = 14}},TxOut {address = Address {unAddress = "\131\ENQK\186?\203{_$\145\134\ESCn+\139\240\163\249%|\223/\223A\202Z\247\a.w\199\SI:"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\147\147\196i\217\199\156^\"~z\242\228\"\136*\142.\188!l\221o\158T\224`\242\&1[\196\DC4"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\206\155\217\224f}\246\230\174y^\139\ACK\177\154\152=!\157\184\192&\v\175\204\239\215\RS\204\254\195\219"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\STX\NUL^\t\129\243\223Q={0\155\193\&0\250_Z|\167\244I\ETX\166]\189\165\188\147u\131\218="}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\140\164,\SI\STX\170\195\SOH\145\ENQ\206\164\172\186n\194G\228\133\147\251\ENQX3%(\230 ~\209\188%"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\175\217\146d[\t\133\238>\174s[\168W\193\177\141W\173\246B\255\177r\145\137\191[Q\230\176\187"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\FS|T#\209X\a\236>\143S\DC1\169Ok/\181y\176g\182\212\DEL\167p\v\ETX\SOH\195c\t\160"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131#c\211\176a\143T\204 \168\&3y\193fGM\209\DC2\FSgM\250\131I~1\236\197\SUB\148V\209"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131\252\229\169j*u.n\159\184\CAN\214\203C2><\a&\191\140\202\150M\176\255\175\DLEFxZ!"}, coin = Coin {getCoin = 100000000000}},TxOut {address = Address {unAddress = "\131=K\138f\DC3\188x\154\144\173\DEL\241T\166\161\141Y\195\176\156\134\237;\146\173\232\250\&2\215\ACK@\254"}, coin = Coin {getCoin = 100000000000}}]}

addrSpec :: Spec
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To preserve horizontal space and keep the txId related additions in one place.

Copy link
Member

Choose a reason for hiding this comment

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

👍 , actually for consistency, I'd have also go for txSpec :)

KtorZ
KtorZ reviewed Jun 19, 2019
View reviewed changes
lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/CompatibilitySpec.hs
spec = do
addrSpec
describe "txId @(Jormungandr n)" $ do
it "(txId largeTx) should match golden" $ do
Copy link
Member

Choose a reason for hiding this comment

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

Nice!
Would be nice to have some more actually covering various cases:

  • Transaction with a single input / output
  • Old and new addresses

What do you think ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not doing old addresses now as per thread:

https://input-output-rnd.slack.com/archives/GBT05825V/p1560938440087200?thread_ts=1560934314.081600&cid=GBT05825V

@Anviking Anviking force-pushed the anviking/219/txid branch 6 times, most recently from b76bbac to 24b1e35 Compare June 19, 2019 13:06
@Anviking Anviking requested a review from KtorZ June 19, 2019 13:06
rvl
rvl approved these changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Looks good.

I was checking out putTokenTransfer (not in this PR) and wondering how we are limiting transaction size before serialisation? Because the maximum number of ins/outs are 255 -- large number, but still a limit.

@Anviking Anviking merged commit 85a8316 into master Jun 20, 2019
@Anviking
Copy link
Collaborator Author

@rvl thanks, good point 😅 I should have a look at that. I suspect all the uses of fromIntegral in the module are potentially very dangerous and that it in this case would overflow.

@KtorZ KtorZ deleted the anviking/219/txid branch June 21, 2019 18:55
@KtorZ KtorZ mentioned this pull request Jul 3, 2019
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl approved these changes

@KtorZ KtorZ Awaiting requested review from KtorZ

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Anviking @rvl @KtorZ