Support reference scripts #4070

paweljakubas · 2023-07-28T11:08:48Z

I have ...

Comments

Issue Number

jonathanknowles

Some early comments!

I propose to revise our generators of arbitrary TxOut values so that we're not always generating values of Nothing for the script field. I suspect that if we do this, that several property tests will fail. (See comments.)

jonathanknowles · 2023-07-31T06:50:13Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut.hs

@@ -57,6 +63,9 @@ import GHC.Generics
 import qualified Cardano.Wallet.Primitive.Types.Coin as Coin
 import qualified Cardano.Wallet.Primitive.Types.TokenBundle as TokenBundle
 import qualified Cardano.Wallet.Primitive.Types.TokenMap as TokenMap
+import qualified Data.Text as T
+import qualified Text.ParserCombinators.ReadP as Parse
+


Suggested change

jonathanknowles · 2023-07-31T07:05:47Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut.hs

@@ -84,7 +95,7 @@ data TxOut = TxOut
 instance Ord TxOut where
    compare = comparing projection
      where
-        projection (TxOut address bundle) = (address, Lexicographic bundle)
+        projection (TxOut address bundle _) = (address, Lexicographic bundle)


Hi @paweljakubas

I think this Ord instance is not lawful w.r.t. the antisymmetry law:

(x <= y && y <= x) ==> (x == y)

Here's a counterexample:

>>> s1 == s2 False >>> TxOut a b s1 <= TxOut a b s2 True >>> TxOut a b s1 >= TxOut a b s2 True >>> TxOut a b s1 == TxOut a b s2 False 💥

To fix this, we need to take all parts of a TxOut into account:

Suggested change

projection (TxOut address bundle _) = (address, Lexicographic bundle)

projection (TxOut address bundle script) =

( address

, Lexicographic bundle

, script

)

jonathanknowles · 2023-07-31T07:06:33Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut.hs

+          scriptToText :: Script KeyHash -> T.Text
+          scriptToText (RequireSignatureOf keyhash) =
+              keyHashToText keyhash
+          scriptToText (RequireAllOf contents) =
+              "all [" <>  T.intercalate "," (map scriptToText contents) <> "]"
+          scriptToText (RequireAnyOf contents) =
+              "any [" <>  T.intercalate "," (map scriptToText contents) <> "]"
+          scriptToText (RequireSomeOf m contents) =
+              "at_least "<> T.pack (show m) <>
+              " [" <>  T.intercalate "," (map scriptToText contents) <> "]"
+          scriptToText (ActiveFromSlot s) =
+              "active_from " <> T.pack (show s)
+          scriptToText (ActiveUntilSlot s) =
+              "active_until " <> T.pack (show s)


Suggested change

scriptToText :: Script KeyHash -> T.Text

scriptToText (RequireSignatureOf keyhash) =

keyHashToText keyhash

scriptToText (RequireAllOf contents) =

"all [" <> T.intercalate "," (map scriptToText contents) <> "]"

scriptToText (RequireAnyOf contents) =

"any [" <> T.intercalate "," (map scriptToText contents) <> "]"

scriptToText (RequireSomeOf m contents) =

"at_least "<> T.pack (show m) <>

" [" <> T.intercalate "," (map scriptToText contents) <> "]"

scriptToText (ActiveFromSlot s) =

"active_from " <> T.pack (show s)

scriptToText (ActiveUntilSlot s) =

"active_until " <> T.pack (show s)

scriptToText :: Script KeyHash -> T.Text

scriptToText (RequireSignatureOf keyhash) =

keyHashToText keyhash

scriptToText (RequireAllOf contents) =

"all [" <> T.intercalate "," (map scriptToText contents) <> "]"

scriptToText (RequireAnyOf contents) =

"any [" <> T.intercalate "," (map scriptToText contents) <> "]"

scriptToText (RequireSomeOf m contents) =

"at_least "<> T.pack (show m) <>

" [" <> T.intercalate "," (map scriptToText contents) <> "]"

scriptToText (ActiveFromSlot s) =

"active_from " <> T.pack (show s)

scriptToText (ActiveUntilSlot s) =

"active_until " <> T.pack (show s)

jonathanknowles · 2023-07-31T07:15:05Z

lib/balance-tx/lib/Cardano/Tx/Balance/Internal/CoinSelection.hs


 toInternalUTxO' :: (TokenBundle -> b) -> (TxIn, TxOut) -> (WalletUTxO, b)
-toInternalUTxO' f (i, TxOut a b) = (WalletUTxO i a, f b)
+toInternalUTxO' f (i, TxOut a b _) = (WalletUTxO i a, f b)


I suspect this will break the following round-trip property:

cardano-wallet/lib/balance-tx/test/spec/Cardano/Tx/Balance/Internal/CoinSelectionSpec.hs

Lines 87 to 89 in 7181feb

prop_toInternalUTxO_toExternalUTxO :: TxIn -> TxOut -> Property

prop_toInternalUTxO_toExternalUTxO i o =

(toExternalUTxO . toInternalUTxO) (i, o) === (i, o)

However, our current property tests will not discover this breakage, as this PR adjusts the generators for TxOut so that they always generate Nothing for the script field.

We should definitely adjust the generator for TxOut used by Cardano.Tx.Balance.Internal.CoinSelectionSpec so that it generates a wider range of values for the script field: i.e., not just Nothing.

To fix this round-trip property, I'm guessing we will probably need to add a script field to the WalletUTxO type (above).

jonathanknowles · 2023-07-31T07:18:01Z

lib/balance-tx/test/spec/Cardano/Tx/Balance/Internal/CoinSelectionSpec.hs

@@ -123,7 +123,7 @@ genSelection = Selection
    genAssetsToBurn = genTokenMap
    genExtraCoinSource = genCoin
    genExtraCoinSink = genCoin
-    genTxOutCoin = TxOut <$> genAddress <*> (TokenBundle.fromCoin <$> genCoin)
+    genTxOutCoin = TxOut <$> genAddress <*> (TokenBundle.fromCoin <$> genCoin) <*> pure Nothing


I think we should generate a wider range of values here, and not just Nothing. In particular, always specifying Nothing hides the fact that this property no longer holds:

cardano-wallet/lib/balance-tx/test/spec/Cardano/Tx/Balance/Internal/CoinSelectionSpec.hs

Lines 87 to 89 in 7181feb

prop_toInternalUTxO_toExternalUTxO :: TxIn -> TxOut -> Property

prop_toInternalUTxO_toExternalUTxO i o =

(toExternalUTxO . toInternalUTxO) (i, o) === (i, o)

jonathanknowles · 2023-07-31T07:22:01Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut.hs

+instance {-# OVERLAPPING #-} Show (Maybe (Script KeyHash)) where
+    show scriptM = case scriptM of
+        Nothing -> "not present"
+        Just script -> T.unpack . scriptToText $ script


Hmm. Is it actually necessary to define an overlapping instance here? 🤔

Could we instead define a Show instance for Script KeyHash (in the module that defines Script), and then just rely upon the default Show instance for Maybe?

jonathanknowles · 2023-07-31T07:23:10Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut/Gen.hs

@@ -54,11 +54,16 @@ genTxOut :: Gen TxOut
 genTxOut = TxOut
    <$> genAddress
    <*> genTokenBundleSmallRange `suchThat` tokenBundleHasNonZeroCoin
+    <*> pure Nothing


I think we should generate a wider range of values here than just Nothing.

Are there already generators for the Script type, and if so, can we import one of them and use it here?

jonathanknowles · 2023-07-31T07:24:10Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut/Gen.hs


 shrinkTxOut :: TxOut -> [TxOut]
-shrinkTxOut (TxOut a b) = uncurry TxOut <$> shrinkInterleaved
+shrinkTxOut (TxOut a b c) =
+    map enrichPair $


Suggested change

map enrichPair $

enrichPair <$>

jonathanknowles · 2023-07-31T07:26:48Z

lib/wallet/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs

+            Scripts.RequireSignature $
+            Ledger.KeyHash $ Crypto.UnsafeHash $ toShort h


Suggested change

Scripts.RequireSignature $

Ledger.KeyHash $ Crypto.UnsafeHash $ toShort h

Scripts.RequireSignature $

Ledger.KeyHash $ Crypto.UnsafeHash $ toShort h

jonathanknowles · 2023-07-31T07:27:14Z

specifications/api/swagger.yaml

+        reference_policy_script_template:
+          <<: *ScriptTemplateValue
+          description: |
+            Optional policy script template that could be used as script reference


Suggested change

Optional policy script template that could be used as script reference

Optional policy script template that could be used as a script reference

jonathanknowles · 2023-07-31T07:37:18Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Tx/TxOut.hs

@@ -67,6 +76,8 @@ data TxOut = TxOut
        :: !Address
    , tokens
        :: !TokenBundle
+    , script
+        :: !(Maybe (Script KeyHash))


Perhaps we can use the StrictMaybe type here instead?

HeinrichApfelmus · 2023-08-02T12:17:13Z

I propose to revise our generators of arbitrary TxOut values so that we're not always generating values of Nothing for the script field. I suspect that if we do this, that several property tests will fail. (See comments.)

Actually, I asked Pawel to not change the TxOut type. 😅 We want to get rid of the TxOut it — and use a type from the Write hierarchy for this feature instead.

paweljakubas · 2023-08-22T10:05:29Z

closing in favor of #4086

paweljakubas added 8 commits July 27, 2023 09:10

update swagger by adding reference_policy_script_template

1ec835d

extending ApiConstructTransactionData

517c859

parseReferenceScript part 1

1f21bca

parseReferenceScript part 2

854f4db

compute referenceScript from template

a3569df

TxOut extension - part 1

77f9700

TxOut extension - part 2

31396a0

deal with ledger conversions

de7b9b1

jonathanknowles reviewed Jul 31, 2023

View reviewed changes

paweljakubas self-assigned this Aug 9, 2023

paweljakubas closed this Aug 22, 2023

paweljakubas deleted the paweljakubas/adp-3090/support-reference-scripts branch August 22, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support reference scripts #4070

Support reference scripts #4070

paweljakubas commented Jul 28, 2023

jonathanknowles left a comment •

edited

Loading

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023 •

edited

Loading

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023 •

edited

Loading

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023

jonathanknowles Jul 31, 2023

HeinrichApfelmus commented Aug 2, 2023

paweljakubas commented Aug 22, 2023

	prop_toInternalUTxO_toExternalUTxO :: TxIn -> TxOut -> Property
	prop_toInternalUTxO_toExternalUTxO i o =
	(toExternalUTxO . toInternalUTxO) (i, o) === (i, o)

		Scripts.RequireSignature $
		Ledger.KeyHash $ Crypto.UnsafeHash $ toShort h

	Optional policy script template that could be used as script reference
	Optional policy script template that could be used as a script reference

Support reference scripts #4070

Support reference scripts #4070

Conversation

paweljakubas commented Jul 28, 2023

Comments

Issue Number

jonathanknowles 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

jonathanknowles Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles Jul 31, 2023 • 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

HeinrichApfelmus commented Aug 2, 2023

paweljakubas commented Aug 22, 2023

jonathanknowles left a comment •

edited

Loading

jonathanknowles Jul 31, 2023 •

edited

Loading

jonathanknowles Jul 31, 2023 •

edited

Loading