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

add pallas stake snapshots integration #1513

Merged
merged 12 commits into from
Feb 22, 2024

Conversation

falcucci
Copy link
Collaborator

@falcucci falcucci commented Feb 19, 2024

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

fixes #1315

- remove a print statement for "computed_stake_distribution"
- simplify `get_stake_pool_hash` function by removing `stakes` argument and adjust the return type.
- now fetch the `stakes.snapshot_mark_pool` directly in the `get_stake_distribution_snapshot` function when inserting into `stake_distribution`.
@falcucci
Copy link
Collaborator Author

@jpraynaud ready for review

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

Hi @falcucci, I left a few comments, otherwise LGTM 👍

mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
- rename `get_stake_distribution_snapshot` to `get_stake_distribution`
- change the test function name to include `with_fallback` instead of `fallback`
@falcucci falcucci requested a review from jpraynaud February 20, 2024 16:05
@falcucci
Copy link
Collaborator Author

@jpraynaud done! I resent the review back to you so you can approve it :)

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for the contribution @falcucci!

Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

Just a suggestion, otherwise LGTM 👍.
Thanks for the work @falcucci 🚀

Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

Nice work ! I've just two comments :).

mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
@sfauvel
Copy link
Collaborator

sfauvel commented Feb 21, 2024

Nothing to say after the review.
Good work.

- refactor the filter condition in the stake snapshots loop
- update `stakes` import for the changed module structure
@jpraynaud
Copy link
Member

Hi @falcucci, can you bump the crates versions so that we merge this PR?

- refactor imports in `pallas_observer.rs`
- update code references to `localstate::queries_v16` to now use the module specifiers directly
- consolidate initialization and usage of `stakes` struct in tests
- adjust stake values and pools in test data
- rename `localstate::queries_v16::stakesnapshot` to `stakesnapshot`
- refactor matching logic in `match` block for query handling
- update the version in cargo.toml from "0.3.4" to "0.3.5"
@falcucci
Copy link
Collaborator Author

@jpraynaud done!

@falcucci falcucci force-pushed the feat/stake-distribution branch from 5390053 to 6e3a590 Compare February 22, 2024 15:52
@falcucci falcucci force-pushed the feat/stake-distribution branch from 6e3a590 to 311c9f4 Compare February 22, 2024 16:50
Cargo.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

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

@falcucci, actually this file should not be modified, except for the version of the mithril-common that is bumped.
Can you roll it back completely and just bump the mithril-common version?

- update the version from `0.3.6` to `0.3.7` in the `mithril-common` package metadata in `cargo.toml`
@jpraynaud jpraynaud merged commit cff2c64 into input-output-hk:main Feb 22, 2024
36 of 40 checks passed
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.

Enhance Mithril/Cardano node communication
5 participants