-
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
Remove code repetition in Conway era CDDL #4178
Conversation
I'm not convinced of the benefits of this change. On the other hand, defining protocol_version itself as a singleton list doesn't seem right either - whereas it would reduce duplication, it can be misleading when reading the types that are referencing it. But maybe I'm missing something - is this duplication creating problems in some ways that are not obvious? Just curious |
From this sentence it seems there are no reasons to keep unchanged definitions between eras; if I am correct, I would prefer this option both for shortness and for clarity.
I can't get the point here. Wouldn't it be just as any other defined type? Could you please elaborate? Thank you
For sure! Maybe
In my specific case, I spent more or less one hour trying to understand the reason for this repetition.. and I ended up opening this PR. Thank you |
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 don't know if there are some reasons to keep the definitions unchanged between CDDL versions; if that's not the case and I was too scrupulous, we could simplify this PR removing protocol_version_ and changing protocol_version definition.
I am all in favor of this suggestion.
Please let me elaborate my thought about this question. This document is not an end in itself, it is the refernce for thousands of other projects. For this reason, in my opinion, it should be kept as clear and minimal as possible. If I may allow one more suggestion, I would move the big comment it has in its middle to a |
Thank you for the explanation @iccicci, I concede to the non-indirection approach that you and Alexey suggested, as my concern seems indeed unfounded. |
Rebased against master |
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.
Looks good to me
Includes: * IntersectMBO/cardano-ledger#4003 * IntersectMBO/cardano-ledger#4178 * IntersectMBO/cardano-ledger#4055 * IntersectMBO/cardano-ledger#4117 * IntersectMBO/cardano-ledger#4040 TODO: [ ] need to check how this affects the script data hash and test that [ ] multi-era
* New Conway.cddl updates (cml-chain/cml-chain-wasm) Includes: * IntersectMBO/cardano-ledger#4003 * IntersectMBO/cardano-ledger#4178 * IntersectMBO/cardano-ledger#4055 * IntersectMBO/cardano-ledger#4117 * IntersectMBO/cardano-ledger#4040 TODO: [ ] need to check how this affects the script data hash and test that [ ] multi-era * conway multi-era update + various fixes * resolve merge conflict * use conway costmodels for all eras (babbage costmdls found in alonzo on sancho testnet) * IntoIter for NonemptySet<T>
Description
protocol_version
is defined as a group and always referred nested (alone) inside an array.I introduced
protocol_version_
with the purpose to not changeprotocol_version
definition: I don't know if there are some reasons to keep the definitions unchanged between CDDL versions; if that's not the case and I was too scrupulous, we could simplify this PR removingprotocol_version_
and changingprotocol_version
definition.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
)