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

Remove mithril common circular dependencies and random feature #2269

Merged
merged 6 commits into from
Feb 4, 2025

Conversation

sfauvel
Copy link
Collaborator

@sfauvel sfauvel commented Feb 3, 2025

Content

Preliminary modifications for the mithril-common split:

  • Removal of the random feature
  • Removal of following circular dependencies
    • entities <-> messages
    • entities <-> protocol
    • entities <-> signable_builder

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is 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)

Issue(s)

Closes #2253

Copy link

github-actions bot commented Feb 3, 2025

Test Results

    4 files  ±0     56 suites  ±0   10m 41s ⏱️ +9s
1 590 tests ±0  1 590 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 888 runs  ±0  1 888 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4c644ae. ± Comparison against base commit 92ecbe8.

♻️ This comment has been updated with latest results.

@sfauvel sfauvel force-pushed the sfa/2253/remove_mithril_common_circular_dependencies branch from fc18ec8 to 8337329 Compare February 3, 2025 15:15
@sfauvel sfauvel force-pushed the sfa/2253/remove_mithril_common_circular_dependencies branch from 8337329 to 229f619 Compare February 3, 2025 15:29
@sfauvel sfauvel temporarily deployed to testing-preview February 3, 2025 15:38 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet February 3, 2025 15:38 — with GitHub Actions Inactive
@sfauvel sfauvel force-pushed the sfa/2253/remove_mithril_common_circular_dependencies branch from a0de13c to 7cf6061 Compare February 3, 2025 16:52
@sfauvel sfauvel marked this pull request as ready for review February 3, 2025 16:53
@sfauvel sfauvel temporarily deployed to testing-preview February 3, 2025 17:01 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet February 3, 2025 17:01 — with GitHub Actions Inactive
@sfauvel sfauvel force-pushed the sfa/2253/remove_mithril_common_circular_dependencies branch from 7cf6061 to 229f619 Compare February 4, 2025 09:04
@sfauvel sfauvel requested review from Alenar, jpraynaud and dlachaume and removed request for Alenar and jpraynaud February 4, 2025 09:04
@sfauvel sfauvel temporarily deployed to testing-preview February 4, 2025 09:11 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet February 4, 2025 09:11 — with GitHub Actions Inactive
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.

LGTM

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 👍

Comment on lines 17 to 20
pub transactions_hashes: Vec<TransactionHash>,

/// Proof of the transactions
transactions_proof: ProtocolMkProof,
pub transactions_proof: ProtocolMkProof,
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to have these fields public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The TryFrom<CardanoTransactionsSetProofMessagePart> for CardanoTransactionsSetProof that uses these fields is moved to the messages module. That's why the visibility change.
We can make them public for the crate (pub(crate)) unless you have an other solution

Move `signed_entity` to `signable_builder` module.
Move `Beacon` and `Artifact` implementations in `signable_builder` module.
* mithril-aggregator from `0.6.25` to `0.6.26`
* mithril-client from `0.10.9` to `0.10.10`
* mithril-common from `0.4.111` to `0.4.112`
* mithril-aggregator-fake from `0.3.19` to `0.3.20`
@sfauvel sfauvel force-pushed the sfa/2253/remove_mithril_common_circular_dependencies branch from 229f619 to 4c644ae Compare February 4, 2025 16:32
@sfauvel sfauvel temporarily deployed to testing-preview February 4, 2025 16:40 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet February 4, 2025 16:40 — with GitHub Actions Inactive
@sfauvel sfauvel merged commit 0163127 into main Feb 4, 2025
43 checks passed
@sfauvel sfauvel deleted the sfa/2253/remove_mithril_common_circular_dependencies branch February 4, 2025 16:44
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.

Split mithril-common crate - Preliminary work
4 participants