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

Emit new TotalAdaPotEvent after epoch transition (gh #2780) #2797

Merged
merged 3 commits into from
May 18, 2022

Conversation

teodanciu
Copy link
Contributor

@teodanciu teodanciu commented May 17, 2022

This is an alternative to #2791

It is a solution for: #2780

The difference is in how the code is refactored. This PR extracts AdaPots in a separate module (outside the API) and uses it from NewEpoch, rather than move calculatePoolDistr inside Wallet.API.

But the question is: do these functions have to be in the API? From what I can tell, there is nothing using them at the moment, but I know little!

Are there better alternatives to this refactoring?

@teodanciu teodanciu changed the title Emit new TotalAdaPotEvent after epoch transition (https://github.com/input-output-hk/cardano-ledger/issues/2780#event-6594454094) Emit new TotalAdaPotEvent after epoch transition #2780) May 17, 2022
@teodanciu teodanciu changed the title Emit new TotalAdaPotEvent after epoch transition #2780) Emit new TotalAdaPotEvent after epoch transition (gh #2780) May 17, 2022
-- * Ada Pots
AdaPots (..),
totalAdaES,
totalAdaPotsES,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this module should re-export these three items.

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.

Besides re-export the ada pots support in the wallet API, this PR looks good to me!

to avoid calling API functions from within `NewEpoch`
@@ -166,6 +169,8 @@ newEpochTransition = do
-- map is empty (db-sync depends on it).
tellEvent $ TotalRewardEvent e regRU
pure es'
let adaPots = totalAdaPotsES es
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in #2791 (comment) I think es''' pots better fits what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thank you for all the explanation, I'm just about to push a new version with this.
Thanks!

@teodanciu teodanciu force-pushed the td/new-adapots-event branch from 12a5a59 to 58c4000 Compare May 18, 2022 10:02
@teodanciu
Copy link
Contributor Author

Forced push to use es''' EpochState. Also exporting AdaPots from Wallet.

@teodanciu
Copy link
Contributor Author

Force-pushed to retrigger build 🤞

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.

LGTM!

@teodanciu teodanciu force-pushed the td/new-adapots-event branch from c26b8bc to 3d9f369 Compare May 18, 2022 12:54
@teodanciu
Copy link
Contributor Author

Tests are passing on my machine (even after deleting dist-newstyle), so I don't get the failure on CI. Retrying

@JaredCorduan JaredCorduan merged commit 09b60ce into master May 18, 2022
@iohk-bors iohk-bors bot deleted the td/new-adapots-event branch May 18, 2022 16:52
@JaredCorduan JaredCorduan mentioned this pull request Jun 2, 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.

3 participants