-
Notifications
You must be signed in to change notification settings - Fork 166
add queryDRepDelegations ledger state query
#5176
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
add queryDRepDelegations ledger state query
#5176
Conversation
lehins
left a comment
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.
First of all, let me say thank you for a very nice contribution.
I can certainly see how this query can be useful, but there are some surprising things that I have some questions about.
Also, unfortunately, it is a bad timing to add such a query, because we are on a verge of replacing UMap in favor of a more accurate and simpler representation in #5128
It doesn't mean we can't add this query. It's just I would recommend addressing all other comments in this PR first and then we can change the implementation in the query itself once the above PR is merged.
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/State/Imp/QuerySpec.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/State/Imp/QuerySpec.hs
Outdated
Show resolved
Hide resolved
8ba689c to
72c566c
Compare
The generated snapshots are 'null' because we failed to fetch the right status when building the initial data. This should be hopefully resolved by: IntersectMBO/cardano-ledger#5176 Signed-off-by: KtorZ <[email protected]>
lehins
left a comment
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 query is exactly what we had discussed. Hopefully it was simpler to work with new AccountState abstraction, rather than the older UMap representation.
Most of my suggestions should lead to a slightly simpler implementation, but the end result should be the same.
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/State/Imp/QuerySpec.hs
Show resolved
Hide resolved
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/State/Imp/QuerySpec.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/State/Imp/QuerySpec.hs
Outdated
Show resolved
Hide resolved
lehins
left a comment
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.
Beautiful! Thank you for your hard work!
Could I ask you one last thing. Could you please create couple of tickets on:
- https://github.com/IntersectMBO/ouroboros-consensus
- and https://github.com/IntersectMBO/cardano-api
that mentions the need to integrate this query all the way to cardano-cli
lehins
left a comment
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.
One last comment and it needs fourmolu applied: https://github.com/IntersectMBO/cardano-ledger/actions/runs/16891288070/job/47946964522?pr=5176
Should we create a ticket in |
1260fee to
0590262
Compare
Yes, it would be good too! Although, I think if cardano-api adds a query, I believe compiler will force cardano-cli to account for it as well. |
queryDRepDelegationState state queryqueryDRepDelegations ledger state query
Also export `credToDRep` and `dRepToCred`
0590262 to
e262c58
Compare
Description
This adds a ledger state query to obtain the delegators (and deposit) for each DRep, including
DRepAlwaysAbstainandDRepAlwaysNoConfidence, which cannot be queried via the existingqueryDRepState. Since there is no sensible expiry or anchor for those DReps, a new type is introduced that is likeDRepStatebut with those fields omitted. The result is computed by folding over theUMap.This would be useful for amaru; see for example this comment.
Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles 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.yamlupdated (usescripts/gen-hie.sh).