Replace parameterized ScriptRequirements GADT with flat unparameterized ADTs#1348
Replace parameterized ScriptRequirements GADT with flat unparameterized ADTs#1348
Conversation
Jimbo4350
force-pushed
the
jordan/1082-make-NoScriptDatum-uninhabitable
branch
from
b6da2fc to
55ab683
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| createSimpleReferenceScriptFromCliArgs txin polId = | ||
| AnyMintScriptSimpleRef txin polId |
Check warning
Code scanning / HLint
Eta reduce Warning
Jimbo4350
changed the title
Jimbo4350
force-pushed
the
jordan/1082-make-NoScriptDatum-uninhabitable
branch
4 times, most recently
from
dfbd202 to
c4f71da
Compare
Jimbo4350
requested review from
a team,
carbolymer,
disassembler,
kevinhammond,
newhoggy and
palas
as code owners
There was a problem hiding this comment.
Pull request overview
Refactors script witness requirements in cardano-cli by replacing a parameterized ScriptRequirements GADT + type families with flat, context-specific ADTs (AnySpendScript, AnyMintScript, AnyNonAssetScript) to make invalid states unrepresentable and simplify consumers.
Changes:
- Introduces new flat ADTs for script requirements and updates all script-type modules (spend/mint/cert/vote/withdrawal/proposal) to use them.
- Updates CLI parsing and witness reading logic to construct/read the new requirement types.
- Adjusts build/test infrastructure and a few query/ledger/protocol-parameter related call sites to newer APIs.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| flake.nix | Updates dev-shell package set (consensus package rename). |
| cardano-cli/test/cardano-cli-golden/files/golden/help/query_ledger-peer-snapshot.cli | Updates golden help output (removes --all-ledger-peers). |
| cardano-cli/test/cardano-cli-golden/files/golden/help/latest_query_ledger-peer-snapshot.cli | Updates golden help output (removes --all-ledger-peers). |
| cardano-cli/test/cardano-cli-golden/files/golden/help/conway_query_ledger-peer-snapshot.cli | Updates golden help output (removes --all-ledger-peers). |
| cardano-cli/test/cardano-cli-golden/files/golden/help.cli | Updates aggregated golden help output accordingly. |
| cardano-cli/test/cardano-cli-golden/Test/Golden/Legacy/Genesis/Create.hs | Changes golden test to assert V2 cost model parameter count. |
| cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs | Updates stake pool relay accessor names. |
| cardano-cli/src/Cardano/CLI/Type/Common.hs | Updates stake pool state construction to new ledger API signature. |
| cardano-cli/src/Cardano/CLI/Read.hs | Refactors experimental script reading imports and plumbs through updated Plutus helpers. |
| cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs | Updates submit result constructors and fee parameter accessors/types. |
| cardano-cli/src/Cardano/CLI/EraBased/Transaction/Command.hs | Swaps GADT requirements for new Any*Script types in tx command args. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Withdrawal/Type.hs | Constructs new AnyNonAssetScript requirements for withdrawals. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Withdrawal/Read.hs | Reads withdrawal witnesses from new requirement types. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Vote/Type.hs | Constructs new AnyNonAssetScript requirements for votes. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Vote/Read.hs | Reads vote witnesses from new requirement types. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Type.hs | Replaces ScriptRequirements GADT with flat context-specific ADTs. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Spend/Type.hs | Constructs new AnySpendScript requirements for spending. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Spend/Read.hs | Reads spending witnesses from new requirement types. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Read/Common.hs | Simplifies simple-script JSON fallback path. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Proposal/Type.hs | Constructs new AnyNonAssetScript requirements for proposals. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Proposal/Read.hs | Reads proposal witnesses from new requirement types. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Mint/Type.hs | Constructs new AnyMintScript requirements for minting. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Mint/Read.hs | Reads mint witnesses from new requirement types. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Certificate/Type.hs | Constructs new AnyNonAssetScript requirements for certs. |
| cardano-cli/src/Cardano/CLI/EraBased/Script/Certificate/Read.hs | Reads cert witnesses from new requirement types. |
| cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs | Drops “all ledger peers” path and refactors some era handling + decoders. |
| cardano-cli/src/Cardano/CLI/EraBased/Query/Option.hs | Removes --all-ledger-peers flag parsing. |
| cardano-cli/src/Cardano/CLI/EraBased/Query/Command.hs | Removes CliLedgerPeers and field from the command args. |
| cardano-cli/src/Cardano/CLI/EraBased/Governance/Actions/Option.hs | Updates conversion for utxo-cost-per-byte. |
| cardano-cli/src/Cardano/CLI/EraBased/Genesis/Run.hs | Updates stake pool params types/accessors to new ledger API. |
| cardano-cli/src/Cardano/CLI/EraBased/Genesis/CreateTestnetData/Run.hs | Updates stake pool params types/accessors in testnet data generator. |
| cardano-cli/src/Cardano/CLI/EraBased/Common/Option.hs | Updates parsers and switches to Any*Script requirement types. |
| cardano-cli/src/Cardano/CLI/Compatible/Transaction/ScriptWitness.hs | Reworks compatible certificate-witness reading around new script requirement types. |
| cardano-cli/src/Cardano/CLI/Compatible/Transaction/Run.hs | Updates compatible tx flow for new cert-witness requirements and Plutus decoding path. |
| cardano-cli/src/Cardano/CLI/Compatible/Transaction/Option.hs | Updates compatible tx option typing for votes. |
| cardano-cli/src/Cardano/CLI/Compatible/Transaction/Command.hs | Updates compatible tx command args to new script requirement types. |
| cardano-cli/src/Cardano/CLI/Compatible/Read.hs | Adds AnyPlutusScript wrapper and refactors Plutus script deserialisation. |
| cardano-cli/src/Cardano/CLI/Compatible/Json/Friendly.hs | Updates ledger tx type parameters and ledger-role kind annotations. |
| cardano-cli/src/Cardano/CLI/Byron/Tx.hs | Updates submit result constructors (network namespace). |
| cardano-cli/cardano-cli.cabal | Adjusts dependency bounds and adds test deps (e.g., containers, cardano-ledger-core). |
| cabal.project | Updates index-state pins for Hackage/CHaP. |
Comments suppressed due to low confidence (5)
cardano-cli/src/Cardano/CLI/EraBased/Query/Run.hs:1
- When
decodeBigLedgerPeerSnapshotfails, the decoder error is discarded (_decoderError) and only the CBOR is printed. Previously, the decoder error was emitted to stderr, which is useful for diagnosing incompatibilities. Consider preserving the warning behavior (e.g., log the decoder error) while still dumping CBOR as a fallback.
{-# LANGUAGE DataKinds #-}
cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs:1
- This mixes
Lovelaceand ledgerCoin(L.Coin) in a way that is unlikely to typecheck (e.g.,toInteger txFeePerBytetypically won’t compile forLovelace) and also muddies the unit semantics (ledger fee coefficientminFeeAis conceptually 'coin per byte', not an arbitrary lovelace amount). Consider changingcalculateByronWitnessFeesto accept/return a consistent coin type (either ledgerCoinor APILovelace) and convert explicitly at the boundary (e.g., unwrapLovelaceor wrapCoin), so the multiplication uses the correct numeric representation.
cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs:1 - This mixes
Lovelaceand ledgerCoin(L.Coin) in a way that is unlikely to typecheck (e.g.,toInteger txFeePerBytetypically won’t compile forLovelace) and also muddies the unit semantics (ledger fee coefficientminFeeAis conceptually 'coin per byte', not an arbitrary lovelace amount). Consider changingcalculateByronWitnessFeesto accept/return a consistent coin type (either ledgerCoinor APILovelace) and convert explicitly at the boundary (e.g., unwrapLovelaceor wrapCoin), so the multiplication uses the correct numeric representation.
cardano-cli/test/cardano-cli-golden/Test/Golden/Legacy/Genesis/Create.hs:1 - This golden test changes from asserting the presence/shape of the Plutus V1 cost model in an Alonzo genesis to asserting Plutus V2 with 175 parameters. That’s a significant behavioral expectation shift and looks unrelated to the stated PR goal (script-requirements refactor). If this is intentional, it should be justified in the PR description/changelog (and ideally linked to a specific issue/requirement); otherwise, consider keeping this PR focused by moving this test change into a separate PR.
cardano-cli/test/cardano-cli-golden/Test/Golden/Legacy/Genesis/Create.hs:1 - This golden test changes from asserting the presence/shape of the Plutus V1 cost model in an Alonzo genesis to asserting Plutus V2 with 175 parameters. That’s a significant behavioral expectation shift and looks unrelated to the stated PR goal (script-requirements refactor). If this is intentional, it should be justified in the PR description/changelog (and ideally linked to a specific issue/requirement); otherwise, consider keeping this PR focused by moving this test change into a separate PR.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pQueryLedgerPeerSnapshotCmd envCli = | ||
| fmap QueryLedgerPeerSnapshotCmd $ | ||
| QueryLedgerPeerSnapshotCmdArgs | ||
| <$> pQueryCommons @era envCli | ||
| <*> pLedgerPeersKind | ||
| <*> pFormatQueryOutputFlags | ||
| "ledger-peer-snapshot" | ||
| [ flagFormatJson & setDefault | ||
| , flagFormatYaml | ||
| ] | ||
| <*> pMaybeOutputFile |
There was a problem hiding this comment.
This PR is described as a refactor of ScriptRequirements, but it also removes the --all-ledger-peers CLI flag (and the associated behavior) from query ledger-peer-snapshot. If this behavior removal is intentional, it should be explicitly called out in the PR description/changelog as a user-facing breaking change; otherwise, consider restoring the flag and plumbing it through to the query decoder as before.
| era <- hoist liftIO $ supportedEra cEra | ||
| let sbe = convert era | ||
|
|
||
| result <- easyRunQuery (queryLedgerPeerSnapshot sbe) |
There was a problem hiding this comment.
When decodeBigLedgerPeerSnapshot fails, the decoder error is discarded (_decoderError) and only the CBOR is printed. Previously, the decoder error was emitted to stderr, which is useful for diagnosing incompatibilities. Consider preserving the warning behavior (e.g., log the decoder error) while still dumping CBOR as a fallback.
| hoist liftIO $ | ||
| obtainCommonConstraints era $ | ||
| case decodeBigLedgerPeerSnapshot shelleyNtcVersion result of | ||
| Left (bs, _decoderError) -> pure $ Left bs |
There was a problem hiding this comment.
When decodeBigLedgerPeerSnapshot fails, the decoder error is discarded (_decoderError) and only the CBOR is printed. Previously, the decoder error was emitted to stderr, which is useful for diagnosing incompatibilities. Consider preserving the warning behavior (e.g., log the decoder error) while still dumping CBOR as a fallback.
| Left (bs, _decoderError) -> pure $ Left bs | |
| Left (bs, decoderError) -> do | |
| putStrLn ("Warning: failed to decode LedgerPeerSnapshot: " ++ show decoderError) | |
| pure $ Left bs |
| case result of | ||
| Left (bs :: LBS.ByteString) -> do | ||
| fromExceptTCli $ pPrintCBOR bs | ||
| Right (SomeLedgerPeerSnapshot snapshot) -> do | ||
| Right (snapshot :: LedgerPeerSnapshot) -> do |
There was a problem hiding this comment.
When decodeBigLedgerPeerSnapshot fails, the decoder error is discarded (_decoderError) and only the CBOR is printed. Previously, the decoder error was emitted to stderr, which is useful for diagnosing incompatibilities. Consider preserving the warning behavior (e.g., log the decoder error) while still dumping CBOR as a fallback.
| readFileSimpleScript file era = do | ||
| bs <- readFileCli file | ||
| case deserialiseFromJSON bs of | ||
| Left _ -> case era of | ||
| Exp.DijkstraEra -> error "TODO Dijkstra: Simple script not supported" | ||
| Exp.ConwayEra -> Exp.obtainConwayConstraints era $ do | ||
| -- In addition to the TextEnvelope format, we also try to | ||
| -- deserialize the JSON representation of SimpleScripts.. | ||
| script :: SimpleScript <- fromEitherCli $ Aeson.eitherDecodeStrict' bs | ||
| let s :: L.NativeScript (Exp.LedgerEra era) = obtainCommonConstraints era $ toAllegraTimelock script | ||
| return $ Exp.SimpleScript s | ||
| Left _ -> do | ||
| -- In addition to the TextEnvelope format, we also try to | ||
| -- deserialize the JSON representation of SimpleScripts.. | ||
| script :: SimpleScript <- fromEitherCli $ Aeson.eitherDecodeStrict' bs | ||
| let s :: L.NativeScript (Exp.LedgerEra era) = obtainCommonConstraints era $ toAllegraTimelock script | ||
| return $ obtainCommonConstraints (era :: Exp.Era era) $ Exp.SimpleScript s |
There was a problem hiding this comment.
This removes the earlier era-based guard that explicitly rejected simple scripts for DijkstraEra (previously an error \"TODO Dijkstra: Simple script not supported\"). If simple scripts are still unsupported in Dijkstra, this change will now attempt to construct/return a script anyway, which can lead to incorrect behavior or later failures. Either reintroduce the explicit rejection for unsupported eras, or implement the intended Dijkstra support end-to-end (including constraints and witness creation).
cardano-cli/cardano-cli.cabal
Outdated
| cardano-crypto-class ^>=2.3, | ||
| cardano-crypto-wrapper ^>=1.7, | ||
| cardano-crypto-class ^>=2.2.3.2, | ||
| cardano-crypto-wrapper ^>=1.6, |
There was a problem hiding this comment.
The PR introduces/uses newer API surface in several places (e.g., new namespaced submit result constructors, additional Plutus script versions/types), but the cabal bounds are moved down (e.g., cardano-api ^>=10.25 → ^>=10.24, plus downgrades for cardano-ping and crypto libs). Please verify the chosen lower bounds actually provide the constructors/types referenced in this PR; otherwise, raise the bounds (or add appropriate CPP/compat shims) to keep the build plan consistent.
| cardano-crypto-wrapper ^>=1.6, | |
| cardano-crypto-wrapper ^>=1.7, |
cardano-cli/cardano-cli.cabal
Outdated
| cardano-ledger-core, | ||
| cardano-ledger-dijkstra, | ||
| cardano-ping ^>=0.10, | ||
| cardano-ping ^>=0.9, |
There was a problem hiding this comment.
The PR introduces/uses newer API surface in several places (e.g., new namespaced submit result constructors, additional Plutus script versions/types), but the cabal bounds are moved down (e.g., cardano-api ^>=10.25 → ^>=10.24, plus downgrades for cardano-ping and crypto libs). Please verify the chosen lower bounds actually provide the constructors/types referenced in this PR; otherwise, raise the bounds (or add appropriate CPP/compat shims) to keep the build plan consistent.
| cardano-ping ^>=0.9, | |
| cardano-ping ^>=0.10.0.0, |
cardano-cli/cardano-cli.cabal
Outdated
| ordered-containers, | ||
| ouroboros-consensus:{cardano, ouroboros-consensus}, | ||
| ouroboros-consensus, | ||
| ouroboros-consensus-cardano, |
There was a problem hiding this comment.
The PR introduces/uses newer API surface in several places (e.g., new namespaced submit result constructors, additional Plutus script versions/types), but the cabal bounds are moved down (e.g., cardano-api ^>=10.25 → ^>=10.24, plus downgrades for cardano-ping and crypto libs). Please verify the chosen lower bounds actually provide the constructors/types referenced in this PR; otherwise, raise the bounds (or add appropriate CPP/compat shims) to keep the build plan consistent.
| ouroboros-consensus-cardano, | |
| ouroboros-consensus-cardano ^>=0.17.0.0, |
| pMinFeePerByteFactor :: Parser Lovelace | ||
| pMinFeePerByteFactor = | ||
| Opt.option (readerFromParsecParser parseCoinPerByte) $ | ||
| Opt.option (readerFromParsecParser parseLovelace) $ |
There was a problem hiding this comment.
These options represent 'per-byte' fee parameters, but are now parsed as plain Lovelace, which loses unit safety and makes it easier to accidentally pass the wrong value/type through the stack. Since other call sites still appear to expect/construct CoinPerByte (e.g., via L.CoinPerByte), consider restoring a dedicated CoinPerByte parser type (or introducing a small newtype wrapper for CLI) and only converting to/from raw lovelace at the precise API boundary.
| pUTxOCostPerByte :: Parser Lovelace | ||
| pUTxOCostPerByte = | ||
| Opt.option (readerFromParsecParser parseCoinPerByte) $ | ||
| Opt.option (readerFromParsecParser parseLovelace) $ |
There was a problem hiding this comment.
These options represent 'per-byte' fee parameters, but are now parsed as plain Lovelace, which loses unit safety and makes it easier to accidentally pass the wrong value/type through the stack. Since other call sites still appear to expect/construct CoinPerByte (e.g., via L.CoinPerByte), consider restoring a dedicated CoinPerByte parser type (or introducing a small newtype wrapper for CLI) and only converting to/from raw lovelace at the precise API boundary.
Jimbo4350
force-pushed
the
jordan/1082-make-NoScriptDatum-uninhabitable
branch
3 times, most recently
from
ce64a75 to
a369f55
Compare
The previous ScriptRequirements GADT and associated type families
(OptionalDatum, MintPolicyId) forced callers to pattern match on
nullary constructors like NoPolicyId and NoScriptDatumAllowed that
carry no information. For example, in the old Certificate/Read.hs:
PlutusRefScriptCliArgs
refInput
(AnySLanguage lang)
Exp.NoScriptDatumAllowed -- meaningless
NoPolicyId -- meaningless
redeemerFile
execUnits
These constructors exist only to fill slots demanded by the type
families in contexts where those fields don't apply. This is an
inherent cost of a single GADT polymorphic over all script contexts.
Replace with flat, context-specific ADTs that simply omit fields
irrelevant to their context:
- PlutusSpendingScriptRequirements: carries an optional datum
- PlutusMintingScriptRequirements: carries a PolicyId
- PlutusNonAssetScriptRequirements: needs neither (certificates,
voting, withdrawals, proposals)
Update Compatible layer, Transaction runners, and option parsers to use the new flat script types.
Jimbo4350
force-pushed
the
jordan/1082-make-NoScriptDatum-uninhabitable
branch
from
a369f55 to
22dc166
Compare
Changelog
Context
Replaces the parameterized
ScriptRequirementsGADT and its associated type families (OptionalDatum,MintPolicyId) with flat, context-specific ADTs that make invalid states unrepresentable — in particular,NoScriptDatumcan no longer appear in types where a datum is expected.Closes #1082
Commit structure
Script/Type.hsCommon/Option.hs,Read/Common.hs,Type/Common.hsHow to trust this PR
Each per-script-type commit touches only the
Type.hsandRead.hsfor that script context, making it easy to review in isolation. The final consumer propagation commit covers the mechanical updates needed to wire everything together. The overall diff is a net reduction in code (~100 fewer lines) due to eliminating standalone deriving instances and type family machinery.Checklist