Add servant post transaction endpoint #111

akegalj · 2019-03-22T10:40:03Z

Issue Number

Overview

I have added post transaction endpoint to servant api
I have added json golden files to cover new api
I have adjusted servant api with swagger spec

Comments

Real diff of this PR is ~250L , rest is mostly golden files produced by our test spec so don't be intemidated. Golden files might be too big: #111 (comment) ?

src/Cardano/Wallet/Api.hs

KtorZ · 2019-03-22T10:54:51Z

src/Cardano/Wallet/Primitive.hs

@@ -221,6 +223,15 @@ newtype Address = Address

 instance NFData Address

+data AddressError
+    = AddressDecodeError T.Text


What about a record with meaningful field to give better meaning to the underlying Text (also, the qualified import isn't necessary here).

Removed - this type was moved to different module (by other author) https://github.com/input-output-hk/cardano-wallet/blob/5c7d690e5d994c931e5b437c5bdbaf222f6dc675/src/Cardano/Wallet/Api/Types.hs#L205 .

It is not a record but it also doesn't seem to be used as a sum type in future. Rather it looks like a self containing wrapper type where constructor/type name tells the storry (at least for now). So I think we can forget about this until we run into more complicated use case

akegalj · 2019-03-29T08:34:26Z

src/Cardano/Wallet/Api/Types.hs

@@ -147,15 +149,10 @@ data WalletPutPassphraseData = WalletPutPassphraseData
    } deriving (Eq, Generic, Show)

 data CreateTransactionData = CreateTransactionData
-    { _targets :: ![TargetOutput]
+    { _targets :: ![TxOut]


@KtorZ what do you think about reusing tx output here and a bit bellow how aeson instances are handled

I have abandoned this idea as it didn't quite suite my needs - I had to wrap fields of TxOut with ApiT and it was easier to opt for a separate type.

ps - If number of such types increases, we could use template haskell to generate such types, types - but number of types is low so there is no need for such heavy machinery at this point (and I am not sure what is our opinion about template haskell in general).

For example if we had something like:

deriveApiTFor TxOut

something like this would create type

data TxOut = TxOut { address :: ApiT Address , coin :: ApiT Coin }

As personally I run only to one such use case so far I didn't want to introduce anything like the solution above - but if there were many such examples we might consider it

src/Cardano/Wallet/Api/Types.hs

src/Cardano/Wallet/Primitive/Types.hs

akegalj · 2019-04-08T16:48:03Z

specifications/api/swagger.yaml

-  maxLength: 64
-  minLength: 64
+  maxLength: 128
+  minLength: 128


According to the comment https://github.com/input-output-hk/cardano-wallet/blob/5c7d690e5d994c931e5b437c5bdbaf222f6dc675/src/Cardano/Wallet/Binary.hs#L569 , Hash "Tx" is a 64 byte string, which translates to 128 long hex encoded string (because byte is encoded as 2 char string).

Hmmm no. The hex-encoded version is 64-byte, the hash itself is 32 bytes. The spec was right here (cf current transaction ids..)

ups - ok. Will fix

specifications/api/swagger.yaml

akegalj · 2019-04-08T17:02:58Z

src/Cardano/Wallet/Api/Types.hs

+    , depth :: !(Quantity "block" Natural)
+    , direction :: !(ApiT Direction)
+    , inputs :: !(NonEmpty ApiCoinSelection)
+    , outputs :: !(NonEmpty ApiCoinSelection)


inputs and outputs could be sepparate datatypes here such as TxInputs and TxOutputs but I decided to merge datatype into one named ApiCoinSelection. I am not sure is this a good idea or not. They do represent the same idea but I opt for the simplicity here and dropped the extra type safety that we were getting from having two types at this level

Alternitavely we would create two types ApiTxInputs and ApiTxOutputs which would look similar to TxIn and TxOut from Primitive module (except they will have wrappers for ApiT). Simiarly we would use ApiTxOut in PostTransactionData.targets.

Also ApiCoinSelection might be unfortunate name pick as it conflicts with coin selection algorithm that we implemented https://github.com/input-output-hk/cardano-wallet/tree/5c7d690e5d994c931e5b437c5bdbaf222f6dc675/src/Cardano/Wallet/CoinSelection although the context is the same

Agreed that keeping types identical here makes sense (from the API perspective, they are the same thing). About the name clash / confusion, I would go for ApiCoins or ApiResolvedInputs to makes it slightly less ambiguous though, it's all more-or-less the same thing.

Something other than ApiCoinSelection 👍

If possible, a short doc-comment about what it is would be awesome too. E.g if you were to go with ApiResolvedInputs, it might be worth knowing that it's used for outputs, and not just inputs of ApiTransaction

ApiCoins sounds reasonable to me. Or with some tweak like ApiOwnedCoins to indicate that they contain addresses, which I presume are the previous and current owner.

doc-comment

@Anviking I guess you meant to document this in code because this object name doesn't appear in our swagger produced UI

akegalj · 2019-04-08T17:03:11Z

src/Cardano/Wallet/Api/Types.hs

@@ -159,6 +177,33 @@ data WalletPutPassphraseData = WalletPutPassphraseData
    , newPassphrase :: !(ApiT (Passphrase "encryption"))
    } deriving (Eq, Generic, Show)

+data PostTransactionData = PostTransactionData
+    { targets :: !(NonEmpty ApiCoinSelection)


see https://github.com/input-output-hk/cardano-wallet/pull/111/files#r273147460

src/Cardano/Wallet/Primitive/Types.hs

akegalj · 2019-04-08T17:08:26Z

test/data/Cardano/Wallet/Api/CreateTransactionData.json

+        },
+        {
+            "passphrase": "7ehYx\\==l5𡄩\"RlwoO6R|k`1~;tVP)&𩺄2?!!?ujj{w]meZ]Ti5{TAu(M;%}_E휊Wao<Ji4AC<~w-&+gHH[fBVj⅝jp↯$e𧶥콁P:\".YOmVAQ7t]PpxEoc5;;/ypb<}e R@@F'H=2Y$ZUH[g𦲧XRH (L&!fCRTV/&wPaqWn54D!5gVⷝ-oshW_l)S/XN贂0<YWjjNM\\fu9",
+            "targets": [


arbitrary generates rather big example here - is this ok or should we aim for a smaller golden file (with only one item in lists/arrays which would probably be ok to validate)?

Na it's okay. We aren't "quickchecking" that but actually, we do want to cover the maximum surface with the golden tests. Bigger is better here.

akegalj · 2019-04-08T17:09:13Z

test/data/Cardano/Wallet/Api/ApiTransaction.json

+                "quantity": 37,
+                "unit": "lovelace"
+            },
+            "inputs": [


see comment here https://github.com/input-output-hk/cardano-wallet/pull/111/files#r273149458

akegalj · 2019-04-08T17:09:40Z

test/data/Cardano/Wallet/Api/PostTransactionData.json

+    "samples": [
+        {
+            "passphrase": "AR1Q_'7!S?n}Owo`OrO8^OJKiS\"0COj=(vkE}duI떚pw&$h?[+ l9*bW>⫪Y192RaqVPg>X!y\\Xsd<&^fqL/ji>xwl]dd+7!]\\*_H鏨oP'\\$(^li6vZ=}Zg\\Yz胶}r+M/I1S",
+            "targets": [


see comment here https://github.com/input-output-hk/cardano-wallet/pull/111/files#r273149458

akegalj · 2019-04-08T17:12:03Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+    arbitrary = Quantity . fromIntegral <$> (arbitrary @Word8)
+
+instance Arbitrary (Hash "Tx") where
+    arbitrary = Hash . B8.pack <$> replicateM 64 arbitrary


huge entropy here - do we want to give less entropy then 64 bytes for constructing Hash "Tx" in order to trigger collisions?

I don't see any particular reason to want collision at this level? Any reason I am missing?

no particular reason. It would be good to have such collision somewhere in tests. If not now top level then probably later somwhere closer to the core/primitives

test/unit/Cardano/Wallet/Api/TypesSpec.hs

akegalj

Diff of this PR is ~250L , rest is mostly golden files produced by our test spec so don't be intemidated

KtorZ

LGTM.

Let's keep the text decoder aligned and consistent with the others and fix the tx id length in the spec and get it merged A.S.A.P.

KtorZ · 2019-04-09T11:51:03Z

specifications/api/swagger.yaml

-  maxLength: 64
-  minLength: 64
+  maxLength: 128
+  minLength: 128


Hmmm no. The hex-encoded version is 64-byte, the hash itself is 32 bytes. The spec was right here (cf current transaction ids..)

KtorZ · 2019-04-09T11:55:28Z

src/Cardano/Wallet/Api/Types.hs

+    , depth :: !(Quantity "block" Natural)
+    , direction :: !(ApiT Direction)
+    , inputs :: !(NonEmpty ApiCoinSelection)
+    , outputs :: !(NonEmpty ApiCoinSelection)


Agreed that keeping types identical here makes sense (from the API perspective, they are the same thing). About the name clash / confusion, I would go for ApiCoins or ApiResolvedInputs to makes it slightly less ambiguous though, it's all more-or-less the same thing.

KtorZ · 2019-04-09T11:56:34Z

src/Cardano/Wallet/Api/Types.hs

+-- | Converts tx hash to a Base16-encoded string.
+encodeApiTxHash :: ApiT (Hash "Tx") -> Text
+encodeApiTxHash =
+    T.decodeUtf8 . convertToBase Base16 . getHash . getApiT


Let's define ToText and FromText instances for those in Primitive.Types 👍

test/data/Cardano/Wallet/Api/ApiT TxStatus.json

KtorZ · 2019-04-09T11:58:50Z

test/data/Cardano/Wallet/Api/CreateTransactionData.json

+        },
+        {
+            "passphrase": "7ehYx\\==l5𡄩\"RlwoO6R|k`1~;tVP)&𩺄2?!!?ujj{w]meZ]Ti5{TAu(M;%}_E휊Wao<Ji4AC<~w-&+gHH[fBVj⅝jp↯$e𧶥콁P:\".YOmVAQ7t]PpxEoc5;;/ypb<}e R@@F'H=2Y$ZUH[g𦲧XRH (L&!fCRTV/&wPaqWn54D!5gVⷝ-oshW_l)S/XN贂0<YWjjNM\\fu9",
+            "targets": [


Na it's okay. We aren't "quickchecking" that but actually, we do want to cover the maximum surface with the golden tests. Bigger is better here.

KtorZ · 2019-04-09T11:59:35Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+    arbitrary = Quantity . fromIntegral <$> (arbitrary @Word8)
+
+instance Arbitrary (Hash "Tx") where
+    arbitrary = Hash . B8.pack <$> replicateM 64 arbitrary


I don't see any particular reason to want collision at this level? Any reason I am missing?

test/unit/Cardano/Wallet/Api/TypesSpec.hs

Anviking

lgtm!

Think calling ApiCoinSelection something else makes sense

Anviking · 2019-04-09T12:42:57Z

src/Cardano/Wallet/Api/Types.hs

+    , depth :: !(Quantity "block" Natural)
+    , direction :: !(ApiT Direction)
+    , inputs :: !(NonEmpty ApiCoinSelection)
+    , outputs :: !(NonEmpty ApiCoinSelection)


Something other than ApiCoinSelection 👍

If possible, a short doc-comment about what it is would be awesome too. E.g if you were to go with ApiResolvedInputs, it might be worth knowing that it's used for outputs, and not just inputs of ApiTransaction

ApiCoins sounds reasonable to me. Or with some tweak like ApiOwnedCoins to indicate that they contain addresses, which I presume are the previous and current owner.

test/data/Cardano/Wallet/Api/ApiT TxStatus.json

KtorZ · 2019-04-09T16:49:01Z

test/data/Cardano/Wallet/Api/ApiBlockData.json

+            "time": "1864-06-04T01:25:12.205046097466Z",
+            "block": {
+                "epoch_number": 11863932,
+                "slot_number": 6928


I love how we are in 1864 but with epoch 11863932 🤷‍♂️

KtorZ · 2019-04-09T16:49:55Z

test/data/Cardano/Wallet/Api/ApiT (Hash "Tx").json

+        "bd1b1817695b42134ff5cc7a023972ca5c3d5c500b235c1a5d0a324f7d3c49284d6a1e51289e5031501f394a283720373c490d414f424d45178c4a27412e763d",
+        "2c47c30f62146920cc32468168daa03f1b127c6f7140044915de1a25541819047f4444224b5408690b480f056141794b38554c736474133e0083461e12787200",
+        "160830005e014910157d434c322f60101e5d785400550e094e7a3e668115782060225c234b7b7f5f3c2b4d345c554568416b4d732a2b4461155c3c4a8d05433b"
+    ]


Those looks surprisingly long. They are 128-char long 🤔 ?

hm - that looks strange because its generated with arbitrary 32 bytes/chars so that should give 64 chars string: https://github.com/input-output-hk/cardano-wallet/blob/dfa7167225ff44477c25333e1613f86e46286f15/test/unit/Cardano/Wallet/Api/TypesSpec.hs#L455 .

> hash <- generate (arbitrary :: Gen (Hash "Tx")) > hash Hash {getHash = "\NAKtc@\249X\DELd\DC3(5W\NUL\\]J('\DC2,_@?s\SO<\vq\RS,M\164"} > encode $ ApiT hash "\"15746340f9587f6413283557005c5d4a2827122c5f403f730e3c0b711e2c4da4\"" > length "15746340f9587f6413283557005c5d4a2827122c5f403f730e3c0b711e2c4da4" 64

Aha, I see you have fixed it. Sorry forgot to push regenerated golden tests

akegalj self-assigned this Mar 22, 2019

Anviking mentioned this pull request Mar 22, 2019

Model API Types for Transactions in Servant #93

Closed

8 tasks

KtorZ reviewed Mar 22, 2019

View reviewed changes

src/Cardano/Wallet/Api.hs Outdated Show resolved Hide resolved

KtorZ reviewed Mar 22, 2019

View reviewed changes

KtorZ force-pushed the master branch 2 times, most recently from 049fddd to a8d7c7d Compare March 23, 2019 12:45

akegalj force-pushed the akegalj/93/create_transaction_servant branch from c1f20d6 to 40bbd60 Compare March 29, 2019 07:11

akegalj commented Mar 29, 2019

View reviewed changes

src/Cardano/Wallet/Api/Types.hs Outdated Show resolved Hide resolved

akegalj commented Mar 29, 2019

View reviewed changes

src/Cardano/Wallet/Primitive/Types.hs Outdated Show resolved Hide resolved

akegalj force-pushed the akegalj/93/create_transaction_servant branch 2 times, most recently from cd5868e to 426d374 Compare April 8, 2019 16:44

akegalj commented Apr 8, 2019

View reviewed changes

specifications/api/swagger.yaml Show resolved Hide resolved

akegalj commented Apr 8, 2019

View reviewed changes

specifications/api/swagger.yaml Show resolved Hide resolved

akegalj commented Apr 8, 2019

View reviewed changes

specifications/api/swagger.yaml Show resolved Hide resolved

akegalj commented Apr 8, 2019

View reviewed changes

src/Cardano/Wallet/Primitive/Types.hs Show resolved Hide resolved

akegalj commented Apr 8, 2019

View reviewed changes

test/unit/Cardano/Wallet/Api/TypesSpec.hs Show resolved Hide resolved

akegalj commented Apr 8, 2019

View reviewed changes

test/unit/Cardano/Wallet/Api/TypesSpec.hs Show resolved Hide resolved

akegalj commented Apr 8, 2019

View reviewed changes

akegalj requested review from Anviking and KtorZ April 8, 2019 18:09

akegalj marked this pull request as ready for review April 8, 2019 18:11

akegalj changed the title ~~Akegalj/93/create transaction servant~~ Add servant post transaction endpoint Apr 8, 2019

KtorZ approved these changes Apr 9, 2019

View reviewed changes

Anviking approved these changes Apr 9, 2019

View reviewed changes

akegalj force-pushed the akegalj/93/create_transaction_servant branch 3 times, most recently from ca612bf to 6b2d8b3 Compare April 9, 2019 15:16

KtorZ reviewed Apr 9, 2019

View reviewed changes

akegalj added 8 commits April 9, 2019 19:25

Rebased on top of master (temp commit - adding json instances)

3ea6e97

Add json instances for new transaction servant api endpoint

939dd94

Add server <-> swagger glue code for post transaction endpoint

0f1802e

Add golden files. Adjust swagger spec with datatypes.

9b5d26b

Add tx hash decode unit test

7b484ad

Fix tx hash size

8f72d8d

Rename ApiCoinSelection into ApiCoins

e9e0bcf

Regenerate golden tests

dfa7167

KtorZ force-pushed the akegalj/93/create_transaction_servant branch from 6b2d8b3 to dfa7167 Compare April 9, 2019 17:26

KtorZ merged commit 1cedad6 into master Apr 9, 2019

KtorZ deleted the akegalj/93/create_transaction_servant branch April 9, 2019 18:18

KtorZ mentioned this pull request Apr 9, 2019

Increase golden test sampleSize from 4 to 8 #168

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add servant post transaction endpoint #111

Add servant post transaction endpoint #111

akegalj commented Mar 22, 2019 •

edited

Loading

KtorZ Mar 22, 2019

akegalj Apr 8, 2019

akegalj Mar 29, 2019

akegalj Apr 8, 2019

akegalj Apr 8, 2019

KtorZ Apr 9, 2019

akegalj Apr 9, 2019

akegalj Apr 8, 2019

KtorZ Apr 9, 2019

Anviking Apr 9, 2019

akegalj Apr 9, 2019 •

edited

Loading

Anviking Apr 9, 2019

akegalj Apr 8, 2019

akegalj Apr 8, 2019

KtorZ Apr 9, 2019 •

edited

Loading

akegalj Apr 8, 2019

akegalj Apr 8, 2019

akegalj Apr 8, 2019

KtorZ Apr 9, 2019

akegalj Apr 9, 2019

akegalj left a comment

KtorZ left a comment

KtorZ Apr 9, 2019

KtorZ Apr 9, 2019

KtorZ Apr 9, 2019

KtorZ Apr 9, 2019 •

edited

Loading

KtorZ Apr 9, 2019

Anviking left a comment

Anviking Apr 9, 2019

KtorZ Apr 9, 2019

KtorZ Apr 9, 2019

akegalj Apr 9, 2019

akegalj Apr 9, 2019

Add servant post transaction endpoint #111

Add servant post transaction endpoint #111

Conversation

akegalj commented Mar 22, 2019 • edited Loading

Issue Number

Overview

Comments

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj Apr 9, 2019 • edited Loading

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 Apr 9, 2019 • edited Loading

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

akegalj left a comment

Choose a reason for hiding this comment

KtorZ 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

KtorZ Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking 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

akegalj commented Mar 22, 2019 •

edited

Loading

akegalj Apr 9, 2019 •

edited

Loading

KtorZ Apr 9, 2019 •

edited

Loading

KtorZ Apr 9, 2019 •

edited

Loading