-
Notifications
You must be signed in to change notification settings - Fork 23
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 fields only if they make sense for the given era #40
Generate fields only if they make sense for the given era #40
Conversation
fa6a8f7
to
b89b3b2
Compare
-- The Babbage and subsequent eras support such a protocol parameter. | ||
-- | ||
data ProtocolUTxOCostPerWordFeature era where | ||
ProtocolUpdateUTxOCostPerWordInAlonzoEra :: ProtocolUTxOCostPerWordFeature AlonzoEra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how to define a "feature". This is exactly the way we currently define features except the naming convention for them is "SupportedInEra". This PR proposes to use the word Feature
in place of SupportedInEra
because it is shorter.
MaryEra -> no | ||
AlonzoEra -> yes ProtocolUpdateUTxOCostPerWordInAlonzoEra | ||
BabbageEra -> no | ||
ConwayEra -> no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every feature defines a FeatureInEra
type-class instance.
featureInEra
uses the same convention as the function bool
and maybe
in the standard library in that those functions take the negative case before the positive case.
b89b3b2
to
d4bc2a2
Compare
@@ -917,7 +933,7 @@ genProtocolParametersUpdate = do | |||
protocolUpdatePoolPledgeInfluence <- Gen.maybe genRationalInt64 | |||
protocolUpdateMonetaryExpansion <- Gen.maybe genRational | |||
protocolUpdateTreasuryCut <- Gen.maybe genRational | |||
protocolUpdateUTxOCostPerWord <- Gen.maybe genLovelace | |||
protocolUpdateUTxOCostPerWord <- featureInEra @ProtocolUTxOCostPerWordFeature (pure Nothing) (const (Just <$> genLovelace)) era |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main part of the PR is implemented here and similar.
We only generate values for fields if they make sense for the given era.
Note, this code is not yet type-safe because the type of protocolUpdateUTxOCostPerWord
and protocolUpdateUTxOCostPerByte
are both Maybe Lovelace
and there is no defence against accidentally confusing the two in the code.
The intent is to eventually define these fields like this:
protocolUpdateUTxOCostPerWord :: FeatureValue ProtocolUTxOCostPerWordFeature era Lovelace
protocolUpdateUTxOCostPerByte :: FeatureValue ProtocolUTxOCostPerByteFeature era Lovelace
This would make the code type-safe because the code that checks the relevant feature for the field would not compile if they are accidentally mismatched.
Unfortunately this is not implemented here because doing so requires ProtocolParameters
type to take an era
type argument, but there are unresolved issues with making that change for reasons unrelated to this PR, but hopefully they can be resolved in consultation with the relevant people.
d4bc2a2
to
08d226d
Compare
-> f (FeatureValue feature era a) | ||
genFeatureValueInEra gen = | ||
featureInEra (pure NoFeatureValue) $ \witness -> | ||
pure NoFeatureValue <|> fmap (FeatureValue witness) gen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not yet used because the relevant fields are not yet using the FeatureValue
type, but it will be once that change is made (possibly in a future PR).
The advantage of having a genFeatureValueInEra
is that using it is less error prone than exist practise which is to define per-feature generators manually. It is very easy to accidentally not return NoFeatureValue
in the case where the feature is supported in the given era in implementation of those manually written functions and that bug currently exists in the code.
@@ -29,7 +31,8 @@ prop_roundtrip_praos_nonce_JSON = H.property $ do | |||
|
|||
prop_roundtrip_protocol_parameters_JSON :: Property | |||
prop_roundtrip_protocol_parameters_JSON = H.property $ do | |||
pp <- forAll genProtocolParameters | |||
AnyCardanoEra era <- forAll $ Gen.element [minBound .. maxBound] | |||
pp <- forAll (genProtocolParameters era) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having generators be era-sensitive improves our test coverage because we can now test across eras.
08d226d
to
17de407
Compare
17de407
to
cc83938
Compare
…ha-on-fork-prs Enable CI in PRs from forks
Description
Introduce the
FeatureInEra
type class. This type class will be used by "feature" types to define a way to determine if that feature is supported in an era.Introduce the
FeatureValue
type. This type is meant to be used to define data types with era-sensitive type-safe fields.This PR does not yet use
FeatureValue
to implement type-safe fields for theProtocolParameter
type because doing so requires thatProtocolParameter
take anera
type argument, and there are challenges to doing that as this time.The changes to introduce type-safe fields will be introduced in #39 when some other issues have been resolved.
More explanation is made inline.
Changelog
Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7
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.