-
Notifications
You must be signed in to change notification settings - Fork 155
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
Fail PParamsUpdate deserialization for invalid costmodels in Conway #3861
Conversation
abd1c0c
to
c8874b4
Compare
4cd69de
to
c8874b4
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.
Not quite what needed to be done, but close. Test cases are very nice! Thank you for that
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
c8874b4
to
daeea08
Compare
b41863e
to
3d207b0
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.
You also need to update what sort of CostModel we generate fro PParamUpdates here: https://github.com/input-output-hk/cardano-ledger/blob/3d207b01dca78b231874ab27a2e016cf22753f86/eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Arbitrary.hs#L507
I suspect that is the reason for CI failures
3d97e8e
to
951a04f
Compare
303b817
to
19a24a4
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.
This is beautiful!
I love the extensive test suite for this functionality!
All of the comments I made are mere suggestions.
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/CostModelsSpec.hs
Outdated
Show resolved
Hide resolved
but collect costmodels for unknown languages
both independently and within PParamsUpdate
since the deserialization of invalid ones is failing
19a24a4
to
c7393c3
Compare
Description
Closes #3772
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
)