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: bls_aggr encapsulate task initialization params in TaskMetadata #254

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

damiramirez
Copy link

@damiramirez damiramirez commented Jan 30, 2025

What Changed?

This PR introduces a refactor in task initialization by encapsulating related parameters into a new struct and simplifying function interfaces.

  • Create a new struct TaskMetadata with a constructor TaskMetadata::new to initialize a new task and with_window_duration to set the window duration.
  • initialize_new_task and single_task_aggregator accept TaskMetadata instead of multiple params.
  • Removed initialize_new_task_with_window.
pub struct TaskMetadata {
    task_index: TaskIndex,
    task_created_block: u32,
    quorum_numbers: Vec<u8>,
    quorum_threshold_percentages: QuorumThresholdPercentages,
    time_to_expiry: Duration,
    window_duration: Duration,
}

Reviewer Checklist

  • New features are tested and documented
  • PR updates the changelog with a description of changes
  • PR has one of the changelog-X labels (if applies)
  • Code deprecates any old functionality before removing it

@damiramirez damiramirez marked this pull request as ready for review January 30, 2025 18:13
Copy link
Contributor

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

Left some minor comments

crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
crates/services/bls_aggregation/src/bls_agg.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
Comment on lines 26 to 36
```rust
let metadata = TaskMetadata::new(
task_index,
block_number,
quorum_numbers,
quorum_threshold_percentages,
time_to_expiry,
)
.with_window_duration(window_duration);
bls_agg_service.initialize_new_task(metadata).await.unwrap();
```
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have before and after examples here. One for initialize_new_task and another with initialize_new_task_with_window

Copy link
Author

Choose a reason for hiding this comment

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

Done 7730ed1! What do you think about the format?

Copy link
Contributor

Choose a reason for hiding this comment

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

The "before" and "after" don't render correctly in markdown. We could have them as code comments instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Really good format otherwise!

@MegaRedHand MegaRedHand added the changelog-breaking [changelog] PR changes existing functionality label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-breaking [changelog] PR changes existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants