to/from json instances for script #84

paweljakubas · 2020-10-14T19:02:05Z

https://jira.iohk.io/browse/ADP-474

"e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a" 
{ "all" : [ "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a"
          , "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735b"
          ]
}
{ "all" : [ "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a"
          , {"any": [ "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735b"
                    ,  "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735c"
                    ] 
            }
          ]
}
{ "all" : [  "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a"
          , {"some": { "from" :[ "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735b"
                      , "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735c"
                      , "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735d"
                      ] 
                        , "at_least" : 2
                     }
            }
          ]
}

ToJSON for Script
FromJSON for Script
testing/roundtrips
all hrp in line with CIP-5: script prefixes extension cardano-foundation/CIPs#31

rvl

Looks ok.

Is this schema specified anywhere? Do we need to have a discussion about the schema?

rvl · 2020-10-16T06:21:02Z

core/lib/Cardano/Script.hs

+        let keyHexed = T.decodeUtf8 $ encode EBase16 key
+        in Object ("key" .= keyHexed)


Suggested change

let keyHexed = T.decodeUtf8 $ encode EBase16 key

in Object ("key" .= keyHexed)

object ["key" .= T.decodeLatin1 (encode EBase16 key)]

done in 3c2bb9a

rvl · 2020-10-16T06:23:30Z

core/lib/Cardano/Script.hs

@@ -212,3 +220,68 @@ blake2b224 =
 -- Size, in bytes, of a hash of public key (without the corresponding chain code)
 hashSize :: Int
 hashSize = hashDigestSize Blake2b_224
+
+-- Examples of Script jsons:
+--{ "key" : "e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a" }


Rather than a json object, could this just be a string?
The label "key" is not quite right because it's a hex-encoded hash of a key.
And were we going to use bech32 for key hashes?

good idea, I was also thinking about it (ie. to express verification key hash as string). also in accordance to "line parser" added all, bech32, base16 and base58, support. see 3c2bb9a

rvl · 2020-10-16T06:28:32Z

core/lib/Cardano/Script.hs

+        mOfN <-
+            (withObject "script" $
+             \o -> o .:? "at_least" :: Json.Parser (Maybe Value)) obj
+        case (reqKey, reqAny, reqAll, mOfN) of


Try replacing this with applicative style <|>. E.g.

parseJSON v = parseKey v <|> parseAnyOf v <|> parseAllOf v <|> parseAtLeast v where parseAnyOf = withObject "Script AnyOf" \o -> RequireAnyOf <$> (o .: "any") -- etc

done 3c2bb9a

rvl · 2020-10-16T06:30:20Z

core/test/Cardano/ScriptSpec.hs

  where
    toHexText = T.decodeUtf8 . encode EBase16
    toHexText' (ScriptHash bytes) = toHexText bytes
+
+instance Arbitrary Script where
+    arbitrary = do


I can't remember ... with generators of recursive types, does the generator need to be made smaller for every step down the tree?

rvl · 2020-10-16T06:38:16Z

core/lib/Cardano/Script.hs

+import Codec.Binary.Encoding
+    ( AbstractEncoding (..), encode, fromBase16 )


I forgot to add that I don't really like module names like this.
It confuses me because it looks like the module comes from another package.
Given that it deals with bech32/base56, it is very much a module which should be called Cardano.Addresses.Encoding.

Also, this module Cardano.Script. There are multiple kinds of scripts in cardano. How about Cardano.Addresses.Script?

sound idea -> b899a28

rvl

Looks better. I have suggestions for the json schema. It would be good to get the json and bech32 HRPs are correct now so we don't need to fix it later.

rvl · 2020-10-21T01:17:41Z

core/lib/Cardano/Address/Script.hs

+            Left _ -> Left $ ErrorKeyHashFromTextInvalidString EBase16
+            Right bytes -> checkPayload bytes
+        Just EBech32{} -> fromBech32
+        Just EBase58 -> case fromBase58 (toBytes str) of


I don't think we should support base58 for key hashes because it will just add confusion. I would even be mildly in favour of only using bech32 and not even supporting hex. The thing I like about bech32 is the HRP - which base16 and base58 don't have. On the other hand, supporting hex is good for interoperability.

now support only to bech32 and base16 (I think some people are very attached to this format - like me;-)) -> 6a1ebd1

rvl · 2020-10-21T01:19:57Z

core/lib/Cardano/Address/Script.hs

+          parseKey = withText "Script KeyHash" $
+              either (fail . showErr) (pure . RequireSignatureOf) . keyHashFromText
+          parseAnyOf =
+              withObject "Script AnyOf" $ \o -> RequireAnyOf <$> (o .: "any" :: Json.Parser [Script])


Are the type annotations needed to make it build?

indeed - not needed - removed here 6d001e1

rvl · 2020-10-21T01:31:16Z

core/test/Cardano/Address/ScriptSpec.hs

+
+instance Arbitrary Script where
+    arbitrary = do
+        reqAllGen <- do
+            n <- choose (1,10)
+            pure $ RequireAllOf <$> vector n
+        reqAnyGen <- do
+            n <- choose (1,10)
+            pure $ RequireAnyOf <$> vector n
+        reqMofNGen <- do
+            m <- choose (2,5)
+            n <- choose ((fromInteger $ toInteger m),10)
+            pure $ RequireMOf m <$> vector n
+        let reqSig =
+                (RequireSignatureOf . KeyHash . BS.pack) <$> replicateM 28 arbitrary
+        oneof
+            (replicate 15 reqSig ++
+            [ reqAllGen
+            , reqAnyGen
+            , reqMofNGen
+            ])


I looked it up... We do need to be careful to avoid generators which fail to terminate, or produce very large results. Generating Recursive Data Types 🤔.

Also, can shrink = genericShrink work here?

Suggested change

instance Arbitrary Script where

arbitrary = do

reqAllGen <- do

n <- choose (1,10)

pure $ RequireAllOf <$> vector n

reqAnyGen <- do

n <- choose (1,10)

pure $ RequireAnyOf <$> vector n

reqMofNGen <- do

m <- choose (2,5)

n <- choose ((fromInteger $ toInteger m),10)

pure $ RequireMOf m <$> vector n

let reqSig =

(RequireSignatureOf . KeyHash . BS.pack) <$> replicateM 28 arbitrary

oneof

(replicate 15 reqSig ++

[ reqAllGen

, reqAnyGen

, reqMofNGen

])

instance Arbitrary Script where

arbitrary = sized script'

where

script' 0 = RequireSignatureOf . KeyHash . BS.pack <$> replicateM 28 arbitrary

script' n =

let scripts = listOf (arbitrary (script' (n `div` 2)))

in oneof

[ RequireAllOf <$> scripts

, RequireAnyOf <$> scripts

, RequireMOf <$> choose (2,5) <*> scripts

-- note: perhaps the last one is too invalid

]

instance Arbitrary Script where arbitrary = sized script' where script' 0 = RequireSignatureOf . KeyHash . BS.pack <$> replicateM 28 arbitrary script' n = let n' = n `div` 2 scripts = listOf (script' n') conds = [ RequireAllOf <$> scripts , RequireAnyOf <$> scripts ] in if n' >= 5 then oneof (conds ++ [RequireSomeOf <$> choose (2,5) <*> scripts]) else oneof conds

seems to stuck when running... on the other hand, for me, it is much less clear to read

OK I fixed it. I think it's important to learn how to generate tests properly and use best practices for quickcheck.

rvl · 2020-10-21T01:39:41Z

core/lib/Cardano/Address/Script.hs

+    toJSON (RequireMOf m content) =
+        let inside = Object ("from" .= fmap toJSON content <> "m" .= toJSON m)
+        in object ["at_least" .= inside]


Could we rename RequireMOf to RequireSomeOf or RequireAtLeast? That is more descriptive to me.

Then with the JSON, perhaps "at_least" → "some" and "m" → "at_least".

Also it's probably possible to shorten this:

Suggested change

toJSON (RequireMOf m content) =

let inside = Object ("from" .= fmap toJSON content <> "m" .= toJSON m)

in object ["at_least" .= inside]

toJSON (RequireSomeOf count scripts) =

object ["some" .= (object ["at_least" .= count, "from" .= scripts])]

good idea -> done in 4b26932

rvl · 2020-10-21T01:43:07Z

core/lib/Cardano/Address/Script.hs

+            (const $ Left $ ErrorKeyHashFromTextInvalidString $ EBech32 ())
+            Right (Bech32.decodeLenient txt)
+        case Bech32.humanReadablePartToText hrp of
+            "script_vkh" -> do


Is this specified anywhere?

yes, it it specified here -> cardano-foundation/CIPs#31

hlint another hlint fix

paweljakubas self-assigned this Oct 14, 2020

paweljakubas requested review from KtorZ and rvl October 15, 2020 08:46

paweljakubas force-pushed the paweljakubas/adp-494/to-from-json-instances-for-script branch 2 times, most recently from 4e61593 to 52a73ea Compare October 15, 2020 08:55

rvl reviewed Oct 16, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-494/to-from-json-instances-for-script branch from 52a73ea to 3c2bb9a Compare October 20, 2020 20:40

paweljakubas requested a review from rvl October 20, 2020 21:01

rvl approved these changes Oct 21, 2020

View reviewed changes

rvl force-pushed the paweljakubas/adp-494/to-from-json-instances-for-script branch from bb05bf4 to 02a941b Compare October 22, 2020 04:29

rvl approved these changes Oct 22, 2020

View reviewed changes

paweljakubas and others added 12 commits October 22, 2020 10:32

add ToJSON instance of Script

915ea82

add FromJSON for Script

b322686

adding roundtrip plus Arbitrary instance for Script

14c53c1

hlint another hlint fix

add bech32 for ver keys with proper prefix

0585dca

refactor ToJSON/FromJSON

d37cd58

relocate Script related assets

6a77081

RequireMOf -> RequireSomeOf

3e5f227

remove unneeded type annotations

bc37a1c

stop supporting Base58 in json

02845e0

Reformat FromJSON Script to make shorter

4ee3c66

ScriptSpec - use Test.QuickCheck.sized on script tree

4c2bed4

hlint fix

c5d9680

paweljakubas force-pushed the paweljakubas/adp-494/to-from-json-instances-for-script branch from a0ba4bc to c5d9680 Compare October 22, 2020 08:32

paweljakubas merged commit 1399751 into master Oct 22, 2020

paweljakubas deleted the paweljakubas/adp-494/to-from-json-instances-for-script branch October 22, 2020 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

to/from json instances for script #84

to/from json instances for script #84

paweljakubas commented Oct 14, 2020 •

edited

Loading

rvl left a comment

rvl Oct 16, 2020

paweljakubas Oct 20, 2020

rvl Oct 16, 2020

paweljakubas Oct 20, 2020

rvl Oct 16, 2020

paweljakubas Oct 20, 2020

rvl Oct 16, 2020

paweljakubas Oct 20, 2020

rvl Oct 16, 2020

paweljakubas Oct 20, 2020

rvl left a comment

rvl Oct 21, 2020

paweljakubas Oct 21, 2020

rvl Oct 21, 2020

paweljakubas Oct 21, 2020

rvl Oct 21, 2020 •

edited

Loading

paweljakubas Oct 21, 2020

rvl Oct 22, 2020

rvl Oct 21, 2020

paweljakubas Oct 21, 2020

rvl Oct 21, 2020

paweljakubas Oct 21, 2020

rvl Oct 22, 2020

		let keyHexed = T.decodeUtf8 $ encode EBase16 key
		in Object ("key" .= keyHexed)

	let keyHexed = T.decodeUtf8 $ encode EBase16 key
	in Object ("key" .= keyHexed)
	object ["key" .= T.decodeLatin1 (encode EBase16 key)]

		import Codec.Binary.Encoding
		( AbstractEncoding (..), encode, fromBase16 )

to/from json instances for script #84

to/from json instances for script #84

Conversation

paweljakubas commented Oct 14, 2020 • edited Loading

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

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

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

rvl Oct 21, 2020 • 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 commented Oct 14, 2020 •

edited

Loading

rvl Oct 21, 2020 •

edited

Loading