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

Improved feature ergonomics #128

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Conversation

newhoggy
Copy link
Collaborator

@newhoggy newhoggy commented Jul 21, 2023

Changelog

- description: |
    Improved feature ergonomics
    New:
    - `Featured`
    - `asFeaturedInEra`
    - `asFeaturedInShelleyBasedEra`
    - `genFeaturedInEra`
    - `genMaybeFeaturedInEra`
    - `conwayEraOnwardsToCardanoEra`
    - `conwayEraOnwardsToCardanoEra`
    - `conwayEraOnwardsToShelleyBasedEra`
    - `shelleyToBabbageEraConstraints`
    - `shelleyToBabbageEraToCardanoEra`
    - `shelleyToBabbageEraToShelleyBasedEra`
    Deprecated:
    - `FeatureValue` Use `Maybe Featured` instead
    - `isFeatureValue`
    - `valueOrDefault`
    - `asFeatureValue`
    - `asFeatureValueInShelleyBasedEra`
  compatibility: breaking
  type: feature

Context

Note, the following is an example only. We do not have to do the described refactoring until we need it.

Currently we define era-sensitive features like this:

data CollateralSupportedInEra era where

     CollateralInAlonzoEra  :: CollateralSupportedInEra AlonzoEra
     CollateralInBabbageEra :: CollateralSupportedInEra BabbageEra
     CollateralInConwayEra  :: CollateralSupportedInEra ConwayEra

deriving instance Eq   (CollateralSupportedInEra era)
deriving instance Show (CollateralSupportedInEra era)

collateralSupportedInEra :: CardanoEra era
                         -> Maybe (CollateralSupportedInEra era)
collateralSupportedInEra ByronEra   = Nothing
collateralSupportedInEra ShelleyEra = Nothing
collateralSupportedInEra AllegraEra = Nothing
collateralSupportedInEra MaryEra    = Nothing
collateralSupportedInEra AlonzoEra  = Just CollateralInAlonzoEra
collateralSupportedInEra BabbageEra = Just CollateralInBabbageEra
collateralSupportedInEra ConwayEra = Just CollateralInConwayEra

And then separately define era-sensitive values like this:

data TxInsCollateral era where

     TxInsCollateralNone :: TxInsCollateral era

     TxInsCollateral     :: CollateralSupportedInEra era
                         -> [TxIn] -- Only key witnesses, no scripts.
                         -> TxInsCollateral era

deriving instance Eq   (TxInsCollateral era)
deriving instance Show (TxInsCollateral era)

The TxInsCollateral can only be used with the witness CollateralSupportedInEra era which can only be constructed within certain eras.

This PR proposes to define like this instead:

data CollateralSupportedInEra era where

     CollateralInAlonzoEra  :: CollateralSupportedInEra AlonzoEra
     CollateralInBabbageEra :: CollateralSupportedInEra BabbageEra
     CollateralInConwayEra  :: CollateralSupportedInEra ConwayEra

deriving instance Eq   (CollateralSupportedInEra era)
deriving instance Show (CollateralSupportedInEra era)

collateralSupportedInEra :: CardanoEra era
                         -> Maybe (CollateralSupportedInEra era)
collateralSupportedInEra ByronEra   = Nothing
collateralSupportedInEra ShelleyEra = Nothing
collateralSupportedInEra AllegraEra = Nothing
collateralSupportedInEra MaryEra    = Nothing
collateralSupportedInEra AlonzoEra  = Just CollateralInAlonzoEra
collateralSupportedInEra BabbageEra = Just CollateralInBabbageEra
collateralSupportedInEra ConwayEra = Just CollateralInConwayEra

Then the definitions of data TxInsCollateral era is unnecessary. Once could just use Maybe (Featured CollateralSupportedInEra era [TxIn]. Just like TxInsCollateral the value can be absent, but using Nothing instead of TxInsCollateralNone.

Featured CollateralSupportedInEra era [TxIn] is better than data TxInsCollateral era in that the former can express that the value is required, which also forces the era to be one that the era supports. This will be useful in defining run commands where the arguments are required which currently cannot be done without defining additional types.

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
  • 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
  • The changelog section in the PR is updated to describe the change
  • 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 marked this pull request as ready for review July 21, 2023 12:24
{-# DEPRECATED asFeatureValueInShelleyBasedEra "Use asFeaturedInShelleyBasedEra instead" #-}

-- | A value only if the feature is supported in this era
data Featured feature era a where
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm skeptical about introducing another abstraction as we are in the process of handing over part of cardano-api to the ledger team. We don't want additional churn as we have an October target we need to hit. Where do you envision this being used?

Copy link
Collaborator Author

@newhoggy newhoggy Jul 21, 2023

Choose a reason for hiding this comment

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

I'm anticipating that we it will be used in the CLI parser.

For example we currently don't have a good way to say some value that's passed into the run function from the parser is mandatory but only for a particular era.

This change is also a simplification of existing code. Currently FeatureValue includes optionality, which I think is better modelled via Maybe.

I think this data type will also provide us a good way to shrink our API, which would mean less code to maintain and make the handover easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can postpone the merge of this PR until after the next cardano-api release.

@newhoggy newhoggy dismissed Jimbo4350’s stale review July 21, 2023 14:34

Counter argument

@newhoggy newhoggy requested a review from Jimbo4350 July 21, 2023 14:35
@newhoggy newhoggy force-pushed the newhoggy/improved-feature-ergonomics branch from caf19b3 to eeaa954 Compare July 25, 2023 07:11
@newhoggy newhoggy force-pushed the newhoggy/improved-feature-ergonomics branch from eeaa954 to fca2ac6 Compare July 25, 2023 07:48
@newhoggy newhoggy force-pushed the newhoggy/improved-feature-ergonomics branch from fca2ac6 to 24e6df1 Compare July 25, 2023 09:29
@newhoggy newhoggy added this pull request to the merge queue Jul 25, 2023
@newhoggy newhoggy removed this pull request from the merge queue due to a manual request Jul 25, 2023
@newhoggy
Copy link
Collaborator Author

All the non-breaking changes from #132 have been moved into this PR.

@newhoggy newhoggy added this pull request to the merge queue Jul 25, 2023
Merged via the queue into main with commit 2b6d95c Jul 25, 2023
22 checks passed
@newhoggy newhoggy deleted the newhoggy/improved-feature-ergonomics branch July 25, 2023 10:29
newhoggy pushed a commit that referenced this pull request Mar 11, 2024
newhoggy pushed a commit that referenced this pull request Mar 11, 2024
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.

None yet

3 participants