-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create CLI for plutus-debug
#4776
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. A suggested a few improvements
@@ -52,6 +52,7 @@ library | |||
Cardano.Ledger.PoolDistr | |||
Cardano.Ledger.PoolParams | |||
Cardano.Ledger.Plutus | |||
Cardano.Ledger.Plutus.CLI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module should go into the executable itself.
First of all we do not want to depend on the optparse-applicative
in the library. Secondly cli options aren't useful to any downstream user of this library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll move it.
{ optsScript :: !String | ||
, optsProtocolVersion :: !(Maybe Version) | ||
, optsLanguage :: !(Maybe Language) | ||
, optsCostModelValues :: ![Int64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be Maybe [Int64]
, because empty cost model is also a legal value that would turn off all plutus primitives, so it could be a sensible option that can be used for debugging
debugPlutus :: Crypto c => String -> IO (PlutusDebugInfo c) | ||
debugPlutus db = | ||
case B64.decode (BSU.fromString db) of | ||
overrideContext :: PlutusWithContext c -> Opts -> PlutusWithContext c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok to move that into the executable section. That being said, alternitivley we could move Opts
definition into this module, while only moving the associated CLI parser into executable section. If we go with the latter approach then Opts
needs a better name, eg. OverridesForPlutusWithContext
cfb11cd
to
0c6402a
Compare
0c6402a
to
159a8dd
Compare
@@ -218,33 +213,6 @@ data PlutusDebugInfo c | |||
(Maybe P.ExBudget) | |||
deriving (Show) | |||
|
|||
debugPlutus :: Crypto c => String -> IO (PlutusDebugInfo c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we cannot remove this function from the library, because it looks like there are already downstream users of it. In particular I found that hydra uses this functionality.
What I suggest in order to resolve it is to create a type in this module:
data PlutusDebugOverrides = PlutusDebugOverrides
{ pdoScript :: !(Maybe ByteString)
, pdoProtocolVersion :: !(Maybe Version)
, pdoLanguage :: !(Maybe Language)
, pdoCostModelValues :: !(Maybe [Int64])
, pdoExUnitsMem :: !(Maybe Natural)
, pdoExUnitsSteps :: !(Maybe Natural)
}
then the Opts
in CLI become:
data Opts = Opts
{ optsScriptWithContext :: !String
, optsOverrides :: !PlutusDebugOverrides
}
deriving (Show)
Then the only breaking change to this function would be addition of a new argument:
debugPlutus :: Crypto c => String -> PlutusDebugOverrides -> IO (PlutusDebugInfo c)
Description
Checklist
CHANGELOG.md
for the affected packages.New section is never added with the code changes. (See RELEASING.md)
.cabal
andCHANGELOG.md
files according to theversioning process.
.cabal
files for all affected packages are updated.If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
fourmolu
(usescripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
has been updated (usescripts/gen-hie.sh
)