-
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
Constitutional Committee Ratification #3745
Conversation
d9ca00d
to
9b951e5
Compare
Yeah, that comment on the ticket was wrong. I've fixed it. All expired and resigned CC members are considered as if they abstained: |
a335e65
to
1a68ef9
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.
Majority of my comments are about how the test suite can be improved.
From what I can see nothing is blocking this PR from being merged.
Great work!!!
eras/conway/impl/test/Test/Cardano/Ledger/Conway/CommitteeRatifySpec.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/CommitteeRatifySpec.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/CommitteeRatifySpec.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/CommitteeRatifySpec.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/CommitteeRatifySpec.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/CommitteeRatifySpec.hs
Outdated
Show resolved
Hide resolved
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.
Spoke too soon 😁
Latest changes made it wrong After looking a bit closer, they are actually fine.
30bb334
to
0bcea3b
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.
Looks great. Thank you for doing this work!
in order to ratify committee votes
as a mandatory check in `Ratify` rule
0bcea3b
to
16f048e
Compare
to make the specialization clear and for uniformity with `CommitteeRatifySpec`
instead rely on the threshold calculation to cause a "no" vote from the committee in this case
16f048e
to
9523c63
Compare
Description
Adds constitutional committee ratification as a mandatory check in
Ratify
.Closes #3701
Closes #3444
The structure of the code is very similar to drep` ratification, so we could abstract it.
Missing:
Fix ConwayFeatures testEDIT: Fixedthere is a contradiction between a comment on the issue and (what I think I see in ) the specEDIT: clarifiedsome unseemly duplication in the test, should find a better way to set expired or resigned members with less noiseEDIT: refactored to remove the duplication!Might be just a problem of semantics, but in this PR, I wouldn't say that expired and resigned "contribute to the ratio" (unless we interpret that as: because they're expired or resigned we don't count them at all..)
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
)