Transaction API Handler #182

KtorZ · 2019-04-25T13:49:57Z

Issue Number

#93

Overview

I have implemented the API handler to construct, sign and submit transactions
I have computed, stored and returned corresponding transaction metadata

Comments

Haven't tested anything yet, still waiting for a few things regarding the transaction signing testing first.

⚠️ Base Branch Isn't master ⚠️

akegalj · 2019-04-25T15:34:32Z

seems a bit broader than what #93 aimed at - but that's minor

KtorZ · 2019-04-25T15:46:14Z

Well, depends how you interpret the acceptance criteria

"The API must support creation of transactions according to the API Swagger specification https://raw.githubusercontent.com/input-output-hk/cardano-wallet/master/specifications/api/swagger.yaml"

:/

akegalj

Looks good so far, few minor points

akegalj · 2019-04-25T15:40:03Z

src/Cardano/Wallet.hs

-
-    , signTx = \wid creds (CoinSelection ins outs chgs) -> DB.withLock db $ do
+        withExceptT ErrSubmitTxNetwork $ postTx network signed
+        let amt = fromIntegral $ sum (getCoin . coin <$> tx ^. #outputs)


hm, in this case amount will always be equal to sum of all input accounts, as this counts in also change amount. Is it expected from clients that TxMeta.amount includes change amount?

Ah! Good point. Will correct this.

akegalj · 2019-04-25T15:43:00Z

src/Cardano/Wallet.hs

@@ -360,8 +407,8 @@ mkWalletLayer db network = WalletLayer
        -> ExceptT ErrNoSuchWallet IO ()
    restoreBlocks wid blocks tip = do
        let (inf, sup) =
-                ( slotId . header . head $ blocks
-                , slotId . header . last $ blocks
+                ( view #slotId . header . head $ blocks


is there a real need to use lens sintax here?

Unfortunately yes, because we have conflicting record names. I am not quite satisfied with that, mostly because we don't use lens consistently at the moment. So, it feels really ad-hoc ine some cases.

aha I see - I didn't notice name conflict. In that case it looks sensible (not ideal - but still very clear to the reader)

akegalj · 2019-04-25T15:47:37Z

src/Cardano/Wallet/Api/Types.hs

@@ -150,7 +152,7 @@ data PostTransactionData = PostTransactionData
    , passphrase :: !(ApiT (Passphrase "encryption"))
    } deriving (Eq, Generic, Show)

-data ApiTransaction = Transaction
+data ApiTransaction = ApiTransaction


:/ nice catch

akegalj · 2019-04-25T15:50:33Z

src/Cardano/Wallet/Api/Types.hs

+    parseJSON bytes = do
+        v@(ApiCoins _ (Quantity c)) <-
+            genericParseJSON defaultRecordTypeOptions bytes
+        if isValidCoin (Coin $ fromIntegral c)


akegalj · 2019-04-25T15:51:41Z

src/Cardano/Wallet/Api/Types.hs

+        if isValidCoin (Coin $ fromIntegral c)
+            then return v
+            else fail $
+                "invalid coin value: value has to be lower than"


"lower than or equal"

Fun fact: In English, "lower than" (or similarly, "higher than") have the same meaning as < and, we therefore require "lower than or equal" to mean <=.

In French, the literal translation for "lower than" would be "inférieur à", which in Maths, is interpreted as <=, and French would say "strictement inférieur à" to mean < (which would translate to "strictly lower than").

Anyway, Good catch 😅

Interesting!

For this example, Croatian follows the same natural language rules as English does (probably that's the reason why it was easier to spot it for me - and easier to do the mistake for you). Interesting how we tend to think/solve-problems in our natural language constrains

akegalj · 2019-04-25T15:54:28Z

src/Cardano/Wallet/Primitive/Model.hs

+    :: Tx
+    -> Wallet s
+    -> Wallet s
+newPending !tx (Wallet !utxo !pending !history !tip !s) =


maybe dumb question: is there a reason strict annotation is really necessary here? (I don't usually use this until I am really sure this will create lots of unnecessary thunks - but I haven't looked at the code to figure out is this so)

On newPending it's more debatable. It's definitely necessary on the applyBlock to avoid constructing too many chunks (as we generally apply a whole epoch before evaluating the state again). On new pending, we evaluate it right away by storing it to disk.

akegalj · 2019-04-25T16:07:32Z

Well, depends how you interpret the acceptance criteria

"The API must support creation of transactions according to the API Swagger specification https://raw.githubusercontent.com/input-output-hk/cardano-wallet/master/specifications/api/swagger.yaml"

:/

I didn't thought this was part of this task but I see it can be if "must support" means "API must support fully working implementation" and not "API must have endpoint" that I thought it means. My bad

KtorZ · 2019-04-26T10:20:33Z

@akegalj: "My bad"

Well, requirements can be clearer too ;) ... Hence the need for discussing and reviewing them.

…evel

…ckData null case

KtorZ · 2019-04-26T12:03:59Z

@akegalj May you have a second look at this ?

akegalj · 2019-04-26T12:05:44Z

@akegalj May you have a second look at this ?

I will have a second look immediatelly

akegalj

LGTM

akegalj · 2019-04-26T14:19:50Z

src/Cardano/Wallet.hs

+                            { status = Pending
+                            , direction = Outgoing
+                            , slotId = currentTip w
+                            , amount = Quantity (amtInps - amtChng)


yes - I think this is what we are aiming for

KtorZ requested a review from akegalj April 25, 2019 13:49

KtorZ self-assigned this Apr 25, 2019

akegalj reviewed Apr 25, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/95/keystore-in-db branch from d6cddec to 2484bf1 Compare April 25, 2019 17:21

Anviking mentioned this pull request Apr 26, 2019

Model API Types for Transactions in Servant #93

Closed

8 tasks

Anviking force-pushed the KtorZ/95/keystore-in-db branch from 2484bf1 to 62a26d2 Compare April 26, 2019 09:40

KtorZ mentioned this pull request Apr 26, 2019

Extend Wallet Layer to Support Transaction Creation #95

Closed

5 tasks

wire-up transaction construction, signing and submission at the API-l…

5f47b3e

…evel

KtorZ force-pushed the KtorZ/93/transaction-api-handler branch from 20478d3 to 513448d Compare April 26, 2019 10:59

KtorZ changed the base branch from KtorZ/95/keystore-in-db to master April 26, 2019 10:59

KtorZ force-pushed the KtorZ/93/transaction-api-handler branch from 513448d to 1ad8083 Compare April 26, 2019 11:10

KtorZ added 2 commits April 26, 2019 14:03

compute, store and return transaction metadata upon submission

0ecd9d0

re-generate golden tests for ApiTransaction to account for new ApiBlo…

6d09535

…ckData null case

KtorZ force-pushed the KtorZ/93/transaction-api-handler branch from 1ad8083 to 6d09535 Compare April 26, 2019 12:03

akegalj approved these changes Apr 26, 2019

View reviewed changes

KtorZ merged commit 598b911 into master Apr 26, 2019

KtorZ deleted the KtorZ/93/transaction-api-handler branch April 26, 2019 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction API Handler #182

Transaction API Handler #182

KtorZ commented Apr 25, 2019

akegalj commented Apr 25, 2019

KtorZ commented Apr 25, 2019

akegalj left a comment •

edited

Loading

akegalj Apr 25, 2019

KtorZ Apr 26, 2019

akegalj Apr 25, 2019

KtorZ Apr 26, 2019

akegalj Apr 26, 2019 •

edited

Loading

akegalj Apr 25, 2019

akegalj Apr 25, 2019

akegalj Apr 25, 2019

KtorZ Apr 26, 2019 •

edited

Loading

akegalj Apr 26, 2019

akegalj Apr 25, 2019

KtorZ Apr 26, 2019

akegalj commented Apr 25, 2019

KtorZ commented Apr 26, 2019

KtorZ commented Apr 26, 2019

akegalj commented Apr 26, 2019

akegalj left a comment

akegalj Apr 26, 2019

Transaction API Handler #182

Transaction API Handler #182

Conversation

KtorZ commented Apr 25, 2019

Issue Number

Overview

Comments

akegalj commented Apr 25, 2019

KtorZ commented Apr 25, 2019

akegalj left a comment • 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

akegalj Apr 26, 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 26, 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

akegalj commented Apr 25, 2019

KtorZ commented Apr 26, 2019

KtorZ commented Apr 26, 2019

akegalj commented Apr 26, 2019

akegalj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj left a comment •

edited

Loading

akegalj Apr 26, 2019 •

edited

Loading

KtorZ Apr 26, 2019 •

edited

Loading