Add `postSignedTx` to HttpBridge #130

Anviking · 2019-03-28T10:04:13Z

Issue Number

#89

Overview

Adds support for submitting signed transactions to the node / httpBridge.

Added encodeSignedTx, encodeWitness,
Improved decodeSignedTx and decodeWitness
Added SignedTx <base64> type and sign :: Tx -> [Witness] -> SignedTx
Added support for submitting txs to POST :network/txs/signed

Comments

Rebased into 3 commits: 1) stub API, 2) extend Binary.hs, 3) connect the two and add integration tests for submitting signed txs

src/Servant/Extra/ContentTypes.hs

test/integration/Cardano/Wallet/Network/HttpBridgeSpec.hs

akegalj

Looks good so far

src/Cardano/Wallet/Binary.hs

Anviking · 2019-04-01T11:40:04Z

src/Cardano/Wallet/Binary.hs

+    <> mconcat (map encodeOne list)
+
+sign :: Tx -> [Witness] -> SignedTx
+sign tx witnesses = signedTxFromBS . toBS $ encodeSignedTx (tx, witnesses)


Unsure about the location of this function as well as SignedTx

For lack of a better place, I think they're fine here for now. Though here I think the name is misleading. I'd expect a "sign" function to produce the actual witnesses for a given address. Here, it's really just composing things together.

(as mentioned in another comment, I don't think there's any need for that at this stage and encodeSignedTx is enough on its own).

(It is currently removed) but I don't like polluting the integration tests with imports to CBOR and BL + and forced to add cborg as dependency.

I do think/suspect something with the signature :: Tx -> [Witness] -> SignedTx should exist.

src/Cardano/Wallet/Network/HttpBridge.hs

Anviking · 2019-04-01T11:46:34Z

test/unit/Cardano/Wallet/BinarySpec.hs

+cborRoundtrip decode encode a = do
+    let bytes = CBOR.toLazyByteString $ encode a
+    let a' = unsafeDeserialiseFromBytes decode bytes
+    a `shouldBe` a'


Two thoughts:

Arbitrary for roundtrips?

Use ToCBOR/FromCBOR classes more? (I say despite having expressed scepticism of classes perviously)

There are no such classes (which I think is a good thing here). And, providing arbitrary instance for Tx, Block and others is trickier than it seems., although, if it just for testing encoder / decoder, we may get away with semantically invalid generators 🤔

KtorZ

Nice work. One thing to fix before merging is the representation for SignedTx and a couple of small remarks 👍

cardano-wallet.cabal

KtorZ · 2019-04-01T15:34:45Z

src/Cardano/Wallet/Binary.hs

+encodeTxWitness wit = mempty
+    <> CBOR.encodeListLen 2
+    <> CBOR.encodeWord8 tag
+    <> CBOR.encodeTag 24 -- "magic" constant


By the by, I figured while looking at the RFC 7049. 24 corresponds to the semantic "Encoded CBOR data item"

Thought about adding it, but didn't fit on a line, and there are multiple existing Hard-Coded Tag value in cardano-sl comments. Would go in the module level comment, or a separate solitary comment in that case 🤷‍♂️

KtorZ · 2019-04-01T15:36:20Z

src/Cardano/Wallet/Binary.hs

+    <> mconcat (map encodeOne list)
+
+sign :: Tx -> [Witness] -> SignedTx
+sign tx witnesses = signedTxFromBS . toBS $ encodeSignedTx (tx, witnesses)


For lack of a better place, I think they're fine here for now. Though here I think the name is misleading. I'd expect a "sign" function to produce the actual witnesses for a given address. Here, it's really just composing things together.

(as mentioned in another comment, I don't think there's any need for that at this stage and encodeSignedTx is enough on its own).

KtorZ · 2019-04-01T15:40:40Z

src/Cardano/Wallet/Primitive/Types.hs

@@ -318,6 +323,16 @@ newtype Timestamp = Timestamp
    } deriving (Show, Generic, Eq, Ord)


+-- | A signed tx is just a base64-encoded string.
+newtype SignedTx = SignedTx { signedTx :: String }


The base64 encoding (or any sort of encoding actually) is good for some contexts; I'd suggest to have our core types work with types directly and perform conversions to base64, base58 or base16 (or anything else) when needed (similarly to what we do with Hash or Address).

newtype SignedTx = SignedTx { signedTx :: ByteString }

src/Cardano/Wallet/Primitive/Types.hs

src/Cardano/Wallet/Network/HttpBridge.hs

KtorZ · 2019-04-01T15:51:00Z

test/unit/Cardano/Wallet/BinarySpec.hs

+cborRoundtrip decode encode a = do
+    let bytes = CBOR.toLazyByteString $ encode a
+    let a' = unsafeDeserialiseFromBytes decode bytes
+    a `shouldBe` a'


There are no such classes (which I think is a good thing here). And, providing arbitrary instance for Tx, Block and others is trickier than it seems., although, if it just for testing encoder / decoder, we may get away with semantically invalid generators 🤔

Anviking · 2019-04-02T12:06:28Z

src/Cardano/Wallet/Network/HttpBridge/Api.hs

+        return $ ApiT . SignedTx $ bs
+      where
+        c :: String -> Either String ByteString
+        c bs = convertFromBase Base64 (B8.pack bs)


Now I should definitely add roundtrip tests

Anviking commented Mar 28, 2019

View reviewed changes

src/Servant/Extra/ContentTypes.hs Outdated Show resolved Hide resolved

Anviking commented Mar 28, 2019

View reviewed changes

test/integration/Cardano/Wallet/Network/HttpBridgeSpec.hs Outdated Show resolved Hide resolved

akegalj reviewed Mar 28, 2019

View reviewed changes

KtorZ assigned Anviking Mar 29, 2019

piotr-iohk mentioned this pull request Mar 29, 2019

Extend network layer to support transaction creation #89

Closed

5 tasks

KtorZ reviewed Mar 29, 2019

View reviewed changes

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

KtorZ reviewed Mar 29, 2019

View reviewed changes

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

Anviking force-pushed the anviking/89/network-post-tx branch from fe4769d to eb58cda Compare April 1, 2019 11:32

Anviking marked this pull request as ready for review April 1, 2019 11:33

Anviking commented Apr 1, 2019

View reviewed changes

Anviking changed the title ~~Anviking/89/network post tx~~ Add postSignedTx to HttpBridge Apr 1, 2019

Anviking requested a review from KtorZ April 1, 2019 12:02

Anviking force-pushed the anviking/89/network-post-tx branch 3 times, most recently from 61a52c7 to 09224c7 Compare April 1, 2019 14:38

KtorZ approved these changes Apr 1, 2019

View reviewed changes

Anviking force-pushed the anviking/89/network-post-tx branch 2 times, most recently from 82e18ad to 35306d7 Compare April 2, 2019 11:17

Add postSignedTx to HttpBridge

577ec5a

Anviking force-pushed the anviking/89/network-post-tx branch from 35306d7 to c6eaea7 Compare April 2, 2019 12:02

Anviking commented Apr 2, 2019

View reviewed changes

Anviking added 2 commits April 2, 2019 14:10

Improve CBOR for SignedTx and Witness

6b4dcdd

Add sign and integration test for submitting txs

028a5ef

Anviking force-pushed the anviking/89/network-post-tx branch from c6eaea7 to 028a5ef Compare April 2, 2019 12:10

KtorZ merged commit 598fb61 into master Apr 2, 2019

KtorZ deleted the anviking/89/network-post-tx branch April 2, 2019 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `postSignedTx` to HttpBridge #130

Add `postSignedTx` to HttpBridge #130

Anviking commented Mar 28, 2019 •

edited

Loading

akegalj left a comment

Anviking Apr 1, 2019

KtorZ Apr 1, 2019

Anviking Apr 2, 2019

Anviking Apr 1, 2019

KtorZ Apr 1, 2019

KtorZ left a comment

KtorZ Apr 1, 2019

Anviking Apr 2, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

Anviking Apr 2, 2019

Add postSignedTx to HttpBridge #130

Add postSignedTx to HttpBridge #130

Conversation

Anviking commented Mar 28, 2019 • edited Loading

Issue Number

Overview

Comments

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `postSignedTx` to HttpBridge #130

Add `postSignedTx` to HttpBridge #130

Anviking commented Mar 28, 2019 •

edited

Loading