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.

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

Convert to use typesafe features in data types #37

Closed
wants to merge 3 commits into from
Closed

Convert to use typesafe features in data types #37

wants to merge 3 commits into from

Conversation

newhoggy
Copy link
Collaborator

@newhoggy newhoggy commented Jun 5, 2023

Description

Add your description here, if it fixes a particular issue please provide a
link
to the issue.

Changelog

- description: |
    <insert-changelog-description-here>
  # no-changes: the API has not changed
  # compatible: the API has changed but is non-breaking
  # breaking: the API has changed in a breaking way
  compatibility: <no-api-changes|compatible|breaking>
  # feature: the change implements a new feature in the API
  # bugfix: the change fixes a bug in the API
  # test: the change fixes modifies tests
  # maintenance: the change involves something other than the API
  # If more than one is applicable, it may be put into a list.
  type: <feature|bugfix|test|maintenance>

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • The change log section in the PR description has been filled in
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@newhoggy newhoggy changed the title Newhoggy/new feature module New typesafe feature module Jun 5, 2023
carbolymer
carbolymer reviewed Jun 5, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/TxBody.hs
txScriptValidityToIsValid :: TxScriptValidity era -> L.IsValid
txScriptValidityToIsValid = scriptValidityToIsValid . txScriptValidityToScriptValidity
data TxScriptValidityFeature era where
TxScriptValiditySupportedInAlonzoEra :: TxScriptValidityFeature AlonzoEra
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to have a constructor for each pair feature-era?

Copy link
Collaborator Author

@newhoggy newhoggy Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note, I've not changed the code. I've only renamed TxScriptValiditySupportedInEra to TxScriptValidityFeature.

The rename is a suggestion because Feature is shorter than SupportedInEra.

As for why the existing code does this.

The constructor serves as a witness that the pair is valid. If someone wants to construct a FeatureValue for a particular feature/era pair they must supply the witness.

The witness proves the pair is valid and if one cannot be supplied, that means the pair is invalid.

This is how invalid pairs are made into compile errors making the code type-safe.

Copy link
Contributor

@carbolymer carbolymer Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So the type level checks would be enough, right? So why not use phantom parameters:

data Feature era feature = Feature

You could still have compile-time checks without manually creating a lot of constructors.

carbolymer
carbolymer reviewed Jun 5, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/Feature.hs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add haddocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I plan to add haddocks as soon as I verify the solution is viable for protocol parameters as well.

@newhoggy newhoggy force-pushed the newhoggy/new-feature-module branch from f8ba1e9 to d7b9064 Compare June 6, 2023 01:59
@newhoggy newhoggy requested a review from carbolymer June 6, 2023 02:06
@newhoggy newhoggy force-pushed the newhoggy/new-feature-module branch 2 times, most recently from 09cce23 to 80f3d5e Compare June 9, 2023 02:28
@newhoggy newhoggy force-pushed the main branch 2 times, most recently from 4e5cfdd to 1abeff1 Compare June 14, 2023 11:20
@newhoggy newhoggy force-pushed the newhoggy/new-feature-module branch from 80f3d5e to 8738685 Compare June 22, 2023 12:23
@newhoggy newhoggy changed the title New typesafe feature module Convert to use typesafe features in data types Jun 22, 2023
@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Aug 15, 2023
@newhoggy newhoggy closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer approved these changes

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

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

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

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

Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@newhoggy @carbolymer