Skip to content

Consolidate script interface #1217

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jimbo4350 merged 12 commits into from
Jun 27, 2025
Merged

Consolidate script interface #1217

Jimbo4350 merged 12 commits into from
Jun 27, 2025

Conversation

@Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Jun 11, 2025 •
edited
Loading

Changelog

- description: |
    Refactor simple and plutus script interface via the `ScriptRequirements` type.
# uncomment types applicable to the change:
  type:
  - compatible     # the API has changed but is non-breaking
  - refactoring    # QoL changes

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 11, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Script/Certificate/Read.hs
Comment on lines 121 to 144
readCertificateScriptWitnesses certs =
mapM
( \(vFile, mCert) -> do
case mCert of
Nothing -> return (vFile, AnyKeyWitnessPlaceholder)
Just cert -> do
sWit <- readCertificateScriptWitness cert
return (vFile, sWit)
)
certs

Check warning

Code scanning / HLint

Eta reduce Warning

cardano-cli/src/Cardano/CLI/EraBased/Script/Certificate/Read.hs:(134,1)-(143,9): Warning: Eta reduce
  
Found:
  readCertificateScriptWitnesses certs
    = mapM
        (\\ (vFile, mCert)
           -> do case mCert of
                   Nothing -> return (vFile, AnyKeyWitnessPlaceholder)
                   Just cert
                     -> do sWit <- readCertificateScriptWitness cert
                           return (vFile, sWit))
        certs
  
Perhaps:
  readCertificateScriptWitnesses
    = mapM
        (\\ (vFile, mCert)
           -> do case mCert of
                   Nothing -> return (vFile, AnyKeyWitnessPlaceholder)
                   Just cert
                     -> do sWit <- readCertificateScriptWitness cert
                           return (vFile, sWit))
cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs
Comment on lines 209 to 213
[ (,mSwit)
<$> ( fmap (Exp.convertToNewCertificate Exp.useEra) $
fromEitherIOCli @(FileError TextEnvelopeError) $
shelleyBasedEraConstraints eon $
readFileTextEnvelope (File certFile)
)

Check notice

Code scanning / HLint

Move brackets to avoid $ Note

cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs:(182,11)-(187,17): Suggestion: Move brackets to avoid $
  
Found:
  (, mSwit)
    <$>
      (fmap (Exp.convertToNewCertificate Exp.useEra)
         $ fromEitherIOCli @(FileError TextEnvelopeError)
             $ shelleyBasedEraConstraints eon
                 $ readFileTextEnvelope (File certFile))
  
Perhaps:
  (, mSwit)
    <$>
      fmap
        (Exp.convertToNewCertificate Exp.useEra)
        (fromEitherIOCli @(FileError TextEnvelopeError)
           $ shelleyBasedEraConstraints eon
               $ readFileTextEnvelope (File certFile))
cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs
[ (,mSwit)
<$> ( fmap (Exp.convertToNewCertificate Exp.useEra) $
shelleyBasedEraConstraints sbe $
fromEitherIOCli $
readFileTextEnvelope (File certFile)
)

Check notice

Code scanning / HLint

Move brackets to avoid $ Note

cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs:(470,11)-(475,17): Suggestion: Move brackets to avoid $
  
Found:
  (, mSwit)
    <$>
      (fmap (Exp.convertToNewCertificate Exp.useEra)
         $ shelleyBasedEraConstraints sbe
             $ fromEitherIOCli $ readFileTextEnvelope (File certFile))
  
Perhaps:
  (, mSwit)
    <$>
      fmap
        (Exp.convertToNewCertificate Exp.useEra)
        (shelleyBasedEraConstraints sbe
           $ fromEitherIOCli $ readFileTextEnvelope (File certFile))
cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs
Comment on lines 711 to 706
[ (,mSwit)
<$> ( fmap (Exp.convertToNewCertificate Exp.useEra) $
fromEitherIOCli $
readFileTextEnvelope (File certFile)
)

Check notice

Code scanning / HLint

Move brackets to avoid $ Note

cardano-cli/src/Cardano/CLI/EraBased/Transaction/Run.hs:(704,13)-(708,19): Suggestion: Move brackets to avoid $
  
Found:
  (, mSwit)
    <$>
      (fmap (Exp.convertToNewCertificate Exp.useEra)
         $ fromEitherIOCli $ readFileTextEnvelope (File certFile))
  
Perhaps:
  (, mSwit)
    <$>
      fmap
        (Exp.convertToNewCertificate Exp.useEra)
        (fromEitherIOCli $ readFileTextEnvelope (File certFile))
@Jimbo4350 Jimbo4350 force-pushed the jordan/consolidate-script-interface branch 7 times, most recently from 6751eb0 to a08f3ca Compare June 18, 2025 18:44
@Jimbo4350 Jimbo4350 force-pushed the jordan/consolidate-script-interface branch from 32604f4 to 1e71580 Compare June 18, 2025 20:09
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 18, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 24, 2025
View reviewed changes
@Jimbo4350 Jimbo4350 force-pushed the jordan/consolidate-script-interface branch 7 times, most recently from 19ad6a5 to 8da24cc Compare June 25, 2025 13:26
@Jimbo4350 Jimbo4350 marked this pull request as ready for review June 25, 2025 13:26
@Jimbo4350 Jimbo4350 force-pushed the jordan/consolidate-script-interface branch from 8da24cc to 9ccc4c0 Compare June 25, 2025 13:31
nbacquey
nbacquey reviewed Jun 26, 2025
View reviewed changes
Copy link
Contributor

@nbacquey nbacquey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some stylistic remarks. I'm not familiar enough with the semantics to have a relevant opinion

cardano-cli/src/Cardano/CLI/EraBased/Script/Type.hs
SimpleReferenceScript
:: SimpleRefScriptCliArgs witnessable -> ScriptRequirements witnessable

deriving instance Show (ScriptRequirements Exp.VoterItem)
Copy link
Contributor

@nbacquey nbacquey Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not instance Show a => Show (ScriptRequirements a)?

Copy link
Contributor Author

@Jimbo4350 Jimbo4350 Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually it will ask for Show (OptionalDatum a) and I will have to provide manual instances for it because you cannot automatically derive instance for type families.

cardano-cli/src/Cardano/CLI/EraBased/Script/Type.hs
| InlineDatum
deriving Show

deriving instance Show (OnDiskPlutusScriptCliArgs Exp.VoterItem)
Copy link
Contributor

@nbacquey nbacquey Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about Show instances

cardano-cli/src/Cardano/CLI/EraBased/Script/Type.hs
-> ExecutionUnits
-> PlutusRefScriptCliArgs witnessable

deriving instance Show (PlutusRefScriptCliArgs Exp.VoterItem)
Copy link
Contributor

@nbacquey nbacquey Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

cardano-cli/src/Cardano/CLI/EraBased/Script/Type.hs
data SimpleRefScriptCliArgs witnessable where
SimpleRefScriptArgs :: TxIn -> MintPolicyId witnessable -> SimpleRefScriptCliArgs witnessable

deriving instance Show (SimpleRefScriptCliArgs Exp.VoterItem)
Copy link
Contributor

@nbacquey nbacquey Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@Jimbo4350 Jimbo4350 force-pushed the jordan/consolidate-script-interface branch from 9ccc4c0 to 8be9beb Compare June 26, 2025 14:11
palas
palas approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@palas palas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simplification! I don't see any issues in the PR (except for the underscore, which seems odd), and types here go a long way. But I would get some more approvals because it is a big PR and my understanding of the changes is limited

Jimbo4350 added 2 commits June 27, 2025 11:01
@Jimbo4350
Introduce data definition ScriptRequirements
e460401 
This is designed to handle reading simple and plutus scripts from disk
@Jimbo4350
Modify voting script witness to use ScriptRequirements
4776d90
@Jimbo4350 Jimbo4350 force-pushed the jordan/consolidate-script-interface branch from dd34973 to d6a08be Compare June 27, 2025 15:17
@Jimbo4350 Jimbo4350 enabled auto-merge June 27, 2025 15:17
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Jun 27, 2025
Merged via the queue into master with commit 298ec6d Jun 27, 2025
27 checks passed
@Jimbo4350 Jimbo4350 deleted the jordan/consolidate-script-interface branch June 27, 2025 18:42
@Jimbo4350 Jimbo4350 mentioned this pull request Jun 30, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nbacquey nbacquey nbacquey left review comments

@palas palas palas approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@carbolymer carbolymer Awaiting requested review from carbolymer carbolymer is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Jimbo4350 @palas @nbacquey