-
Notifications
You must be signed in to change notification settings - Fork 165
Store delegators in pool state #5316
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
base: master
Are you sure you want to change the base?
Conversation
b86f5cb
to
e358c21
Compare
4dea23b
to
f44c36b
Compare
7cf8b76
to
47d319c
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.
This is a great piece of work. There is only one real problem in this PR and the rest of the comments are things that I've notice that can be improved thanks to your new changes.
eras/shelley/test-suite/test/Test/Cardano/Ledger/Shelley/Examples/Combinators.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
cState | ||
& certDStateL . accountsL | ||
%~ adjustAccountState (stakePoolDelegationAccountStateL ?~ stakePool) stakeCred | ||
& certPStateL %~ adjustPState stakePool |
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.
This will require processDelegationInternal
to accept Maybe (AccountState era)
, instead of Maybe Delegatee
, which should be totally doable
& certPStateL %~ adjustPState stakePool | |
& certPStateL %~ unDelegReDelegStakePool cred accountState (Just stakePool) |
DelegStakeVote oldPool _ | Just oldPool /= mNewPool -> Just oldPool | ||
_ -> Nothing | ||
|
||
unDelegDRep :: |
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.
Instead of unDelegDRep
I propose to add the exact same function as I suggested for stake pools in the Shelley DELEG rule, but for DReps and move it to where VState is defined:
-- Update reverse DRep delegation Set whenever stake credential unregisters or changes its delegation. Whenever new delegation matches the old one this is a noop
unDelegReDelegDRep ::
Credential 'Staking ->
AccountState era ->
-- ^ Account that is loosing its current delegation and/or acquiring a new one
Maybe DRep ->
-- ^ Potential new delegation. In case when stake credential unregisters this must be `Nothing`.
VState era ->
VState era
unDelegReDelegDRep stakeCred accountState mNewDRep =
fromMaybe (vsDRepsL %~ addNewDelegation) $ do
dRep@(DRepCredential dRepCred) <- accountState ^. dRepDelegationAccountStateL
pure $
-- There is no need to update set of delegations if delegation is unchanged
if Just dRep == mNewStakePool
then id
else
vsDRepsL %~ addNewDelegation . Map.adjust (drepDelegsL %~ Set.delete stakeCred) dRepCred)
where
addNewDelegation =
case mNewDRep of
Just (DRepCredential dRepCred) ->
Map.adjust (drepDelegsL %~ Set.insert stakeCred) dRepCred
Nothing -> id
adjustPState newPool = | ||
(psStakePoolsL %~ Map.adjust (spsDelegsL %~ Set.insert stakeCred) newPool) | ||
. unDelegStakePool stakeCred mCurDelegatee (Just newPool) |
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.
No longer needed
adjustPState newPool = | |
(psStakePoolsL %~ Map.adjust (spsDelegsL %~ Set.insert stakeCred) newPool) | |
. unDelegStakePool stakeCred mCurDelegatee (Just newPool) |
cState | ||
& certDStateL . accountsL | ||
%~ adjustAccountState (dRepDelegationAccountStateL ?~ dRep) stakeCred | ||
& certVStateL %~ unDelegDRep stakeCred mCurDelegatee |
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.
& certVStateL %~ unDelegDRep stakeCred mCurDelegatee | |
& certVStateL %~ unDelegReDelDRep stakeCred mAccountState (Just dRep) |
dReps | ||
| preserveIncorrectDelegation = cState ^. certVStateL . vsDRepsL | ||
| otherwise = cState' ^. certVStateL . vsDRepsL |
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.
This will need to be slightly rewritten:
dReps | |
| preserveIncorrectDelegation = cState ^. certVStateL . vsDRepsL | |
| otherwise = cState' ^. certVStateL . vsDRepsL | |
in if preserveIncorrectDelegation | |
then cState & certVStateL %~ case dRep of | |
DRepCredential dRepCred -> | |
vsDRepsL %~ Map.adjust (drepDelegsL %~ Set.insert stakeCred) dRepCred | |
_ -> id | |
else cState' |
And all the stuff below is no longer needed.
f803f7e
to
75acf46
Compare
75acf46
to
4f3f4ba
Compare
4f3f4ba
to
e8348a9
Compare
Description
Store, in each pool's StakePoolState, the stake credentials that have delegated to that pool.
This set is updated in DELEG rule, for both shelley and conway:
The set is used to improve the performance of removing delegations when retiring the pool.
Based on PR: #5330
Closes: #4990
Checklist
CHANGELOG.md
files updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabal
andCHANGELOG.md
files when necessary, according to theversioning process.
.cabal
files updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh
).scripts/cabal-format.sh
).scripts/gen-cddl.sh
)hie.yaml
updated (usescripts/gen-hie.sh
).