Conversation
|
Code coverage report:
|
0xAustinWang
approved these changes
Apr 1, 2026
KodeyThomas
approved these changes
Apr 1, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request prevents the aggregator from submitting (and later surfacing via GetMessagesSince) aggregated reports that meet quorum but cannot be mapped into an exportable verifier result due to missing committee/destination-verifier configuration, while also tracking this condition via a new metric.
Changes:
- Add a new counter metric (
aggregator_aggregations_blocked_unexportable) and interface method to record when quorum is met but export mapping is impossible. - Update aggregation submission flow to pre-validate exportability (via
MapAggregatedReportToVerifierResultProto) and skip submission when unexportable. - Adjust report-to-proto mapping rules so message-discovery reports can omit destination verifier metadata, with new/updated unit tests covering the behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/mocks/mock_AggregatorMetricLabeler.go | Extends the metric labeler mock with IncrementAggregationsBlockedUnexportable. |
| aggregator/pkg/monitoring/noop.go | Adds a no-op implementation for the new metric method. |
| aggregator/pkg/monitoring/metrics.go | Registers the new counter and implements the increment method on the real labeler. |
| aggregator/pkg/model/model_mappers.go | Updates mapping logic to allow nil destination metadata for message-discovery version; otherwise require destination verifier mapping. |
| aggregator/pkg/model/map_aggregated_report_mapper_test.go | Adds tests validating destination-verifier rules for message-discovery vs non-message-discovery versions. |
| aggregator/pkg/common/metrics.go | Extends AggregatorMetricLabeler interface with the new method. |
| aggregator/pkg/aggregation/aggregator.go | Adds committee reference and blocks submission when quorum met but report is not exportable; increments new metric. |
| aggregator/pkg/aggregation/aggregator_test.go | Updates existing aggregation tests and adds coverage for blocked-unexportable scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
carte7000
deleted the
simon/aggregaotr/avoid-aggregation-when-not-exportable
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.