Skip to content
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

Updates cardano-cli to not use PReferenceScript/SReferenceScript ScriptHash field #918

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Swordlash
Copy link

Changelog

- description: Updates cardano-cli to not use PReferenceScript/SReferenceScript ScriptHash field
  type:
  - compatible
  - refactoring

Context

See the upstream PR.
The Maybe ScriptHash field in P/SReferenceScript is redundant in cardano-api, causing confusion. Nonetheless it's used in cardano-cli as a temporary container for user-provided input. This PR refactors it out so the field can be safely removed from upstream.

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

Comment on lines 1360 to 1361
gatherMintingWitnesses ((pid'm, sWit) : rest) =
case scriptWitnessPolicyId pid'm sWit of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
gatherMintingWitnesses ((pid'm, sWit) : rest) =
case scriptWitnessPolicyId pid'm sWit of
gatherMintingWitnesses ((mPid, sWit) : rest) =
case scriptWitnessPolicyId mPid sWit of

Rationale: having a quote in the middle of a variable name is surprising

Copy link
Author

@Swordlash Swordlash Sep 30, 2024

Choose a reason for hiding this comment

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

I picked up this style in Standard Chartered ('m for Maybe, 'e for Either). No issue, I will adhere to suggested style.

Copy link
Author

Choose a reason for hiding this comment

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

All updated, squashed and formatted.

@smelc
Copy link
Contributor

smelc commented Sep 30, 2024

@Swordlash> please format your changes with:

fourmolu -i ...modified hs files...
stylish-haskell -i ...modified hs files...

(in this order: fourmolu first, then stylish-haskell)

@Jimbo4350
Copy link
Contributor

@Swordlash thank you for bringing this to our attention. I have been aware of this for some time but haven't been able to dedicate the time to a solution. We're discussing a potential solution that involves modifying the cardano-api and avoids introducing Maybe PolicyId as it's done here. I've asked @carbolymer to work on it.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

We are discussing another way to modify cardano-api to avoid having to specify Maybe PolicyId.

@Swordlash
Copy link
Author

Thanks @Jimbo4350, lmk if you'd like me to do something, I'm prepared to dedicate some time for it this week (next weeks I'm away).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants