Minting script witness refactor #971

Jimbo4350 · 2024-11-18T19:52:25Z

Changelog

- description: |
    Minting script witness refactor
    TODO: Add more details
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

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

carbolymer

LGTM after first pass 👍🏻 Nice work.

carbolymer · 2024-11-19T13:37:44Z

cardano-cli/src/Cardano/CLI/Plutus/Minting.hs

+  , createOnDiskSimpleOfPlutusScriptCliArgs
+  , createOnDiskSimpleReferenceScriptCliArgs
+  , createOnDiskPlutusReferenceScriptCliArgs


I think we should switch to either using OnDiskXXXXThing or FileXXXXThing names convention for the consistency, eventually. It would make navigating codebase easier.

What about:

createSimpleOrPlutusScriptFromCliArgs createSimpleReferenceScriptFromCliArgs createPlutusReferenceScriptFromCliArgs

That's fine I think. I had in mind the type names as well.

carbolymer · 2024-11-19T13:44:25Z

cardano-cli/src/Cardano/CLI/Plutus/Minting.hs

I don't think this module is in the right place. Can we split it into two:

cardano-cli/src/Cardano/CLI/Types/Plutus.hs with MintScriptWitWithPolId, CliMintScriptRequirements and createOnDiskXXX functions

cardano-cli/src/Cardano/CLI/Read/Plutus.hs with readMintScriptWitness and CliScriptWitnessError

?

I'm not a fan of the current module structure, but it never felt like a good moment to turn everything upside down.

For the moment I think we should follow it rather than introducing different changes.

I propose the following change:
https://gist.github.com/carbolymer/63b23d0df940c61d1501a6763d86134c

I'm not insisting on keeping EraBased if we have a better alternative here.

Thoughts?

100% agree with the new structure and the module split.

So then based on what you suggested we would have:

Cardano/CLI ├── Byron ├── Commands ├── EraBased │ └── Plutus │ └── Read.hs │ └── Types.hs │ │

├── Commands could be organised by domains too I think.

cardano-cli/src/Cardano/CLI/Plutus/Minting.hs

cardano-cli/src/Cardano/CLI/Read.hs

cardano-cli/src/Cardano/CLI/Plutus/Minting.hs

cardano-cli/src/Cardano/CLI/Read.hs

Define the data definition CliMintScriptRequirements This type makes it clearer that we require the policy id for transaction construction when using a minting script

AnyScriptLanguage Replace ScriptWitnessFiles WitCtxMint with CliMintScriptRequirements

AnyPlutusScriptVersion instead of AnyScriptLanguage

Refactor readScriptWitness and eliminate invalid states Factor out fromSomeTypeSimpleScript and fromSomeTypePlutusScripts fromSomeTypePlutusScripts should automatically be updated as soon as the Enum AnyPlutusScriptVersion instance is updated in cardano-api

Add ScriptDecodeUnknownPlutusScriptVersion to ScriptDecodeError

- Cardano.CLI.Types.Errors.PlutusScriptDecodeError - Cardano.CLI.Types.Errors.ScriptDataError

cardano-cli/src/Cardano/CLI/Types/Common.hs

handle all minting scripts (simple and plutus) we no longer have to accomodate for the PolicyId in the constructors PlutusReferenceScriptWitnessFiles and SimpleReferenceScriptWitnessFiles This is evidenced by the diff of this commit The goal is to deprecate ScriptWitnessFiles and replace it with a collection of types for the different script purposes. The first example of this is MintScriptWitnessWithPolicyId era

from PReferenceScript and SReferenceScript constructors Co-authored-by: Mateusz Gałażyn <[email protected]>

smelc · 2024-11-22T10:51:48Z

cardano-cli/src/Cardano/CLI/Types/Errors/PlutusScriptDecodeError.hs

+      "Version mismatch in code: script version that was read"
+        <> pretty version
+        <> " but tried to decode script version: "
+        <> pshow v


Suggested change

"Version mismatch in code: script version that was read"

<> pretty version

<> " but tried to decode script version: "

<> pshow v

"Version mismatch in code: script version that was read"

<+> pretty version

<+> "but tried to decode script version:"

<+> pshow v

cardano-cli/src/Cardano/CLI/Types/Errors/ScriptDataError.hs

cardano-cli/src/Cardano/CLI/Types/Errors/ScriptDecodeError.hs

carbolymer · 2024-11-22T11:29:57Z

cardano-cli/src/Cardano/CLI/Types/Common.hs

+-- which says nothing about the script type (simple vs plutus) or script version.
+-- As a result need to separate the different script purposes into
+-- their own separate data definitions where we can make changes specific to that script purpose
+-- more easily without affecting the rest of the api.


Beautiful 🤩

carbolymer · 2024-11-22T22:02:58Z

cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs

@@ -1230,9 +1248,9 @@ getAllReferenceInputs
      :: ScriptWitness witctx era -> Maybe TxIn
    getReferenceInput sWit =
      case sWit of
-        PlutusScriptWitness _ _ (PReferenceScript refIn _) _ _ _ -> Just refIn
+        PlutusScriptWitness _ _ (PReferenceScript refIn) _ _ _ -> Just refIn


This function can be removed, as this functionality is provided by getScriptWitnessReferenceInput from cardano-api.

Jimbo4350 marked this pull request as ready for review November 18, 2024 19:52

Jimbo4350 requested review from newhoggy, carbolymer, smelc, palas, kevinhammond and disassembler as code owners November 18, 2024 19:52

carbolymer mentioned this pull request Nov 19, 2024

Refactor TxMintValue IntersectMBO/cardano-api#663

Merged

3 tasks

carbolymer reviewed Nov 19, 2024

View reviewed changes

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch from 708410b to 672b332 Compare November 20, 2024 19:29

carbolymer reviewed Nov 20, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/Read.hs Outdated Show resolved Hide resolved

carbolymer approved these changes Nov 20, 2024

View reviewed changes

smelc and others added 11 commits November 20, 2024 21:37

Use new inject function instead of the XToY era functions

a272b0b

Remove use of experimental api in transaction balancing

ea0476c

Bump cardano-api-10.3

3e9a647

Add Cardano.CLI.Plutus.Minting module

3a164af

Define the data definition CliMintScriptRequirements This type makes it clearer that we require the policy id for transaction construction when using a minting script

Refine plutusP to return AnyPlutusScriptVersion instead of

8426cdb

AnyScriptLanguage Replace ScriptWitnessFiles WitCtxMint with CliMintScriptRequirements

Modify PlutusReferenceScriptWitnessFiles to accept

d4d3753

AnyPlutusScriptVersion instead of AnyScriptLanguage

Replace ScriptWitnessFiles WitCtxMint with CliMintScriptRequirements

5c276ae

Propagate MintScriptWitWithPolId era

d7a6085

Remove gatherMintingWitnesses because by default we require a PolicyId

e567a54

Add TxCmdCliScriptWitnessError to TxCmdError

34281ab

Add ScriptDecodeUnknownPlutusScriptVersion to ScriptDecodeError

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch from 672b332 to 1d7728e Compare November 21, 2024 15:31

Jimbo4350 requested review from a team as code owners November 21, 2024 15:31

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch from 1d7728e to d09f60b Compare November 21, 2024 15:52

Jimbo4350 added 2 commits November 21, 2024 11:52

Review rename and refactor suggestions

8453b12

Module split and rename suggestions

fafcea1

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch from d09f60b to 4ff64a8 Compare November 21, 2024 15:53

smelc mentioned this pull request Nov 21, 2024

Use new inject function instead of the XToY era functions #969

Closed

2 tasks

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch from 4ff64a8 to 424ad7b Compare November 21, 2024 16:11

Add separate error modules:

6c4869f

- Cardano.CLI.Types.Errors.PlutusScriptDecodeError - Cardano.CLI.Types.Errors.ScriptDataError

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch 4 times, most recently from 7e08bf1 to c157954 Compare November 21, 2024 16:54

carbolymer reviewed Nov 21, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/Types/Common.hs Outdated Show resolved Hide resolved

carbolymer approved these changes Nov 21, 2024

View reviewed changes

Jimbo4350 and others added 3 commits November 21, 2024 15:07

Update createTxMintValue and propagate the removal of Maybe PolicyId

1e136a3

from PReferenceScript and SReferenceScript constructors Co-authored-by: Mateusz Gałażyn <[email protected]>

Formatting fixes

5649c78

Jimbo4350 force-pushed the jordan/minting-script-witness-refactor branch from c157954 to 5649c78 Compare November 21, 2024 19:10

smelc approved these changes Nov 22, 2024

View reviewed changes

carbolymer reviewed Nov 22, 2024

View reviewed changes

smelc mentioned this pull request Nov 25, 2024

Add support for scripts in fromProposalProcedure #978

Closed

2 tasks

Jimbo4350 added this pull request to the merge queue Nov 26, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2024

smelc added this pull request to the merge queue Nov 26, 2024

Merged via the queue into master with commit 308ce97 Nov 26, 2024
25 checks passed

smelc deleted the jordan/minting-script-witness-refactor branch November 26, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minting script witness refactor #971

Minting script witness refactor #971

Jimbo4350 commented Nov 18, 2024 •

edited

Loading

carbolymer left a comment

carbolymer Nov 19, 2024

Jimbo4350 Nov 20, 2024

carbolymer Nov 20, 2024

carbolymer Nov 19, 2024

Jimbo4350 Nov 20, 2024 •

edited

Loading

carbolymer Nov 20, 2024 •

edited

Loading

smelc Nov 22, 2024

carbolymer Nov 22, 2024

carbolymer Nov 22, 2024 •

edited

Loading

Minting script witness refactor #971

Minting script witness refactor #971

Conversation

Jimbo4350 commented Nov 18, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

carbolymer left a comment

Choose a reason for hiding this comment

carbolymer Nov 19, 2024

Choose a reason for hiding this comment

Jimbo4350 Nov 20, 2024

Choose a reason for hiding this comment

carbolymer Nov 20, 2024

Choose a reason for hiding this comment

carbolymer Nov 19, 2024

Choose a reason for hiding this comment

Jimbo4350 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

carbolymer Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

smelc Nov 22, 2024

Choose a reason for hiding this comment

carbolymer Nov 22, 2024

Choose a reason for hiding this comment

carbolymer Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 commented Nov 18, 2024 •

edited

Loading

Jimbo4350 Nov 20, 2024 •

edited

Loading

carbolymer Nov 20, 2024 •

edited

Loading

carbolymer Nov 22, 2024 •

edited

Loading