Add transaction signing API endpoint with stub impl #2642

rvl · 2021-05-06T21:26:35Z

Issue Number

Overview

Swagger spec updates for new transaction signing API.
New API types and servant stuff.
Rename some definitions to old so as to mark them as soft-deprecated.
Add stub implementation to server handler with TODO comments.

paweljakubas · 2021-05-10T16:26:22Z

lib/core/src/Cardano/Wallet.hs

+    -> ((k 'RootK XPrv, Passphrase "encryption") -> (XPrv, Passphrase "encryption"))
+       -- ^ Reward account derived from the root key (or somewhere else).
+    -> Passphrase "raw"
+    -> ByteString


Maybe introduce newtype TxBody rather than ByteString ?

Yes it needs some type, but that will be added in the next PR. This function is a placeholder stub, so I hope that ByteString is good enough for the moment.

paweljakubas · 2021-05-10T16:29:06Z

lib/core/src/Cardano/Wallet/Api.hs

+    :> "transactions"
+    :> "sign"
+    :> ReqBody '[JSON] PostSignTransactionData
+    :> PostAccepted '[JSON, OctetStream] ApiSerialisedTransaction


we want both json and octet, not just octet?

Is it connected with the fact that we want both binary (serviced by octet) and hex string (serviced by json)?

The user can decide whether they want raw transaction bytes, or hex-encoded bytes inside a JSON object.

paweljakubas · 2021-05-10T16:31:13Z

lib/core/src/Cardano/Wallet/Api/Types.hs

-data PostTransactionData (n :: NetworkDiscriminant) = PostTransactionData
+data PostSignTransactionData = PostSignTransactionData
+    { txBody :: !ApiSerialisedTransaction -- TODO: ADP-902 or tx
+    , passphrase :: !(ApiT (Passphrase "lenient"))


"lenient" here 🤔

It means that there is no minimum length for the given passphrase.
We only want a minimum length when creating a new wallet or changing its password.

paweljakubas · 2021-05-10T16:31:59Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+instance FromJSON ApiSerialisedTransaction where
+    parseJSON = fmap ApiSerialisedTransaction . fromBaseText Base16
+instance ToJSON ApiSerialisedTransaction where
+    toJSON = toBaseText Base16 . view #payload


very elegant I must say:-)

paweljakubas · 2021-05-10T16:40:11Z

specifications/api/swagger.yaml

+      title: Serialized transaction witnesses
+
+    ApiSerialisedTransaction: &ApiSerialisedTransaction
+      oneOf:


ah, I got it now, why [JSON, OctetStream]

paweljakubas · 2021-05-10T16:42:43Z

specifications/api/swagger.yaml

  /wallets/{walletId}/transactions:
    post:
      operationId: postTransaction
-      tags: ["Transactions"]
+      tags: ["Legacy Transactions"]


what about waiting with Transactions -> Legacy Transactions and stable -> deprecated until the last ticket in this epic?

OK fair enough.

paweljakubas · 2021-05-10T16:48:46Z

specifications/api/swagger.yaml

+          required:
+            - body
+            - wits
+          properties:


How should I understand this? Either we get serialized hex which means all needed witnesses were constructed and added and the signed txs can be submitted OR I have partially signed tx, ie., not enough witnesses collected yet to treat it as fully signed tx?

I added some more swagger doc strings.
The idea is that the user can submit an encoded tx, or an encoded tx body with zero or more witnesses.
The signing endpoint will add new witnesses to the set.

paweljakubas · 2021-05-10T16:54:30Z

@rvl we have now

postSignTransaction in which we have serialized tx body and password needed to sign. As a result we have blob that can be sent.
postSignedTransaction which submits the result of 1.

What about creating serialized tx body from outputs, delegations, ... It is in another ticket, right?

rvl · 2021-05-11T05:46:15Z

Thanks for the review comments @paweljakubas .

You were probably slightly confused because I made an error with the return types.
PostSignedTransaction should return either a TxId or error, nothing else.

Constructing the TxBody is for another PR, ticket ADP-909.

KtorZ · 2021-05-11T07:47:35Z

lib/core/src/Cardano/Wallet/Api.hs

+type SignTransaction n = "wallets"
+    :> Capture "walletId" (ApiT WalletId)
+    :> "transactions"
+    :> "sign"


What about signatures to be a bit more rest-ish. Verbs / action should ideally be captured by the HTTP method.

I'm afraid that REST is a poor fit for this endpoint (perhaps in general too, but that's a separate discussion).

The resource is the wallet (specifically the keys contained in the wallet) and action is to sign a blob of data (a TxBody), given a spending password, and return the blob of data plus its signatures (TxBody + TxWits).

None of the HTTPs methods adequately describe this signing operation. And no request path name can make a verb look like a noun.

So I think the least worst name is something short and simple: POST /v2/wallet/<id>/transactions/sign.

Slightly related, I think the corresponding servant handler function name I have chosen is thoroughly confusing: postSignTransaction. What do you think of renaming that to signTransactionHandler ?

(perhaps in general too, but that's a separate discussion).

I couldn't agree more.

None of the HTTPs methods adequately describe this signing operation. And no request path name can make a verb look like a noun.

I see it differently 🤔 , to me POST /wallets/{walletId}/witnesses for instance would read just fine. This creates a witness resource from a given payload on the given wallet. So it's kinda rest-ish.

Slightly related, I think the corresponding servant handler function name I have chosen is thoroughly confusing: postSignTransaction. What do you think of renaming that to signTransactionHandler ?

Yeah... I always have mixed feelings when naming handlers. One the one hand, keeping some form of consistent naming from the endpoint structure makes it "brain dead" and easy to name / figure out. But on the other hand, it's often ugly and, we have deviated so many times from it that we have lost any benefits from consistent naming. This being said, I like your suggestion 😊

KtorZ · 2021-05-11T07:54:13Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -778,22 +779,27 @@ data ByronWalletPutPassphraseData = ByronWalletPutPassphraseData
    , newPassphrase :: !(ApiT (Passphrase "raw"))
    } deriving (Eq, Generic, Show)

-data PostTransactionData (n :: NetworkDiscriminant) = PostTransactionData
+data PostSignTransactionData = PostSignTransactionData
+    { tx :: !ApiSerialisedTransaction -- TODO: ADP-902 or tx


We've avoided shorten names and acronym in the API so far. I'd suggest to go for "transaction" or "body" since this is really just the transaction body without witnesses in principle.

Yep good idea, fixed.

KtorZ · 2021-05-11T07:55:09Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -2072,9 +2078,19 @@ instance ToJSON (ApiT BoundType) where
 instance FromJSON (ApiT BoundType) where
    parseJSON = fmap ApiT . genericParseJSON defaultSumTypeOptions

-instance DecodeAddress t => FromJSON (PostTransactionData t) where
+instance FromJSON ApiSerialisedTransaction where
+    parseJSON = fmap ApiSerialisedTransaction . fromBaseText Base16


Wouldn't Base64 be a better fit here? We typically use Base16 for short-ish strings and base64 for longer payloads.

I don't think it matters that much. I thought hex might be slightly easier to use. Base64 is 1.33× raw size and hex is 2× raw size - both acceptable for transactions given their max size. Anyway I have changed it to Base64 because the sign metadata endpoint uses Base64, as you say. I also changed the encoding accepted by the cardano-wallet transaction submit CLI from hex to base64 to align with this.

KtorZ · 2021-05-11T07:57:23Z

lib/core/test/unit/Cardano/Wallet/Api/Malformed.hs

+         jsonValid = first (BodyParam . Aeson.encode) <$> paymentCases ++
+            [ -- passphrase
+              ( [aesonQQ|
+                { "tx_body": "cafecafe"


I didn't catch where the code from Api/Types could generate a tx_body field 🤔 ?

Yes I changed the types and forgot to update these tests.

KtorZ · 2021-05-11T07:58:29Z

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs

 instance
    ( ReflectMethod m
    , Accept ct
-    ) => GEveryEndpoints (Verb (m :: StdMethod) s '[ct] a)
+    , GEveryEndpoints (Verb m s cts a)
+    ) => GEveryEndpoints (Verb (m :: StdMethod) s (ct ': cts) a)


paweljakubas

lgtm, the rest bits can come in next PRs

Anviking · 2021-06-03T13:32:06Z

lib/core/src/Cardano/Wallet/Primitive/Types/Tx.hs

+-- | @SerialisedTxParts@ is a serialised transaction body, and a possibly
+-- incomplete set of serialised witnesses, along with an encoding of the
+-- combined transaction.
+data SerialisedTxParts = SerialisedTxParts


Why is SerialisedTxParts needed when we have SerialisedTx?

Isn't the structure it exposes already unambiguously exposed in the raw CBOR?

Yes, but I think it will be helpful for API consumers to get both.

rvl · 2021-07-06T06:17:08Z

bors r+

paweljakubas · 2021-07-06T06:26:10Z

lib/core/src/Cardano/Wallet.hs

+        )
+    => ctx
+    -> WalletId
+    -> ((k 'RootK XPrv, Passphrase "encryption") -> (XPrv, Passphrase "encryption"))


well, as I did in construct tx PR, one can argue we can read public reward account key from wallet state, we need this structure only when private reward account key is needed...which probably is the case here

paweljakubas

much work is needed to implement signing and prove it works which is to be done in coming PRs. API seems reasonable, some solid/beautifully-conveying-idea-under-the-hood data structures were introduced

iohk-bors · 2021-07-06T07:06:32Z

Build succeeded:

rvl self-assigned this May 6, 2021

paweljakubas reviewed May 10, 2021

View reviewed changes

rvl force-pushed the rvl/adp-902/tx-signing-api branch from 3e6c7f9 to 78da2db Compare May 11, 2021 05:56

KtorZ reviewed May 11, 2021

View reviewed changes

paweljakubas approved these changes May 12, 2021

View reviewed changes

rvl force-pushed the rvl/adp-902/tx-signing-api branch 4 times, most recently from b4f682d to 6e4a0e0 Compare May 20, 2021 14:12

rvl force-pushed the rvl/adp-902/tx-signing-api branch 2 times, most recently from 4d6959f to ace76e5 Compare May 28, 2021 11:43

rvl force-pushed the rvl/adp-902/tx-signing-api branch from ace76e5 to aac5f1d Compare June 2, 2021 15:53

Anviking reviewed Jun 3, 2021

View reviewed changes

rvl force-pushed the rvl/adp-902/tx-signing-api branch 4 times, most recently from c5a072a to e5e832b Compare July 1, 2021 11:17

rvl marked this pull request as ready for review July 1, 2021 11:43

rvl force-pushed the rvl/adp-902/tx-signing-api branch from e5e832b to c260987 Compare July 2, 2021 10:28

rvl and others added 3 commits July 5, 2021 21:05

Swagger changes

9f43ae7

API type and stub server handlers

90afbad

change endpoint to pass malformed

2dcdc8a

Remove octet-stream alternatives from swagger and Api

23b568f

rvl force-pushed the rvl/adp-902/tx-signing-api branch from c260987 to 3b80538 Compare July 5, 2021 13:52

swagger: Remove alternate tx representations

d6eddc3

rvl force-pushed the rvl/adp-902/tx-signing-api branch from 3b80538 to 554f07a Compare July 5, 2021 13:59

Rework and simplify Api Types

a365f05

rvl force-pushed the rvl/adp-902/tx-signing-api branch from 554f07a to a365f05 Compare July 5, 2021 14:27

paweljakubas reviewed Jul 6, 2021

View reviewed changes

paweljakubas approved these changes Jul 6, 2021

View reviewed changes

iohk-bors bot merged commit 81f3c47 into master Jul 6, 2021

iohk-bors bot deleted the rvl/adp-902/tx-signing-api branch July 6, 2021 07:06

WilliamKingNoel-Bot pushed a commit that referenced this pull request Jul 6, 2021

docs: Merge #2642

7bee939

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transaction signing API endpoint with stub impl #2642

Add transaction signing API endpoint with stub impl #2642

rvl commented May 6, 2021 •

edited

Loading

paweljakubas May 10, 2021

rvl May 11, 2021

paweljakubas May 10, 2021

paweljakubas May 10, 2021

rvl May 11, 2021

paweljakubas May 10, 2021

rvl May 11, 2021

paweljakubas May 10, 2021

paweljakubas May 10, 2021

paweljakubas May 10, 2021

rvl May 11, 2021

paweljakubas May 10, 2021

rvl May 11, 2021

paweljakubas commented May 10, 2021

rvl commented May 11, 2021

KtorZ May 11, 2021

rvl May 11, 2021

KtorZ May 12, 2021 •

edited

Loading

KtorZ May 11, 2021

rvl May 11, 2021

KtorZ May 11, 2021

rvl May 11, 2021

KtorZ May 11, 2021

rvl May 11, 2021

KtorZ May 11, 2021

paweljakubas left a comment

Anviking Jun 3, 2021

rvl Jun 4, 2021

rvl commented Jul 6, 2021

paweljakubas Jul 6, 2021

paweljakubas left a comment

iohk-bors bot commented Jul 6, 2021

Add transaction signing API endpoint with stub impl #2642

Add transaction signing API endpoint with stub impl #2642

Conversation

rvl commented May 6, 2021 • edited Loading

Issue Number

Overview

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented May 10, 2021

rvl commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ May 12, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented Jul 6, 2021

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

iohk-bors bot commented Jul 6, 2021

rvl commented May 6, 2021 •

edited

Loading

KtorZ May 12, 2021 •

edited

Loading