-
Notifications
You must be signed in to change notification settings - Fork 158
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
Proposal deposits in SPO voting stake #4324
Proposal deposits in SPO voting stake #4324
Conversation
cbc7ebb
to
97d9375
Compare
97d9375
to
c77828b
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.
The solution is solid. There are a few minor improvements that we can make, other than that this PR looks great.
eras/shelley/test-suite/test/Test/Cardano/Ledger/Shelley/Examples/PoolLifetime.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState/PulsingReward.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/src/Cardano/Ledger/Conway/Governance/DRepPulser.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/src/Cardano/Ledger/Conway/Governance/DRepPulser.hs
Outdated
Show resolved
Hide resolved
c77828b
to
21f4b2b
Compare
eras/conway/impl/src/Cardano/Ledger/Conway/Governance/DRepPulser.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/src/Cardano/Ledger/Conway/Governance/DRepPulser.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/Vars.hs
Outdated
Show resolved
Hide resolved
425a7e7
to
7a877b2
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.
PR looks great. Just one comment that we've discussed on a call
* Add individualPoolStakeCoin field to IndividualPoolStake as numerator. * Add pdTotal field to PoolDistr as denominator. * Change calculatePoolDistr' to include the numerator and denominator. * Update computeDRepDistr to include updating PoolDistr numerator and denominator with proposal-deposits. * Update spoAcceptedRatio to calculate in terms of the numerator/ denominator pair instead of Rational.
We reset the newly added fields: individualTotalPoolStake and pdTotalActiveStake, in the ledger state that testCHAINEXample compares with checkTrace, so as to not interfere with the chained ledger states in these tests.
The prop_RATIFY test revealed a bug in the current implementation of spoAcceptedRatio that constructs a `Ratio` out of the numerator and denominator of the PoolDistr. We use the same method of defaulting it to unit if it is zero.
196f4c1
to
00a0970
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.
LGTM!
9920384
to
0a1cc62
Compare
Description
Resolves #4061
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
)