Implement Jörmungandr TransactionLayer #439

Anviking · 2019-06-19T12:18:03Z

Issue Number

#219

~~⚠️ base branch is KtorZ/transaction-id-golden-tests ⚠️~~

Overview

I have implemented a preliminary TransactionLayer

Comments

Golden tests are wip (not pushed). ~~I'd call for merging this as soon as possible still.~~

Anviking · 2019-06-19T12:21:00Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Transaction.hs

-newTransactionLayer = TransactionLayer
-    { mkStdTx = error "TODO: See http-bridge as starting point"
+newTransactionLayer
+    :: forall n . Hash "Block0Hash"


@KtorZ

I am not sure about this type ... And, does this really have to be the hash from the first block header?

To my knowledge — yes

https://github.com/input-output-hk/rust-cardano/blob/e5d974f7bedeb00c9c9d688ac66094a34bf8f40d/chain-impl-mockchain/src/transaction/witness.rs#L60-L62

Anviking · 2019-06-19T12:25:24Z

I resurrected this from #418 and merged in the \-> Quantity 0 from #437.

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Transaction.hs

Anviking · 2019-06-19T13:25:08Z

Hm… the diff is messed up since I force-pushed anviking/219/txid :/

paweljakubas · 2019-06-19T13:28:25Z

general comment : the signing looks good at first sight, but I will be persuaded if extensive golden tests will support this feeling :-)

akegalj

It mostly looks good to me. There are few places where I am not sure in the correctness (I wasn't able to find the corresponding place in rust code)

akegalj · 2019-06-21T04:58:15Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

+putMessage :: Message -> Put
+putMessage m = withSizeHeader16be $ case m of
+    Transaction (tx, wits) -> do
+        putWord8 2


we should probably add a comment explaining why constant 2 (or reference to the actuall rust api line containing this info)

akegalj · 2019-06-21T05:06:33Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

@@ -244,37 +258,43 @@ putSignedTransaction (tx, witnesses) = do
 putWitness :: TxWitness -> Put
 putWitness witness =
    case witness of
-        PublicKeyWitness xPub (Hash sig) -> do
+        PublicKeyWitness _xPub (Hash sig) -> do
            -- Witness sum type:


we might want to define sum type that would describe the constants in this module, at least for documentation

akegalj · 2019-06-21T05:08:36Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

            putWord8 1
-            putByteString xPub
+            --putByteString xPub


But then old address scheme won't work, right? (as it expects xPub in its format)

akegalj · 2019-06-21T05:11:38Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

    mapM_ putInput inputs
    mapM_ putOutput outputs
  where
    putInput (TxIn inputId inputIx) = do
        -- NOTE: special value 0xff indicates account spending
        -- only old utxo/address scheme supported for now
        putWord8 $ fromIntegral inputIx
+        putWord64be 14 -- TODO: We need input value!


I am not sure I understand the idea behind this ?

EDIT: aha - I see https://github.com/input-output-hk/rust-cardano/blob/13af7aa08cdac8efc9dc8b0f7ff9162d28b2d479/chain-impl-mockchain/src/transaction/transfer.rs#L21 👍

akegalj · 2019-06-21T05:27:05Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Compatibility.hs

@@ -84,7 +79,7 @@ genesis = BlockHeader
 -- The corresponding rust implementation is:
 -- https://github.com/input-output-hk/rust-cardano/blob/e5d974f7bedeb00c9c9d688ac66094a34bf8f40d/chain-impl-mockchain/src/transaction/transaction.rs#L115-L119
 instance TxId (Jormungandr n) where
-    txId = blake2b256 . putTokenTransfer
+    txId = blake2b256 . putTxBody


👍 https://github.com/input-output-hk/rust-cardano/blob/e5d974f7bedeb00c9c9d688ac66094a34bf8f40d/chain-impl-mockchain/src/transaction/transaction.rs#L117

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

akegalj · 2019-06-21T05:40:44Z

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/TransactionSpec.hs

+            let keyFrom _ = Just (key, pwd)
+            let inps = pure
+                    ( txin
+                    , TxOut (Address "used for lookup") (Coin 14)


address content is not important?

KtorZ · 2019-06-22T00:22:12Z

This PR is to merge after:

Parameterize Tx data-type over the wallet core engine (allowing to work with different representations) #451
Review transaction ids golden tests (only new addresses) #452
Added golden tests for transactions signing using jcli (only new address format for now though..)
Removed commented-code or unused code
Did some refactoring, replaced custom functions with built-in, removed unneeded indirections and renamed some functions / top-level expressions
Fixed bug in the transaction signer for Jormungandr, it used to use the transaction id as a payload, whereas it should be using the id from each transaction input instead!

cc @paweljakubas @Anviking

Co-authored-by: KtorZ <matthias.benkort@gmail.com>

This makes it more flexible to manipulate 'TxWitness' across different backend target. The application layer only regards it as a bunch of bytes anyway. Co-authored-by: KtorZ <matthias.benkort@gmail.com>

…ection

…ernal file)

KtorZ self-requested a review June 19, 2019 12:19

Anviking commented Jun 19, 2019

View reviewed changes

Anviking requested a review from paweljakubas June 19, 2019 12:25

Anviking force-pushed the anviking/219/signing branch from 8762a3d to 586db8d Compare June 19, 2019 12:26

Anviking force-pushed the anviking/219/txid branch from a936dc8 to b76bbac Compare June 19, 2019 12:34

Anviking force-pushed the anviking/219/signing branch from 6a6a7ec to 85ae50e Compare June 19, 2019 12:40

Anviking force-pushed the anviking/219/txid branch from b76bbac to 24b1e35 Compare June 19, 2019 13:06

paweljakubas reviewed Jun 19, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Transaction.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Jun 19, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Transaction.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/219/signing branch 2 times, most recently from 86f81c4 to 1a85616 Compare June 19, 2019 13:27

paweljakubas approved these changes Jun 19, 2019

View reviewed changes

Anviking self-assigned this Jun 19, 2019

Anviking force-pushed the anviking/219/txid branch from 24b1e35 to 64c9000 Compare June 20, 2019 10:39

Anviking force-pushed the anviking/219/signing branch from 2714a7a to 3f42a9a Compare June 20, 2019 15:37

KtorZ force-pushed the anviking/219/signing branch from 0478c90 to 095a434 Compare June 20, 2019 16:56

Anviking force-pushed the anviking/219/signing branch from 095a434 to dba548c Compare June 21, 2019 03:45

Anviking changed the base branch from anviking/219/txid to master June 21, 2019 03:45

akegalj reviewed Jun 21, 2019

View reviewed changes

Anviking force-pushed the anviking/219/signing branch from dba548c to eb58f1f Compare June 21, 2019 06:55

akegalj force-pushed the anviking/219/signing branch from eb58f1f to 0388931 Compare June 21, 2019 14:05

KtorZ changed the base branch from master to KtorZ/transaction-id-golden-tests June 21, 2019 19:51

KtorZ force-pushed the anviking/219/signing branch from 0388931 to bd4119e Compare June 21, 2019 19:53

KtorZ self-assigned this Jun 21, 2019

KtorZ force-pushed the KtorZ/transaction-id-golden-tests branch 2 times, most recently from 55cfa17 to 8f5b202 Compare June 22, 2019 00:17

KtorZ force-pushed the anviking/219/signing branch from bd4119e to ddf0286 Compare June 22, 2019 00:18

KtorZ force-pushed the KtorZ/transaction-id-golden-tests branch from 8f5b202 to 43f1aca Compare June 22, 2019 11:41

KtorZ force-pushed the anviking/219/signing branch from ddf0286 to a05841d Compare June 22, 2019 11:42

KtorZ changed the base branch from KtorZ/transaction-id-golden-tests to master June 22, 2019 12:35

Anviking and others added 7 commits June 22, 2019 14:36

Implement jörmungandr mkStdTx (first draft)

eb73054

minor style adjustment, mostly removing extra newlines

9933ac2

Fix build errors

adf526f

Co-authored-by: KtorZ <matthias.benkort@gmail.com>

Make 'TxWitness' a mere wrapper around a 'ByteString'

b062084

This makes it more flexible to manipulate 'TxWitness' across different backend target. The application layer only regards it as a bunch of bytes anyway. Co-authored-by: KtorZ <matthias.benkort@gmail.com>

replace 'whileM' with 'many' and remove unecessary 'FromBinary' indir…

f5ee198

…ection

review goldent tests for comparing tx with jcli

1cbbc4d

fix witness creation: use txId from each input!

4d233da

KtorZ force-pushed the anviking/219/signing branch from a05841d to 0e8f1f0 Compare June 22, 2019 12:37

KtorZ added 4 commits June 22, 2019 16:09

review Jormungandr BinarySpec module (favor hex encoding + remove ext…

d61d34b

…ernal file)

cleanup dead-code even further in Jormungandr Binary

1ae8142

add decode test for Jormungandr genesis block on Mainnet

3ada3a8

re-generate nix machinery

6c2f5d1

KtorZ force-pushed the anviking/219/signing branch from 0e8f1f0 to 6c2f5d1 Compare June 22, 2019 14:10

KtorZ merged commit de43053 into master Jun 22, 2019

KtorZ deleted the anviking/219/signing branch June 22, 2019 15:01

KtorZ mentioned this pull request Jul 3, 2019

Jörmungandr REST-API / Network Layer #219

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Jörmungandr TransactionLayer #439

Implement Jörmungandr TransactionLayer #439

Anviking commented Jun 19, 2019 •

edited by KtorZ

Loading

Anviking Jun 19, 2019

Anviking Jun 19, 2019

Anviking commented Jun 19, 2019

Anviking commented Jun 19, 2019

paweljakubas commented Jun 19, 2019 •

edited

Loading

akegalj left a comment

akegalj Jun 21, 2019

akegalj Jun 21, 2019

akegalj Jun 21, 2019

akegalj Jun 21, 2019

akegalj Jun 21, 2019

akegalj Jun 21, 2019

KtorZ commented Jun 22, 2019

Implement Jörmungandr TransactionLayer #439

Implement Jörmungandr TransactionLayer #439

Conversation

Anviking commented Jun 19, 2019 • edited by KtorZ Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Jun 19, 2019

Anviking commented Jun 19, 2019

paweljakubas commented Jun 19, 2019 • edited Loading

akegalj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Jun 22, 2019

Anviking commented Jun 19, 2019 •

edited by KtorZ

Loading

paweljakubas commented Jun 19, 2019 •

edited

Loading