Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Test SPO vote counting #4700

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Test SPO vote counting #4700

wants to merge 4 commits into from
+602 −0

Conversation

Lucsanszky
Copy link
Contributor

@Lucsanszky Lucsanszky commented Oct 17, 2024 •
edited by lehins
Loading

Description

Follow-up for #4659
Resolves #4617
Resolves #4727

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
    New section is never added with the code changes. (See RELEASING.md)
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .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)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@Lucsanszky Lucsanszky force-pushed the ldan/spo-voting-tests branch 4 times, most recently from 2cde68d to d7e9320 Compare October 24, 2024 18:23
@Lucsanszky Lucsanszky force-pushed the ldan/spo-voting-tests branch 9 times, most recently from bf58a41 to 913d87d Compare November 1, 2024 09:15
@Lucsanszky Lucsanszky force-pushed the ldan/spo-voting-tests branch 2 times, most recently from 0d7381b to c5c631d Compare November 2, 2024 03:57
@Lucsanszky Lucsanszky marked this pull request as ready for review November 2, 2024 18:16
@Lucsanszky Lucsanszky requested a review from a team as a code owner November 2, 2024 18:16
aniketd
aniketd reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@aniketd aniketd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor suggestions. LGTM! 👍

eras/conway/impl/test/Test/Cardano/Ledger/Conway/SPORatifySpec.hs Outdated Show resolved Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/SPORatifySpec.hs Outdated Show resolved Hide resolved
eras/conway/impl/test/Test/Cardano/Ledger/Conway/SPORatifySpec.hs Outdated Show resolved Hide resolved
@Lucsanszky Lucsanszky force-pushed the ldan/spo-voting-tests branch 2 times, most recently from fe2d6d0 to b0bc17c Compare November 5, 2024 09:56
@Lucsanszky
Copy link
Contributor Author

Note to whoever takes this over as I'm off this week: I reinstated the previously removed tests in the last commit. I only adjusted them minimally, so they pass. I think the one with the NoConfidence action could be removed as I often used NoConfidence in the Imp tests to check the vote counting behaviour. The committee update one could stay or maybe adjusted to better fit the recently added tests. I also wouldn't be against dropping that commit altogether and not reinstating those old tests at all because committee updates are also covered by the newly added acceptedRatioProp as that could generate such cases.

@Lucsanszky Lucsanszky force-pushed the ldan/spo-voting-tests branch 2 times, most recently from 4de44d3 to d8d0f53 Compare November 13, 2024 12:24
@Lucsanszky
Copy link
Contributor Author

Regarding #4727, I dropped the NoConfidence test since we cover that case quite extensively and I changed one of the newly added tests to cover the CommitteeUpdate case. I think this is now good to go, so I'll enable auto-merge and wait for approval.

lehins
lehins approved these changes Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Thank you!

@Lucsanszky
Test SPO voting threshold properties
e929387 
Resolves #4617
@Lucsanszky
Add delegateSPORewardAddressToDRep_ helper
544584f
@Lucsanszky
Test SPO default voting during bootstrap
a788c8d 
  * Assert that the default vote is `Abstain` in general
  * Assert that `HardForkInitiation` is an exception to the above
    and default vote is `No` in this case
@Lucsanszky
Test SPO default voting after bootstrap
76e800a 
  * Assert that default vote is `No` in general
  * Assert that default vote is `No` for `HardForkInitiation`
    and this cannot be overruled by the new default vote customisation
  * Assert that the default vote can be customised by delegating
    a pool's reward account to:
      - An `AlwaysNoConfidence` DRep, in which case the default vote
        becomes `Yes` for a `NoConfidence` action
      - An `AlwaysAbstain` DRep, in which case the default vote
        become `Abstain` in general
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lehins lehins lehins approved these changes

@aniketd aniketd Awaiting requested review from aniketd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bring back couple of Imp tests that check SPO voting Add threshold property tests for SPO voting
3 participants
@Lucsanszky @aniketd @lehins