-
Notifications
You must be signed in to change notification settings - Fork 156
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
Babbage / Conway PParams JSON serialization #3953
Conversation
a48ec37
to
140a523
Compare
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.
I think it would be nice to add to Babbage changelog that protocolVersion issue is fixed.
Two issues on CI:
Golden test for ConwayGenesis
The conway-genesis.json
need to change, since FromJSON
was wrongfully derived for threasholds and you fixed it in the code (thank you!):
{
"poolVotingThresholds": {
- "pvtCommitteeNormal": 0,
- "pvtCommitteeNoConfidence": 0,
- "pvtHardForkInitiation": 0,
- "pvtMotionNoConfidence": 0
+ "committeeNormal": 0,
+ "committeeNoConfidence": 0,
+ "hardForkInitiation": 0,
+ "motionNoConfidence": 0
},
"dRepVotingThresholds": {
- "dvtMotionNoConfidence": 0,
- "dvtCommitteeNormal": 0,
- "dvtCommitteeNoConfidence": 0,
- "dvtUpdateToConstitution": 0,
- "dvtHardForkInitiation": 0,
- "dvtPPNetworkGroup": 0,
- "dvtPPEconomicGroup": 0,
- "dvtPPTechnicalGroup": 0,
- "dvtPPGovGroup": 0,
- "dvtTreasuryWithdrawal": 0
+ "motionNoConfidence": 0,
+ "committeeNormal": 0,
+ "committeeNoConfidence": 0,
+ "updateToConstitution": 0,
+ "hardForkInitiation": 0,
+ "ppNetworkGroup": 0,
+ "ppEconomicGroup": 0,
+ "ppTechnicalGroup": 0,
+ "ppGovGroup": 0,
+ "treasuryWithdrawal": 0
},
CostModels in PParams
We need to adjust how we deal with problematic CostModels, since currently they do not roundtrip. I have an idea on what we can do about this, so I'll submit a separate PR that fixes that.
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/JSON.hs
Outdated
Show resolved
Hide resolved
@lehins Fixed the golden |
This will also be useful to test other JSON roundtrips.
This also adds a JSON PParams roundtrip test for Alonzo.
When adding a FromJSON for Conway PParams and testing the roundtrip, it turned out that the JSON instances were not aligned correctly.
This fixes most of the roundtrips for Conway PParams, but seemingly something changed on the cost models still.
Co-authored-by: Alexey Kuleshevich <alexey.kuleshevich@iohk.io>
This was using the field names from the haskell type and are fixed now to be roundtripping without the prefixes.
The serialization of included cost models is problematic.
9e70706
to
1bad44d
Compare
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.
That's perfect. Thank you!
Description
This adds JSON deserialization for
Babbage
andConway
PParams
and is an extension of #3949.Creates
Test.Cardano.Ledger.Core.JSON
with aToJSON -> FromJSON
roundtripFixes the
ToJSON
instance ofBabbagePParams
which was missing aprotocolVersion
Adds JSON roundtrip tests to Alonzo, Babbage and Conway PParams.
Adds
protocolVersion
also toConway
PParams
serialization.Updated the golden
genesis-conway.json
asFromJSON
instances are non-derived now and match theToJSON
instances forPoolVotingThresholds
andDRepVotingThresholds
Checklist
.cabal
andCHANGELOG.md
files according to theversioning process.
.cabal
files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)CHANGELOG.md
for the affected packages. New section is never added with the code changes. (See RELEASING.md)fourmolu
(usescripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
has been updated (usescripts/gen-hie.sh
)