ADP-919: Sign Transactions Endpoint #2943

KtorZ · 2021-10-01T16:55:27Z

📍 Remove withdrawal from the sign-transaction API.
Withdrawals are 'needed' in this context when they refer to an external wallet (i.e. via a mnemonic phrase) so that, the required signature for such withdrawal can be produced (derived from the root key associated with the mnemonic). However:

(a) The use-case for such external withdrawals stem from the ITN rewards, which were distributed more than 14 months ago to Shelley wallets that had participated in the incentivized testnet. It is reasonable to think that most concerned users have already redeemed their rewards. Those who haven't can still do it via the old transaction endpoint.

(b) Having withdrawal as part of this API endpoint is awkward and confusing. Instead, it would be more logical / practical to perform the signature during the construction of the transaction; that is, when constructing a transaction with an external withdrawal, the result would already contain a signature from the associated mnemonic. Such transaction would then still need to be passed to the sign-transaction endpoint to gather signature from the wallet itself.

Alternatively, not supporting this feature through the new transaction construction endpoint is also a very sensible choice. Regardless of the option, worrying about it now is unnecessary / a waste of time.
📍 Extract vk signing from 'mkTx'
Note that, doing so I have slightly altered the semantic of the function which can no longer return a 'ErrKeyNotFoundForAddress'.

Not finding the relevant key for a given input will now simply skip that input. Ultimately, the code used to yield to a 500 server errors BUT, that case was truly unreachable. Proof is that there's no end-to-end test which happen to be looking for that error case since it cannot (must not) occur under any circumstances. It really is a deep server error.

This change does not however makes the error 'silent', but merely defers it to the submission. If a necessary signature is missing, the transaction will eventually be rejected by the local node which will inform the user about the missing key.

The rationale for changing the semantic is that, in the context of signing 'external' transactions, we want to allow other inputs than ours. This is crucial in the context of Alonzo since transactions may contain and consume script inputs. So it's important for this operation to skip such inputs and simply care about signing inputs relevant of the wallet.

Note that, as a TODO, we should also now check for extra required signatures which can be specified in the transaction body.
📍 Extend transaction layer with 'addVkWitnesses' and use it.
📍 Move withdrawal signing into 'signTransaction'
So that we avoid too many repetition in the upcoming implementation of addVkWitnesses, and also, centralize the creation of signatures into one place. Note that this does not change the semantic of the function and it works regardless of where the reward account comes from (external or internal).
📍 Implement 'addVkWitnesses' for the shelley target.
A bit of repetition here because of the encapsulated era which can't leave its scope. All-in-all, it's not a big deal and someone has to deal with the multiple eras into some place, might it be via a type-class or via similar branches in a case like here. Note also that in case of Byron transactions, this does not work, which is likely a fair limitation.
📍 Add basic end-to-end test for signing
The test does a full cycle: construction > signing > submission.

However, some elements of the signing response are currently stubbed out. Obtaining them (the serialized body and witnesses) is doable but also look like extra / needless work :thinking_face: ? I'd argue that returning both the full serialized tx AND the serialized body AND the serialized witnesses is redundant. It seems like the interface currently is the serialized transaction (also with the construction endpoint) so it'd make more sense to only return this. To be discussed.
📍 Fix unit test after changing semantic of transaction construction
This no longer fails on unknown inputs, but skip the input.
📍 Add test rejecting unsigned Tx
📍 Add new test for signing withdrawals
📍 Fix compilation error on withdrawal test.
📍 Add counter-example serialized transaction.
📍 Add unit test for failed CBOR decoding of tx w/ withdrawal
This unit test comes from failed integration test for signing transactions.
📍 Decode transaction bytes from base16 before deserialising them 🤦.
📍 Rely on 'SealedTx' JSON instance for serialization (Base64)
And do not provide raw bytes which are automatically serialized as Base16
📍 Factor out submission code from all new-tx sign tests.
📍 Remove redundant response elements from sign transaction result.
The rationale is that, these elements are already present in the serialized transaction and, we assume that a client able to consume parts of the serialized transaction and put them together manually into a full serialized transaction would know anyway how to handle CBOR in the first place. Thus, splitting the full serialized transactions into smaller part if left as a client concern.
📍 Add e2e test scenario illustrating requiredSigners for transactions
This currently fails, as expected, since the feature isn't yet implemented in the API handler.
📍 Include signatures for 'extra-key-witnesses' in transactions
Note however that the wallet will only include signatures for keys it knows of, that is, for keys that are within the current visible window of the sequential state. That is okay because we do not expect the wallet to actually 'have' keys outside of the tracked window.

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

KtorZ · 2021-10-01T17:00:18Z

TODO:

More test scenarios to cover additional use-cases (see end of e2e spec file, some bullet points)
Support for extra required signatures which is a special field introduced in Alonzo that may contain extra signatures. Those keys need to be resolved somehow but it's unclear how (the wallet doesn't keep track of keys per se, but of addresses...)
Challenge / Discuss the current choice of response for the sign-transaction API. I believe that the current response is redundant and adds extra work for little benefits? (see commit notes)

cc @sevanspowell

(NB: the second point may be a known limitation we accept for now in order to get things unlocked a bit).

piotr-iohk · 2021-10-04T11:41:39Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+    -- - Signing with pre-existing witnesses
+    -- - Signing with collaterals
+    -- - Signing with extra required signatures
+    -- - ideas?


There are actually several cases in this file that were created only up to "Construct" that, I guess, can be now filled with sign and submit steps. They all have relevant comment:

TODO: now we should sign it and send it in two steps

or

TODO: sign

piotr-iohk · 2021-10-04T11:44:09Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                ]
+        r <- request @ApiTxId ctx submitEndpoint headers (NonJson $ BL.fromStrict sealedTx)
+        verify r
+            [ expectResponseCode HTTP.status500


Perhaps it would be nice to "catch" 500 and have some 40x here?

We thought the same, though not really relevant to this PR but I think the submitEndpoint should not return 500 errors for invalid transactions, since this is precisely a client error.

I've issued bug ticket for this ADP-1172.

KtorZ · 2021-10-04T17:22:14Z

lib/core/test/unit/Cardano/WalletSpec.hs

+                (xprv, Passphrase pwd) <- keystore addr
+                let sigData = tx ^. #txId . #getHash
+                let sig = CC.unXSignature $ CC.sign pwd (getKey xprv) sigData
+                return $ xpubToBytes (getKey $ publicKey xprv) <> sig


Not exactly relevant to this PR, but necessary because of the change of semantic when signing / the removal of the ErrKeyNotFoundForAddress

KtorZ · 2021-10-04T17:23:43Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+        let bytes = unsafeFromHex "84a70081825820410a9cd4af08b3abe25c2d3b87af4c23d0bb2fb7577b639d5cfbdfe13a4a696c0c0d80018182583901059f0c7b9899793d2c9afaeff4fd09bedd9df3b8cb1b9c301ab8e0f7fb3c13a29d3798f1b77b47f2ddb31c19326b87ed6f71fb9a27133ad51b000001001d19d714021a000220ec03198d0f05a1581de1fb3c13a29d3798f1b77b47f2ddb31c19326b87ed6f71fb9a27133ad51b000000e8d4a510000e80a0f5f6"
+        let sealedTx = sealedTxFromBytes bytes
+        sealedTx `shouldSatisfy` isRight
+


We stumbled upon this while testing, and got surprised by the result so we made it a golden test. In the end, the roundtrip test below is actually pretty weak as it doesn't test most constituents of transactions. In particular, withdrawals aren't tested at all by this property 😬

@KtorZ

2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk > This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :) This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction. Co-authored-by: Arnaud Bailly <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]>

@KtorZ

2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk > This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :) This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction. Co-authored-by: Arnaud Bailly <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]>

@KtorZ

2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk > This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :) This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction. Co-authored-by: Arnaud Bailly <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]>

piotr-iohk · 2021-10-05T11:03:54Z

bors r+

2943: ADP-919: Sign Transactions Endpoint r=piotr-iohk a=KtorZ - 📍 **Remove withdrawal from the sign-transaction API.** Withdrawals are 'needed' in this context when they refer to an external wallet (i.e. via a mnemonic phrase) so that, the required signature for such withdrawal can be produced (derived from the root key associated with the mnemonic). However: (a) The use-case for such external withdrawals stem from the ITN rewards, which were distributed more than 14 months ago to Shelley wallets that had participated in the incentivized testnet. It is reasonable to think that most concerned users have already redeemed their rewards. Those who haven't can still do it via the old transaction endpoint. (b) Having withdrawal as part of this API endpoint is awkward and confusing. Instead, it would be more logical / practical to perform the signature during the construction of the transaction; that is, when constructing a transaction with an external withdrawal, the result would already contain a signature from the associated mnemonic. Such transaction would then still need to be passed to the sign-transaction endpoint to gather signature from the wallet itself. Alternatively, not supporting this feature through the new transaction construction endpoint is also a very sensible choice. Regardless of the option, worrying about it _now_ is unnecessary / a waste of time. - 📍 **Extract vk signing from 'mkTx'** Note that, doing so I have slightly altered the semantic of the function which can no longer return a 'ErrKeyNotFoundForAddress'. Not finding the relevant key for a given input will now simply skip that input. Ultimately, the code used to yield to a 500 server errors BUT, that case was truly unreachable. Proof is that there's no end-to-end test which happen to be looking for that error case since it cannot (must not) occur under any circumstances. It really is a deep server error. This change does not however makes the error 'silent', but merely defers it to the submission. If a necessary signature is missing, the transaction will eventually be rejected by the local node which will inform the user about the missing key. The rationale for changing the semantic is that, in the context of signing 'external' transactions, we want to allow _other_ inputs than ours. This is crucial in the context of Alonzo since transactions may contain and consume script inputs. So it's important for this operation to skip such inputs and simply care about signing inputs relevant of the wallet. Note that, as a TODO, we should also now check for extra required signatures which can be specified in the transaction body. - 📍 **Extend transaction layer with 'addVkWitnesses' and use it.** - 📍 **Move withdrawal signing into 'signTransaction'** So that we avoid too many repetition in the upcoming implementation of addVkWitnesses, and also, centralize the creation of signatures into one place. Note that this does not change the semantic of the function and it works regardless of where the reward account comes from (external or internal). - 📍 **Implement 'addVkWitnesses' for the shelley target.** A bit of repetition here because of the encapsulated era which can't leave its scope. All-in-all, it's not a big deal and someone has to deal with the multiple eras into some place, might it be via a type-class or via similar branches in a case like here. Note also that in case of Byron transactions, this does not work, which is likely a fair limitation. - 📍 **Add basic end-to-end test for signing** The test does a full cycle: construction > signing > submission. However, some elements of the signing response are currently stubbed out. Obtaining them (the serialized body and witnesses) is _doable_ but also look like extra / needless work :thinking_face: ? I'd argue that returning both the full serialized tx AND the serialized body AND the serialized witnesses is redundant. It seems like the interface currently is the serialized transaction (also with the construction endpoint) so it'd make more sense to only return this. To be discussed. - 📍 **Fix unit test after changing semantic of transaction construction** This no longer fails on unknown inputs, but skip the input. - 📍 **Add test rejecting unsigned Tx** - 📍 **Add new test for signing withdrawals** - 📍 **Fix compilation error on withdrawal test.** - 📍 **Add counter-example serialized transaction.** - 📍 **Add unit test for failed CBOR decoding of tx w/ withdrawal** This unit test comes from failed integration test for signing transactions. - 📍 **Decode transaction bytes from base16 before deserialising them 🤦.** - 📍 **Rely on 'SealedTx' JSON instance for serialization (Base64)** And do not provide raw bytes which are automatically serialized as Base16 - 📍 **Factor out submission code from all new-tx sign tests.** - 📍 **Remove redundant response elements from sign transaction result.** The rationale is that, these elements are already present in the serialized transaction and, we assume that a client able to consume parts of the serialized transaction and put them together manually into a full serialized transaction would know anyway how to handle CBOR in the first place. Thus, splitting the full serialized transactions into smaller part if left as a client concern. - 📍 **Add e2e test scenario illustrating requiredSigners for transactions** This currently fails, as expected, since the feature isn't yet implemented in the API handler. - 📍 **Include signatures for 'extra-key-witnesses' in transactions** Note however that the wallet will only include signatures for keys it knows of, that is, for keys that are within the current visible window of the sequential state. That is okay because we do not expect the wallet to actually 'have' keys outside of the tracked window. Co-authored-by: KtorZ <[email protected]> Co-authored-by: Arnaud Bailly <[email protected]> Co-authored-by: IOHK <[email protected]>

iohk-bors · 2021-10-05T11:36:10Z

Build failed:

buildkite/cardano-wallet

@KtorZ

2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk > This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :) This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction. Co-authored-by: Arnaud Bailly <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]>

KtorZ · 2021-10-05T12:58:21Z

bors r+

iohk-bors · 2021-10-05T12:59:50Z

Canceled.

KtorZ · 2021-10-05T13:07:42Z

bors r+

iohk-bors · 2021-10-05T13:47:55Z

Merge conflict.

KtorZ · 2021-10-05T13:50:30Z

bors r+

KtorZ · 2021-10-05T13:51:12Z

bors r-

Withdrawals are 'needed' in this context when they refer to an external wallet (i.e. via a mnemonic phrase) so that, the required signature for such withdrawal can be produced (derived from the root key associated with the mnemonic). However: (a) The use-case for such external withdrawals stem from the ITN rewards, which were distributed more than 14 months ago to Shelley wallets that had participated in the incentivized testnet. It is reasonable to think that most concerned users have already redeemed their rewards. Those who haven't can still do it via the old transaction endpoint. (b) Having withdrawal as part of this API endpoint is awkward and confusing. Instead, it would be more logical / practical to perform the signature during the construction of the transaction; that is, when constructing a transaction with an external withdrawal, the result would already contain a signature from the associated mnemonic. Such transaction would then still need to be passed to the sign-transaction endpoint to gather signature from the wallet itself. Alternatively, not supporting this feature through the new transaction construction endpoint is also a very sensible choice. Regardless of the option, worrying about it _now_ is unnecessary / a waste of time.

Note that, doing so I have slightly altered the semantic of the function which can no longer return a 'ErrKeyNotFoundForAddress'. Not finding the relevant key for a given input will now simply skip that input. Ultimately, the code used to yield to a 500 server errors BUT, that case was truly unreachable. Proof is that there's no end-to-end test which happen to be looking for that error case since it cannot (must not) occur under any circumstances. It really is a deep server error. This change does not however makes the error 'silent', but merely defers it to the submission. If a necessary signature is missing, the transaction will eventually be rejected by the local node which will inform the user about the missing key. The rationale for changing the semantic is that, in the context of signing 'external' transactions, we want to allow _other_ inputs than ours. This is crucial in the context of Alonzo since transactions may contain and consume script inputs. So it's important for this operation to skip such inputs and simply care about signing inputs relevant of the wallet. Note that, as a TODO, we should also now check for extra required signatures which can be specified in the transaction body.

So that we avoid too many repetition in the upcoming implementation of addVkWitnesses, and also, centralize the creation of signatures into one place. Note that this does not change the semantic of the function and it works regardless of where the reward account comes from (external or internal).

A bit of repetition here because of the encapsulated era which can't leave its scope. All-in-all, it's not a big deal and someone has to deal with the multiple eras into some place, might it be via a type-class or via similar branches in a case like here. Note also that in case of Byron transactions, this does not work, which is likely a fair limitation.

The test does a full cycle: construction > signing > submission. However, some elements of the signing response are currently stubbed out. Obtaining them (the serialized body and witnesses) is _doable_ but also look like extra / needless work :thinking_face: ? I'd argue that returning both the full serialized tx AND the serialized body AND the serialized witnesses is redundant. It seems like the interface currently is the serialized transaction (also with the construction endpoint) so it'd make more sense to only return this. To be discussed.

This no longer fails on unknown inputs, but skip the input.

This unit test comes from failed integration test for signing transactions.

And do not provide raw bytes which are automatically serialized as Base16

The rationale is that, these elements are already present in the serialized transaction and, we assume that a client able to consume parts of the serialized transaction and put them together manually into a full serialized transaction would know anyway how to handle CBOR in the first place. Thus, splitting the full serialized transactions into smaller part if left as a client concern.

This currently fails, as expected, since the feature isn't yet implemented in the API handler.

Note however that the wallet will only include signatures for keys it knows of, that is, for keys that are within the current visible window of the sequential state. That is okay because we do not expect the wallet to actually 'have' keys outside of the tracked window.

KtorZ · 2021-10-05T16:53:57Z

bors +

pretty please 🙏

iohk-bors · 2021-10-05T16:53:59Z

Did you mean "r+"?

KtorZ · 2021-10-05T16:54:12Z

bors r+

KtorZ · 2021-10-05T16:54:19Z

I didn't miss you.

iohk-bors · 2021-10-05T17:40:16Z

Build succeeded:

KtorZ requested review from sevanspowell and piotr-iohk October 1, 2021 16:55

KtorZ self-assigned this Oct 1, 2021

KtorZ mentioned this pull request Oct 1, 2021

Implement signTransaction #2754

Closed

10 tasks

KtorZ commented Oct 1, 2021

View reviewed changes

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

KtorZ force-pushed the KtorZ/ADP-919/sign-transaction branch 2 times, most recently from 356a34e to 5118cf4 Compare October 4, 2021 09:46

piotr-iohk reviewed Oct 4, 2021

View reviewed changes

ghost mentioned this pull request Oct 4, 2021

Lenient SealedTx decoding #2946

Merged

KtorZ commented Oct 4, 2021

View reviewed changes

sevanspowell approved these changes Oct 5, 2021

View reviewed changes

piotr-iohk approved these changes Oct 5, 2021

View reviewed changes

KtorZ force-pushed the KtorZ/ADP-919/sign-transaction branch from 9becd53 to c1606fb Compare October 5, 2021 12:58

KtorZ force-pushed the KtorZ/ADP-919/sign-transaction branch from b2b3c22 to 33eafc5 Compare October 5, 2021 13:50

KtorZ and others added 17 commits October 5, 2021 18:12

Extend transaction layer with 'addVkWitnesses' and use it.

8dffa98

Fix unit test after changing semantic of transaction construction

416869c

This no longer fails on unknown inputs, but skip the input.

Add test rejecting unsigned Tx

c1a59fe

Add new test for signing withdrawals

f190dff

Fix compilation error on withdrawal test.

ad0adff

Add counter-example serialized transaction.

a88e3d7

Add unit test for failed CBOR decoding of tx w/ withdrawal

0b1bdca

This unit test comes from failed integration test for signing transactions.

Decode transaction bytes from base16 before deserialising them 🤦.

f2cb02a

Rely on 'SealedTx' JSON instance for serialization (Base64)

49c24ad

And do not provide raw bytes which are automatically serialized as Base16

Factor out submission code from all new-tx sign tests.

effbd15

Add e2e test scenario illustrating requiredSigners for transactions

ca6c2a6

This currently fails, as expected, since the feature isn't yet implemented in the API handler.

KtorZ force-pushed the KtorZ/ADP-919/sign-transaction branch from ea30fc0 to 60b8075 Compare October 5, 2021 16:14

KtorZ and others added 3 commits October 5, 2021 18:53

Remove faulty 'chevron' in docstring causing haddock to choke hard.

9133a2d

Regenerate nix

fc0114c

KtorZ force-pushed the KtorZ/ADP-919/sign-transaction branch from 60b8075 to fc0114c Compare October 5, 2021 16:53

iohk-bors bot merged commit 807980f into master Oct 5, 2021

iohk-bors bot deleted the KtorZ/ADP-919/sign-transaction branch October 5, 2021 17:40

WilliamKingNoel-Bot pushed a commit that referenced this pull request Oct 5, 2021

docs: Merge #2943

b0c52dd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADP-919: Sign Transactions Endpoint #2943

ADP-919: Sign Transactions Endpoint #2943

KtorZ commented Oct 1, 2021 •

edited

Loading

KtorZ commented Oct 1, 2021 •

edited

Loading

piotr-iohk Oct 4, 2021

piotr-iohk Oct 4, 2021

KtorZ Oct 4, 2021

piotr-iohk Oct 4, 2021

KtorZ Oct 4, 2021

KtorZ Oct 4, 2021

piotr-iohk commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

KtorZ commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

ADP-919: Sign Transactions Endpoint #2943

ADP-919: Sign Transactions Endpoint #2943

Conversation

KtorZ commented Oct 1, 2021 • edited Loading

KtorZ commented Oct 1, 2021 • edited Loading

piotr-iohk Oct 4, 2021

Choose a reason for hiding this comment

piotr-iohk Oct 4, 2021

Choose a reason for hiding this comment

KtorZ Oct 4, 2021

Choose a reason for hiding this comment

piotr-iohk Oct 4, 2021

Choose a reason for hiding this comment

KtorZ Oct 4, 2021

Choose a reason for hiding this comment

KtorZ Oct 4, 2021

Choose a reason for hiding this comment

piotr-iohk commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

KtorZ commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 5, 2021

KtorZ commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

KtorZ commented Oct 1, 2021 •

edited

Loading

KtorZ commented Oct 1, 2021 •

edited

Loading