Make sign tx testable #3022

sevanspowell · 2021-11-16T04:39:58Z

This PR is primarily about improving the testability of signTransaction, and providing a number of property tests.

In the signTransaction code path, pushed IO further to the edge of the program, enabling us to unit test more of the logic of signTransaction.
Provided a number of property tests to prove key points of signTransaction (it's witness adding behaviour)
- Also, specifically added a test to assert that signTransaction maintains the script integrity hash of a transaction, for ADP-1047.
Expanded the transaction generator slightly to possibly generate ScriptWitnesses for TxIns.

Comments

Issue Number

ADP-1047 / ADP-1139

rvl · 2021-11-24T09:07:04Z

@sevanspowell Could we please have a title and description for this PR?

paweljakubas · 2021-12-20T08:50:09Z

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

-        liftHandler $ W.signTransaction wrk wid pwd sealedTx
+    sealedTx' <- withWorkerCtx ctx wid liftE liftE $ \wrk -> liftHandler $ do
+        let
+            db = wrk ^. W.dbLayer @IO @s @k


We have those parts in almost all places in where. Shouldn't we have here also (just for consistency)?

It won't work in this case, because wrk is not an argument to the outer function, wrk won't be in scope in the where block.

paweljakubas · 2021-12-20T08:51:37Z

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

 import qualified Cardano.Api.Shelley as Cardano
+import qualified Cardano.Crypto.Hash.Blake2b as Blake2b


what about using here as Crypto ?

or just import without qualification like in other places in codebase?

paweljakubas · 2021-12-20T08:55:27Z

lib/core/src/Cardano/Wallet.hs

+  -- necessary
+signTransaction tl era keyLookup (rootKey, rootPwd) utxo =
+    let
+        rewardAcnt :: (XPrv, Passphrase "encryption")


what about exposing this function? It is used also in tests. Maybe it would be good to have code reuse

paweljakubas

very nice properties added. It would be great to continue this path of testing when adding test coverage of decodeTx (in some forthcoming PRs).

We see that signing can add some bits and this is tested.What about properties showing that signing will not change inps, outs, validity, ...., some things' ordering already present in the body?
I see common patterns in properties. For example, we have the same structure:

case Cardano.THINGSupportedInEra era of
    Nothing -> 
    Just _ ->

I wonder if there is a way to extract that common structure and reuse it in all property tests?

sevanspowell · 2021-12-21T04:06:58Z

very nice properties added. It would be great to continue this path of testing when adding test coverage of decodeTx (in some forthcoming PRs).
1. We see that signing can add some bits and this is tested.What about properties showing that signing will not change inps, outs, validity, ...., some things' ordering already present in the body?

2. I see common patterns in properties. For example,  we have the same structure:
case Cardano.THINGSupportedInEra era of
    Nothing -> 
    Just _ -> 
I wonder if there is a way to extract that common structure and reuse it in all property tests?

Thanks for the feedback @paweljakubas! I've addressed a number of your points in subsequent commits.

rvl

Excellent work - these are really good tests to have! ⭐

rvl · 2021-12-23T02:37:59Z

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

+    forAll (genTxInEra era) $ \tx -> do
+        let


May as well combine the let and where bindings at the deeper level...

Suggested change

forAll (genTxInEra era) $ \tx -> do

let

forAll (genTxInEra era) $ \tx -> bodyContentBefore == bodyContentAfter

where

tl = testTxLayer

(same goes for the other prop_ functions)

rvl · 2021-12-23T02:44:56Z

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

+    conjoin $ (flip foldMap) elems $ \x ->
+        [ x `elem` xs
+        & counterexample ("actual set: " <> show xs)
+        & counterexample ("expected elem: " <> show x)
+        ]


I don't think the foldMap is needed?

Suggested change

conjoin $ (flip foldMap) elems $ \x ->

[ x `elem` xs

& counterexample ("actual set: " <> show xs)

& counterexample ("expected elem: " <> show x)

]

counterexample ("actual set: " <> show xs) $ conjoin

[ x `elem` xs & counterexample ("expected elem: " <> show x)

| x <- elems ]

rvl · 2021-12-23T02:52:19Z

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

+        forAllEras (\era -> it ("signTransaction adds reward account witness when necessary (" <> show era <> ")") $
+            property (prop_signTransaction_addsRewardAccountKey era))


I was thinking that it would be nice to have the property description/right beside the property definition, but still exposing the Property as a standalone function. Something like this...

Suggested change

forAllEras (\era -> it ("signTransaction adds reward account witness when necessary (" <> show era <> ")") $

property (prop_signTransaction_addsRewardAccountKey era))

describe desc_signTransaction_addsRewardAccountKey $

forAllEras $ \era -> it (show era) $

property (prop_signTransaction_addsRewardAccountKey era))

-- ... --

desc_signTransaction_addsRewardAccountKey = "signTransaction adds reward account witness when necessary" :: String

Agree, could be cool. But I wouldn't want these to be the only tests that do it.

rvl · 2021-12-23T02:58:43Z

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

+            rootK = first liftRawKey rootXPrv
+
+            rawRewardK :: (XPrv, Passphrase "encryption")
+            rawRewardK = (getRawKey $ deriveRewardAccount (snd rootK) (fst rootK), snd rootK)


If we find that tests are taking too long to run due to key generation, we could shift these things up into a precooked record of key inputs.

rvl · 2021-12-23T03:14:09Z

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

+                                [mkShelleyWitness txBody rawRewardK]
+
+            checkCoverage $
+                expectedWits `checkSubsetOf` (getSealedTxWitnesses sealedTx')


Cool. I was wondering whether the reward account xpub could be used to verify new witnesses, but it seems easier and more effective to just generate the witness directly, as you have done.

- For signTransaction: - Move IO operations out of the wallet layer and into the server handler. We want to move IO operations to the edge of our program, in this case, the "edge" is our server handler. Doing so makes the wallet layer easier to test.

- This update allows the spending witness for a TxIn to be a script witness or a key witness. Previously only key witnesses were supported.

- Add the following property tests for signTransaction: - adds the reward account witness when necessary - adds extra key witnesses when necessary - adds tx in witnesses when necessary - adds collateral witnesses when necessary - never removes witnesses

- Add tests to ensure signTransaction doesn't modify TxBody. - Make "checkSubsetOf" test more concise and reusable.

sevanspowell · 2021-12-24T00:55:51Z

bors try

iohk-bors · 2021-12-24T01:30:55Z

try

Build succeeded:

sevanspowell · 2021-12-24T01:58:10Z

bors r+

sevanspowell · 2021-12-24T01:58:20Z

bors cancel

iohk-bors · 2021-12-24T01:58:21Z

Canceled.

sevanspowell · 2021-12-24T01:58:56Z

bors r+

iohk-bors · 2021-12-24T02:41:09Z

Build succeeded:

sevanspowell force-pushed the feature/adp-1139/make-signTx-testable branch 2 times, most recently from 685aa39 to f284cf8 Compare November 19, 2021 07:46

sevanspowell changed the title ~~Feature/adp 1139/make sign tx testable~~ Make sign tx testable Dec 14, 2021

sevanspowell force-pushed the feature/adp-1139/make-signTx-testable branch 3 times, most recently from 2c557b0 to d4b02ee Compare December 20, 2021 04:59

sevanspowell requested review from rvl and paweljakubas December 20, 2021 05:04

sevanspowell self-assigned this Dec 20, 2021

sevanspowell marked this pull request as ready for review December 20, 2021 05:04

sevanspowell force-pushed the feature/adp-1139/make-signTx-testable branch from d4b02ee to 5870069 Compare December 20, 2021 05:04

paweljakubas reviewed Dec 20, 2021

View reviewed changes

paweljakubas approved these changes Dec 20, 2021

View reviewed changes

rvl approved these changes Dec 23, 2021

View reviewed changes

sevanspowell added 9 commits December 23, 2021 17:42

Expand Tx generator to create script witnesses for TxIns

2a72ca0

- This update allows the spending witness for a TxIn to be a script witness or a key witness. Previously only key witnesses were supported.

Check script integrity hash

e313511

Extract common "feature supported in era" code

e76fc68

Import as Crypto for consistency

7482e74

Ensure signTransaction doesn't change TxBody at all

2baaf02

- Add tests to ensure signTransaction doesn't modify TxBody. - Make "checkSubsetOf" test more concise and reusable.

Combine where and let bindings

825e192

Remove unnecessary foldMap

5351ce1

sevanspowell force-pushed the feature/adp-1139/make-signTx-testable branch from bc22af0 to 5351ce1 Compare December 23, 2021 09:42

iohk-bors bot added a commit that referenced this pull request Dec 24, 2021

Try #3022:

9501ef5

iohk-bors bot merged commit 2cb11cc into master Dec 24, 2021

iohk-bors bot deleted the feature/adp-1139/make-signTx-testable branch December 24, 2021 02:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sign tx testable #3022

Make sign tx testable #3022

sevanspowell commented Nov 16, 2021 •

edited

Loading

rvl commented Nov 24, 2021

paweljakubas Dec 20, 2021

sevanspowell Dec 21, 2021

paweljakubas Dec 20, 2021

paweljakubas Dec 20, 2021

paweljakubas Dec 20, 2021

paweljakubas left a comment •

edited

Loading

sevanspowell commented Dec 21, 2021

rvl left a comment

rvl Dec 23, 2021

rvl Dec 23, 2021

rvl Dec 23, 2021

sevanspowell Dec 23, 2021

rvl Dec 23, 2021

rvl Dec 23, 2021

sevanspowell commented Dec 24, 2021

iohk-bors bot commented Dec 24, 2021

sevanspowell commented Dec 24, 2021

sevanspowell commented Dec 24, 2021

iohk-bors bot commented Dec 24, 2021

sevanspowell commented Dec 24, 2021

iohk-bors bot commented Dec 24, 2021

		import qualified Cardano.Api.Shelley as Cardano
		import qualified Cardano.Crypto.Hash.Blake2b as Blake2b

-    forAll (genTxInEra era) $ \tx -> do
-        let
+    forAll (genTxInEra era) $ \tx -> bodyContentBefore == bodyContentAfter
+        where
+            tl = testTxLayer

		forAllEras (\era -> it ("signTransaction adds reward account witness when necessary (" <> show era <> ")") $
		property (prop_signTransaction_addsRewardAccountKey era))

Make sign tx testable #3022

Make sign tx testable #3022

Conversation

sevanspowell commented Nov 16, 2021 • edited Loading

Comments

Issue Number

rvl commented Nov 24, 2021

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 • edited Loading

Choose a reason for hiding this comment

sevanspowell commented Dec 21, 2021

rvl 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

sevanspowell commented Dec 24, 2021

iohk-bors bot commented Dec 24, 2021

try

sevanspowell commented Dec 24, 2021

sevanspowell commented Dec 24, 2021

iohk-bors bot commented Dec 24, 2021

sevanspowell commented Dec 24, 2021

iohk-bors bot commented Dec 24, 2021

sevanspowell commented Nov 16, 2021 •

edited

Loading

paweljakubas left a comment •

edited

Loading