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

Refactor: signable builder services compute full protocol message #1942

Merged
merged 33 commits into from
Sep 25, 2024

Conversation

jpraynaud
Copy link
Member

@jpraynaud jpraynaud commented Sep 20, 2024

Content

This PR includes a refactoring of the signable builder service so that it computes the next aggregate verification key part of the protocol message instead of delegating this responsibility to the state machines runners in signer and aggregator.

A new SignableSeedBuilder trait has been created which is responsible for computing part of the protocol message by the SignableBuilder service to enrich it.

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

Issue(s)

Closes #1941

@jpraynaud jpraynaud self-assigned this Sep 20, 2024
Copy link

github-actions bot commented Sep 20, 2024

Test Results

    4 files  ±0     54 suites  ±0   9m 48s ⏱️ +9s
1 299 tests +2  1 299 ✅ +2  0 💤 ±0  0 ❌ ±0 
1 510 runs  +2  1 510 ✅ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 71fef49. ± Comparison against base commit 17b94ba.

♻️ This comment has been updated with latest results.

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 👍

mithril-signer/src/services/signable_seed_builder.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM
I have some questions about responsabilities of the services

mithril-aggregator/src/services/signable_seed_builder.rs Outdated Show resolved Hide resolved
@jpraynaud jpraynaud force-pushed the jpraynaud/1941-refactor-signable-builder branch 2 times, most recently from 7fb6683 to d445ce7 Compare September 24, 2024 14:43
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.

LGTM 🔥

mithril-signer/src/services/epoch_service.rs Outdated Show resolved Hide resolved
…lues

And let the responsibility of using it in the signable builder service.
- 'mithril-aggregator' from '0.5.67' to '0.5.68'
- 'mithril-common' from '0.4.57' to '0.4.58'
- 'mithril-signer' from '0.2.186' to '0.2.187'.
@jpraynaud jpraynaud force-pushed the jpraynaud/1941-refactor-signable-builder branch from ab51421 to 71fef49 Compare September 24, 2024 16:21
@jpraynaud jpraynaud merged commit e584a5b into main Sep 25, 2024
48 of 78 checks passed
@jpraynaud jpraynaud deleted the jpraynaud/1941-refactor-signable-builder branch September 25, 2024 07:23
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.

Refactor signable builder services to compute full protocol message in signer/aggregator
4 participants