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

Improve evaluate transaction balance #3342

Merged
merged 5 commits into from
Mar 21, 2023
Merged

Improve evaluate transaction balance #3342

merged 5 commits into from
Mar 21, 2023

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Mar 17, 2023 •
edited
Loading

Description

Add your description here, if it fixes a particular issue please provide a
link
to the issue.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the CHANGELOG.md for affected package
  • 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

@lehins lehins force-pushed the lehins/improve-evaluateTransactionBalance branch from 8311997 to 28e3012 Compare March 17, 2023 23:46
@lehins lehins changed the title Lehins/improve evaluate transaction balance Improve evaluate transaction balance Mar 17, 2023
@lehins lehins force-pushed the lehins/improve-evaluateTransactionBalance branch 3 times, most recently from 162e501 to e3f09ad Compare March 20, 2023 19:59
@lehins lehins marked this pull request as ready for review March 20, 2023 20:00
@lehins lehins force-pushed the lehins/improve-evaluateTransactionBalance branch from e3f09ad to 4a1fd46 Compare March 20, 2023 20:21
JaredCorduan
JaredCorduan approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

looks great to me!

@lehins lehins force-pushed the lehins/improve-evaluateTransactionBalance branch from 4a1fd46 to c67d99f Compare March 21, 2023 15:06
TimSheard
TimSheard approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

Add a few comments about what's changed that makes passing the functions around instead of the DState. Looks good to me.

eras/mary/impl/src/Cardano/Ledger/Mary/UTxO.hs Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/UTxO.hs Show resolved Hide resolved
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/Tx/Body.hs Show resolved Hide resolved
libs/cardano-ledger-api/test/Test/Cardano/Ledger/Api/Tx/Body.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/DPState.hs Outdated Show resolved Hide resolved
lehins added 5 commits March 21, 2023 21:11
@lehins
Change the way produced and consumed work:
1599ee5 
It order to avoid requiring the full knowledge about the `DPState`
during comuptation of `consumed` and `produced` transaction balance this
commit makes the computation more abstract by accepting functions that
can provide that knowledge, but only for the certificates relevant to a
particular `TxBody`, rather than the whole system.

* Change `totalCertsDeposits` and add `getProducedValue`.
  `totalCertsDepositsDPState` can be used to recover the previous
  behavior of `totalCertsDeposits`.

* Avoid knowledge of `UMap` when computing delegation deposit refunds

* Add `keyCertsRefundsDPState`.

* Make `getConsumedValue` accept a deposit lookup function instead of a
  `DPState`

* Introduce `consumed` that uses `getConsumedValue`

* Add `lookupDepositDState` and `lookupRewardDState`
@lehins
Addition of evalBalanceTxBody to cardano-ledger-api
63ab695
@lehins
Add a property test for evalBalanceTxBody:
6569d80 
This test brings back previous implementation of
`evaluateTransactionBalance` and checks that they produce identical
results.
@lehins
Deprecate evaluateTransactionBalance in favor of evalBalanceTxBody
f5dee7b
@lehins
Minor cleanup and simplification. Change `StakePoolRetirementWrongEpo…
8661576 
…chPOOL`:

Change types in `StakePoolRetirementWrongEpochPOOL` from `Word64` to `EpochNo`
@lehins lehins force-pushed the lehins/improve-evaluateTransactionBalance branch from c67d99f to 8661576 Compare March 21, 2023 18:11
@lehins lehins enabled auto-merge March 21, 2023 18:27
@lehins lehins merged commit b3f8866 into master Mar 21, 2023
@iohk-bors iohk-bors bot deleted the lehins/improve-evaluateTransactionBalance branch March 21, 2023 19:32
@lehins lehins mentioned this pull request Mar 21, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimSheard TimSheard TimSheard approved these changes

@JaredCorduan JaredCorduan JaredCorduan approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@lehins @JaredCorduan @TimSheard