use TH-generated plutus scripts #5048

NadiaYvette · 2023-04-03T14:40:15Z

Using TH-generated scripts helps keep flake dependencies down & avoids having to go through a different team for changes to the scripts.

This helps keep flake dependencies down & avoids having to go through a different team for changes to the scripts.

deepfire · 2023-04-07T09:41:50Z

bench/plutus-scripts-bench/plutus-scripts-bench.cabal

    , bytestring
    , serialise
    , template-haskell
+    , filepath


We try to keep the package and module lists sorted.

deepfire · 2023-04-07T09:44:27Z

bench/plutus-scripts-bench/src/Cardano/Benchmarking/PlutusScripts.hs

-  , (normalizeModuleName Loop.scriptName      , asAnyLang Loop.scriptSerialized)
-  , (normalizeModuleName Schnorr.scriptName   , asAnyLang Schnorr.scriptSerialized)
-  ]
+  [ CustomCall.script, ECDSA.script, Loop.script, Schnorr.script ]


This will have quicker to perceive diffs, if we break this line to list-entry-per-line format:

Suggested change

[ CustomCall.script, ECDSA.script, Loop.script, Schnorr.script ]

[ CustomCall.script

, ECDSA.script

, Loop.script

, Schnorr.script

]

deepfire · 2023-04-07T09:46:52Z

bench/plutus-scripts-bench/src/Cardano/Benchmarking/PlutusScripts.hs

-findPlutusScript
-  = (`lookup` getAllScripts)
+findPlutusScript s
+  = listToMaybe [psScript t | t <- getAllScripts, last (split (=='.') . pack $ psName t) == pack (takeBaseName s)]


I think this might be easier to read:

Suggested change

= listToMaybe [psScript t | t <- getAllScripts, last (split (=='.') . pack $ psName t) == pack (takeBaseName s)]

= listToMaybe [ psScript t

| t <- getAllScripts

, last (split (=='.') . pack $ psName t) == pack (takeBaseName s) ]

Also, further, last (split (=='.') . pack $ psName t) == pack (takeBaseName s) might be worth binding to a local function name -- to hint at intended meaning.

bench/plutus-scripts-bench/src/Cardano/Benchmarking/PlutusScripts/CustomCall.hs

deepfire · 2023-04-07T09:53:49Z

bench/plutus-scripts-bench/src/Cardano/Benchmarking/ScriptAPI.hs

+data BenchScript
+  = BenchScript
+    { psName   :: String
+    , psScript :: ScriptInAnyLang
+    }


I apologise for not raising this matter sooner, since it'd save work, but I can't fail to mention this either.

The choice of name here is a bit confusing -- if taken with a bit of a wider context "bench script" can mean different things in the benchmarking automation ecosystem.

A bit more specificity, like, perhaps, but not necessarily, PlutusBenchScript or PlutusWorkloadScript would be really useful.

deepfire · 2023-04-07T09:54:58Z

bench/plutus-scripts-bench/src/Cardano/Benchmarking/ScriptAPI.hs

+{-# OPTIONS_GHC -fno-warn-orphans #-}
+
+module Cardano.Benchmarking.ScriptAPI
+  (BenchScript, psName, psScript, mkBenchScript)


Again, to reach for easier diff reading time down the line:

Suggested change

(BenchScript, psName, psScript, mkBenchScript)

( BenchScript

, psName

, psScript

, mkBenchScript

)

deepfire · 2023-04-07T10:16:59Z

bench/tx-generator/src/Cardano/TxGenerator/Setup/Plutus.hs

-readPlutusScript fp
+readPlutusScript :: Either String FilePath -> IO (Either TxGenError ScriptInAnyLang)
+readPlutusScript (Left s)
+  = return


Very minor, but pure in stead of return is more suggestive.

deepfire · 2023-04-07T10:29:57Z

nix/workbench/profile/prof1-variants.jq

@@ -276,7 +276,7 @@ def all_profile_variants:
   ({ generator:
      { plutus:
          { type:                       "LimitSaturationLoop"
-          , script:                     "v1/loop.plutus"
+          , script:                     { "Left": "Loop.hs" }


I'd question whether the .hs suffix really belong in the script names.

We want the script name to be present in the reports -- and the report generator (and the rest of the workbench, by the way) mostly treats profile definitions as abstract, where all names are clean and usable directly.

If the report generator had to do the .hs suffix stripping, that'd be a pretty severe violation of the abstraction.

The same goes for the Left/Right wrappers -- they'd be unnatural to deal with.

So what I suggest -- since we're unlikely to use the Plutus scripts directly -- they all depend on the script-specific tx-generator knowledge to run them -- to just:

drop the Left wrappers from the profile definitions

unconditionally add the Left at the point where we pass the value to tx-generator

So it'd be a Nix-only change.

deepfire

@NadiaYvette -- this is excellent work, thank you!

deepfire · 2023-04-08T08:47:15Z

bench/plutus-scripts-bench/src/Cardano/Benchmarking/ScriptAPI.hs

+prepareScriptName :: String -> String
+prepareScriptName
+  = head
+  . dropWhile (=="hs")
+  . map unpack
+  . reverse
+  . split (=='.')
+  . last
+  . split (=='/')
+  . pack


https://hackage.haskell.org/package/filepath-1.4.100.3/docs/System-FilePath.html#v:dropExtension can simplify this a lot.

nix/workbench/profile/prof1-variants.jq

Commment what it's doing, make better use of filepath primitives, remove dependency on the text package, and use less golfy style.

mgmeier

Thank you very much @NadiaYvette, great work.

NadiaYvette added the WIP Work In Progress (cannot be merged yet) label Apr 3, 2023

NadiaYvette self-assigned this Apr 3, 2023

NadiaYvette requested review from a team, deepfire, mgmeier, fmaste, jutaro and cleverca22 as code owners April 3, 2023 14:40

NadiaYvette force-pushed the nyc-plutus-bench-01 branch from 7844ee4 to 4a36e28 Compare April 3, 2023 15:08

plutus-scripts-bench: Use TH-generated scripts.

74098c1

This helps keep flake dependencies down & avoids having to go through a different team for changes to the scripts.

NadiaYvette force-pushed the nyc-plutus-bench-01 branch from 72fa031 to 74098c1 Compare April 7, 2023 00:12

deepfire reviewed Apr 7, 2023

View reviewed changes

bench/plutus-scripts-bench/src/Cardano/Benchmarking/PlutusScripts/CustomCall.hs Show resolved Hide resolved

deepfire reviewed Apr 7, 2023

View reviewed changes

deepfire approved these changes Apr 7, 2023

View reviewed changes

NadiaYvette added 2 commits April 7, 2023 11:17

plutus-scripts-bench: Do stylistic suggestions from Serge's code review.

3c51222

plutus-scripts-bench: Hide suffixes & Either from users.

2c9d74e

deepfire reviewed Apr 8, 2023

View reviewed changes

nix/workbench/profile/prof1-variants.jq Show resolved Hide resolved

NadiaYvette added 2 commits April 8, 2023 12:21

plutus-scripts-bench: rewrite script name normalisation

897cb2e

Commment what it's doing, make better use of filepath primitives, remove dependency on the text package, and use less golfy style.

plutus-scripts-bench: fix hlint warnings

2fe2c43

mgmeier force-pushed the nyc-plutus-bench-01 branch from 1e8d37c to 2fe2c43 Compare April 11, 2023 15:23

mgmeier approved these changes Apr 11, 2023

View reviewed changes

NadiaYvette removed the WIP Work In Progress (cannot be merged yet) label Apr 11, 2023

mgmeier enabled auto-merge April 11, 2023 15:28

mgmeier added this pull request to the merge queue Apr 11, 2023

Merged via the queue into master with commit 26af241 Apr 11, 2023

iohk-bors bot deleted the nyc-plutus-bench-01 branch April 11, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use TH-generated plutus scripts #5048

use TH-generated plutus scripts #5048

NadiaYvette commented Apr 3, 2023 •

edited

Loading

deepfire Apr 7, 2023

deepfire Apr 7, 2023

deepfire Apr 7, 2023 •

edited

Loading

deepfire Apr 7, 2023

deepfire Apr 7, 2023 •

edited

Loading

deepfire Apr 7, 2023 •

edited

Loading

deepfire Apr 7, 2023

deepfire Apr 7, 2023

deepfire left a comment

deepfire Apr 8, 2023

mgmeier left a comment

use TH-generated plutus scripts #5048

use TH-generated plutus scripts #5048

Conversation

NadiaYvette commented Apr 3, 2023 • edited Loading

deepfire Apr 7, 2023

Choose a reason for hiding this comment

deepfire Apr 7, 2023

Choose a reason for hiding this comment

deepfire Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

deepfire Apr 7, 2023

Choose a reason for hiding this comment

deepfire Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

deepfire Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

deepfire Apr 7, 2023

Choose a reason for hiding this comment

deepfire Apr 7, 2023

Choose a reason for hiding this comment

deepfire left a comment

Choose a reason for hiding this comment

deepfire Apr 8, 2023

Choose a reason for hiding this comment

mgmeier left a comment

Choose a reason for hiding this comment

NadiaYvette commented Apr 3, 2023 •

edited

Loading

deepfire Apr 7, 2023 •

edited

Loading

deepfire Apr 7, 2023 •

edited

Loading

deepfire Apr 7, 2023 •

edited

Loading