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

Generate protocolUpdateUtxoCostPerByte in genProtocolParametersUpdate era dependently #5070

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Apr 7, 2023

Depends on #5069

@newhoggy newhoggy changed the title Newhoggy/generate protocol update u tx o cost per byte in gen protocol parameters update era dependently Generate protocolUpdateUtxoCostPerByte in genProtocolParametersUpdate era dependently Apr 7, 2023
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch from 6794662 to 6bc5c3c Compare April 7, 2023 09:45
@newhoggy newhoggy marked this pull request as ready for review April 7, 2023 09:49
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 3 times, most recently from 4c7cc37 to 1bba825 Compare April 7, 2023 12:22
@newhoggy newhoggy requested a review from a team as a code owner April 7, 2023 12:22
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 3 times, most recently from 15feec0 to d24a256 Compare April 11, 2023 08:20
--
-- The Babbage and subsequent eras support such a protocol parameter.
--
data ProtocolUTxOCostPerByteSupportedInEra era where
Copy link
Contributor

Choose a reason for hiding this comment

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

So we actually explicitly avoided using GADTs for ProtocolParameter fields because it would result in an explosion of type definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added GADTs because that was the way we handle era support for things that aren't protocol parameters.

The cost will be (2 x numEras + 5) x numRelevantProtocolPameters. I don't yet have a good idea how many protocol parameters need this. At least each new protocol parameter will need this.

The ability to determine what protocol parameters are available for what era will be important to generators but I also suspect users of our API will find this useful too.

As for what alternative solutions are available, it will need to be one that allows us to express both the addition and removal of protocol parameters because new eras can and have removed protocol parameters.

If not expressed as types it could be expressed as a runtime value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For comparison:

  • Number of features: 24
  • Number of protocol parameters: 25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated PR with an more concise encoding. This means we write less code when adding a new parameter or feature.

We could also convert the existing feature supported in era code to use this new system instead which would reduce the amount of code.

@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch from 1841f7d to bf07a9f Compare April 13, 2023 01:28
{-# LANGUAGE GADTs #-}
{-# LANGUAGE StandaloneDeriving #-}

module Cardano.Api.Features
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This module introduces a new unified type-safe era-based feature flag system.


data ProtocolParameterUTxOCostPerWord

data ProtocolParameterUTxOCostPerByte
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each new feature is represented as an un-exported type.

Copy link
Contributor

@carbolymer carbolymer Apr 13, 2023

Choose a reason for hiding this comment

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

Maybe DataKinds would be better here to have a single kind for them?

-- | A representation of a feature that is supported in a given era.
data Feature f where
ProtocolParameterUTxOCostPerWord :: Feature ProtocolParameterUTxOCostPerWord
ProtocolParameterUTxOCostPerByte :: Feature ProtocolParameterUTxOCostPerByte
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Feature GADT has a constructor for each feature. This type is exported rather than the individual feature types.

deriving instance Show (Feature f)

instance ToJSON (Feature f) where
toJSON = toJSON . show
Copy link
Contributor Author

@newhoggy newhoggy Apr 13, 2023

Choose a reason for hiding this comment

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

Unifying the features into one type means we only need type class instances for the Feature type and not for each feature type.

ProtocolParameterUTxOCostPerWordSupportedInAlonzoEra :: SupportedInEra ProtocolParameterUTxOCostPerWord AlonzoEra

ProtocolParameterUTxOCostPerByteSupportedInBabbageEra :: SupportedInEra ProtocolParameterUTxOCostPerByte BabbageEra
ProtocolParameterUTxOCostPerByteSupportedInConwayEra :: SupportedInEra ProtocolParameterUTxOCostPerByte ConwayEra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SupportedInEra type allows us to only add code for when a feature is supported in an era. We don't have to add the negative cases.

Copy link
Contributor

@carbolymer carbolymer Apr 13, 2023

Choose a reason for hiding this comment

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

It's a just a cartesian product of eras and features in a form of a constructor for each, so it will be cumbersome to maintain I think. How about just a type class?

data ByronEra
data ShelleyEra
data AllegraEra
data MaryEra
data AlonzoEra
data BabbageEra
data ConwayEra

-- This can be turned into a single datatype using DataKinds
data ProtocolParameterUTxOCostPerWord
data ProtocolParameterUTxOCostPerByte
data UnknownFeature

data CardanoEra era where
     ByronEra   :: CardanoEra ByronEra
     ShelleyEra :: CardanoEra ShelleyEra
     AllegraEra :: CardanoEra AllegraEra
     MaryEra    :: CardanoEra MaryEra
     AlonzoEra  :: CardanoEra AlonzoEra
     BabbageEra :: CardanoEra BabbageEra
     ConwayEra  :: CardanoEra ConwayEra


data Feature f where
  ProtocolParameterUTxOCostPerWord :: Feature ProtocolParameterUTxOCostPerWord
  ProtocolParameterUTxOCostPerByte :: Feature ProtocolParameterUTxOCostPerByte
  UnknownFeature :: Feature UnknownFeature

class SupportedInEra f e where
  supportedInEra :: Feature f -> CardanoEra e -> Bool

instance {-# OVERLAPPABLE #-} SupportedInEra f e where
  supportedInEra _ _ = False

instance ProtocolParameterUTxOCostPerWord `SupportedInEra` ByronEra where
  supportedInEra _ _ = True

instance SupportedInEra ProtocolParameterUTxOCostPerByte ByronEra where
  supportedInEra _ _ = True

-- >>> supportedInEra ProtocolParameterUTxOCostPerWord ByronEra
-- True

-- >>> supportedInEra ProtocolParameterUTxOCostPerByte ByronEra
-- True

-- >>> supportedInEra UnknownFeature ByronEra
-- False

-- This can be simplified further with a phantom type, not requiring to create a new constructor for each
-- feature, which is already a type
data Feature' f = Feature'

class SupportedInEra' f e where
  supportedInEra' :: Feature' f -> CardanoEra e -> Bool

instance {-# OVERLAPPABLE #-} SupportedInEra' f e where
  supportedInEra' _ _ = False

instance SupportedInEra' ProtocolParameterUTxOCostPerWord ByronEra where
  supportedInEra' _ _ = True

instance SupportedInEra' ProtocolParameterUTxOCostPerByte ByronEra where
  supportedInEra' _ _ = True


-- >>> supportedInEra' (Feature' @ProtocolParameterUTxOCostPerByte) ByronEra
-- True

-- >>> supportedInEra' (Feature' @ProtocolParameterUTxOCostPerByte) ByronEra
-- True

-- >>> supportedInEra' (Feature' @UnknownFeature) ByronEra
-- False

we can simplify this even further with {-# LANGUAGE AllowAmbiguousTypes #-}:

class SupportedInEra'' f e where
  supportedInEra'' :: Bool

instance {-# OVERLAPPABLE #-} SupportedInEra'' a b where
  supportedInEra'' = False
instance  SupportedInEra'' ProtocolParameterUTxOCostPerByte ByronEra where
  supportedInEra'' = True

-- >>> supportedInEra'' @UnknownFeature @ByronEra
-- False

-- >>> supportedInEra'' @ProtocolParameterUTxOCostPerByte @ByronEra
-- True

Do we need features to be types?

supportedInEra ProtocolParameterUTxOCostPerWord AlonzoEra = Just ProtocolParameterUTxOCostPerWordSupportedInAlonzoEra

supportedInEra ProtocolParameterUTxOCostPerByte BabbageEra = Just ProtocolParameterUTxOCostPerByteSupportedInBabbageEra
supportedInEra ProtocolParameterUTxOCostPerByte ConwayEra = Just ProtocolParameterUTxOCostPerByteSupportedInConwayEra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The supportedInEra type allows us to only add code for when a feature is supported in an era. We don't have to add the negative cases.

protocolParamMaxValueSize <- Gen.maybe genNat
protocolParamCollateralPercent <- Gen.maybe genNat
protocolParamMaxCollateralInputs <- Gen.maybe genNat
protocolParamUTxOCostPerByte <- sequence $ supportedInEra ProtocolParameterUTxOCostPerByte era $> genLovelace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example of how supportedInEra is used.

@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 2 times, most recently from e4ea02a to b8d9232 Compare April 15, 2023 03:52
@newhoggy newhoggy enabled auto-merge April 15, 2023 03:53
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch from b8d9232 to 726ea6e Compare April 18, 2023 05:20
@newhoggy newhoggy requested a review from Jimbo4350 April 25, 2023 00:17
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 2 times, most recently from a1392d6 to dd8817d Compare April 27, 2023 01:23
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 2 times, most recently from 44704ba to cc0c164 Compare May 10, 2023 06:31
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch from cc0c164 to 75827e1 Compare May 11, 2023 14:45
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 2 times, most recently from 41ea6c3 to 6d86667 Compare May 19, 2023 13:03
@newhoggy newhoggy requested review from coot and a team as code owners May 19, 2023 13:03
@newhoggy newhoggy force-pushed the newhoggy/generate-protocolUpdateUTxOCostPerByte-in-genProtocolParametersUpdate-era-dependently branch 3 times, most recently from 2fc2c1e to 75827e1 Compare May 19, 2023 13:26
@newhoggy newhoggy closed this May 22, 2023
auto-merge was automatically disabled May 22, 2023 00:23

Pull request was closed

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