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.

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

Redo how we manage deposits #3127

Merged
merged 2 commits into from
Dec 12, 2022
Merged

Redo how we manage deposits #3127

merged 2 commits into from
Dec 12, 2022

Conversation

TimSheard
Copy link
Contributor

@TimSheard TimSheard commented Nov 4, 2022

This PR explores one mechanism for tracking deposits for each Stake credential and each Pool.
Motivated by #3113

Here is what happened
Added deposits (key deposits) to DState and (pool deposits) PState.
Functions that computed deposits, obligation, and refunds (keyRefunds
and totalDeposits, obigation etc) were changed to take DPState as input
Added DPState to UtxoEnv replacing pool stake. Incorporated TreeDiff
to show differences in two NewEpochState's. Gathered all the TreeDiff
stuff into one module Cardano.Ledger.TreeDiff in cardano-binary. This
includes all needed orphan instances from external types. Importing this module
should supply everything needed, and carry the orphan instances.
Added (.->>) as a version of (.->) that uses tree diff in small-steps-test
Added property tests that test the new invariants that must hold
1) utxosDepoits == sum (dsDeposited) + sum (psDeposited)
2) dom rewards == dom dsDeposited

@TimSheard TimSheard force-pushed the ts-deposits-as-map branch 3 times, most recently from 06cd38d to 0243f57 Compare November 22, 2022 03:06
@TimSheard TimSheard force-pushed the ts-deposits-as-map branch 2 times, most recently from f4f60f8 to 18e42dc Compare December 2, 2022 20:58
@TimSheard TimSheard changed the title WIP Rethink how we do deposits Redo how we manage deposits Dec 2, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
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.

I haven't looked through the full PR yet, here are my comments thus far

.gitignore Outdated Show resolved Hide resolved
libs/cardano-ledger-binary/src/Cardano/Ledger/TreeDiff.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-binary/src/Cardano/Ledger/TreeDiff.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-binary/src/Cardano/Ledger/TreeDiff.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-binary/cardano-ledger-binary.cabal Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@teodanciu
Copy link
Contributor

Went through all the code, added some small remarks and asked (likely stupid) questions mostly for my education .
Had no idea this change would impact so much of the codebase and how many things would have to be adapted to the change, a lot of work!

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.

Here are some more comments and suggestions

I don't fully grasp the logic in this PR, since I had limited exposure to staking and delegations, however from what I do understand I don't see any real problems.

@TimSheard TimSheard force-pushed the ts-deposits-as-map branch 2 times, most recently from 2ade737 to 0005baa Compare December 8, 2022 11:30
@TimSheard TimSheard force-pushed the ts-deposits-as-map branch 2 times, most recently from b178fd1 to a66d4d3 Compare December 8, 2022 14:05
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.

👍

.gitignore Outdated Show resolved Hide resolved
functions that computed deposits, obligation, and refunds (keyRefunds
and totalDeposits, obigation etc) were changed to take DPState as input
Added DPState to UtxoEnv replacing pool stake. Incorporated TreeDiff
to show differences in two NewEpochState's. Gathered all the TreeDiff
stuff into one module Cardano.Ledger.TreeDiff in cardano-binary. This
includes all needed orphan instances from external types. Importing this module
should supply everything needed, and carry the orphan instances.
Added (.->>) as a version of (.->) that uses tree diff in small-steps-test
Added property tests that test the new invariants that must hold
1) utxosDepoits == sum (dsDeposited) + sum (psDeposited)
2) dom rewards == dom dsDeposited
Added Cardano.Ledger.Shelley.Internal, module to be used when debugging.
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.

Awesome work!!!

@TimSheard TimSheard merged commit 546b83d into master Dec 12, 2022
@iohk-bors iohk-bors bot deleted the ts-deposits-as-map branch December 12, 2022 14:37
@TimSheard TimSheard mentioned this pull request Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants