-
Notifications
You must be signed in to change notification settings - Fork 162
Fix JSON parsing for CostModels #5241
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
Conversation
36ed223
to
e30c886
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.
Both the test failures pertain to the change in the parameter name counts from 231 to 251.
Looks good to me otherwise 👍
The problem lies in the fact that starting with Conway era number of cost model can vary, while number of parameters in any genesis file must stay the same, since that was the number of parameters with which era was initiated with. This PR: * Fixes the parsing where addition of new cost model parameters in a newer version of plutus results in a failure, unless new parameters are added to the genesis file, which would be a wrong thing to do. * Fixes the total number of parameters with which Conway era has started with. This was a not really a problem, since parsing for cost model parameters in Conway did not enforce the initial number * Start enforcing the initial number of parameters in the Conway Genesis
e30c886
to
2e9055e
Compare
@aniketd Would you mind reviewing my test fix, if you're still around? |
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.
Interesting that the JSON deserialization of CostModels has so much more complexity compared to the CBOR one (untouched by this change), with a wider definition of what "lenient" means. I suppose because the JSON in the genesis file) is more error prone ? Whereas for CBOR we know what we serialize.
Anyway, looks good to me.
@teodanciu LGTM! 🙌 |
No, it is because JSON for CostModel in Genesis file must be stable (i.e. Genesis files never change on mainnet), while JSON in LedgerState and CBOR in transaction are more versatile. Hence is the distinction between lenient and strict. |
Description
JSON parsing for CostModel is incorrect today. The problem lies in the fact that starting with Conway era number of cost model can vary, while number of parameters in any genesis file must stay the same, since that was the number of parameters with which era was initiated with.
Confirmation of the problem and the fix is in a separate PR due to the amount of changes it required: #5243
This PR:
Checklist
CHANGELOG.md
files updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabal
andCHANGELOG.md
files when necessary, according to theversioning process.
.cabal
files updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh
).scripts/cabal-format.sh
).scripts/gen-cddl.sh
)hie.yaml
updated (usescripts/gen-hie.sh
).