[KS-196] Validate Signatures in Mercury Aggregator #522
Conversation
bolekk
commented
May 20, 2024
bolekk
commented
- Add Metadata field to TriggerEvent. Use it to pass Streams DON info ("SignersMetadata" struct) between trigger receiver and OCR aggregator.
- Extend feeds OCR aggregator with an initial phase of agreeing on a set of valid signers and the F value.
- Add ReportContext field to FeedReport - Streams DON signs both fields together so we need to pass them to validate sigantures.
bolekk
force-pushed
the
feature/KS-196-signatures
branch
from
c1c0a4c
to
d7d6fe3
Compare
bolekk
force-pushed
the
feature/KS-196-signatures
branch
from
d7d6fe3
to
0ddb73d
Compare
bolekk
force-pushed
the
feature/KS-196-signatures
branch
from
0ddb73d
to
d1c37bd
Compare
pkg/capabilities/capabilities.go
Outdated
| @@ -80,6 +80,7 @@ type TriggerEvent struct { | |||
| TriggerType string | |||
| ID string | |||
| Timestamp string | |||
| Metadata values.Value | |||
There was a problem hiding this comment.
Metadata is reserved for workflow-specific data, not trigger-specific data (see comment below). However, if the capability response type is a report, it might make sense to include the signers in the metadata. Metadata field expectations based on capability response type should be documented, though.
There was a problem hiding this comment.
Also, shouldn't this be on the CapabilityResponse and not TriggerEvent?
There was a problem hiding this comment.
I want it to be trigger-specific, I will move the comment to apply to both fields.
I initially wanted to put it inside payload but didn't want to add a yet another layer of wrapping.
bolekk
force-pushed
the
feature/KS-196-signatures
branch
from
d1c37bd
to
d695db0
Compare
| @@ -37,23 +41,24 @@ type dataFeedsAggregator struct { | |||
|
|
|||
| var _ types.Aggregator = (*dataFeedsAggregator)(nil) | |||
|
|
|||
| // EncodableOutcome is a list of AggregatedPricePoints | |||
| // This Aggregator has two phases: | |||
| // 1. Agree on valid trigger signers by extracting them from event metadata and aggregating using MODE (i.e. F+1 copies needed). | |||
There was a problem hiding this comment.
what is the comment about F+1? is that specific to MODE? if so, i don't understand how one is related to the other
There was a problem hiding this comment.
yeah, MODE but the value needs to appear at least F+1 times
| // find latest valid Mercury report for each feed ID | ||
| func (a *dataFeedsAggregator) Aggregate(previousOutcome *types.AggregationOutcome, observations map[ocrcommon.OracleID][]values.Value, f int) (*types.AggregationOutcome, error) { | ||
| allowedSigners, minRequired, payloads := a.extractSignersAndPayloads(observations, f) | ||
| if len(payloads) > 0 && minRequired == 0 { |
There was a problem hiding this comment.
this is an odd check. why not return an error from the extract? or maybe rename minRequired?
There was a problem hiding this comment.
It's a little tricky because for empty observations (from valid nodes!) we will extract zero minRequired but we still want to complete the rest of Aggregate to return meaningful outcome. I re-worded the error message to be more descriptive.
| // validate each report and convert to a list of Mercury reports | ||
| Unwrap(raw values.Value) ([]FeedReport, error) | ||
| // validate each report and convert to a list of Feed reports | ||
| Unwrap(wrapped values.Value, allowedSigners [][]byte, minRequiredSignatures int) ([]FeedReport, error) |
There was a problem hiding this comment.
this doesn't look like Unwrap any longer. UnwrapAndValidate? FeedsFromValue()? CreateFeeds()?
There was a problem hiding this comment.
How about UnwrapValid()? :)
bolekk
force-pushed
the
feature/KS-196-signatures
branch
from
d695db0
to
8590a67
Compare
1. Add Metadata field to TriggerEvent. Use it to pass Streams DON info ("SignersMetadata" struct) between trigger receiver and OCR aggregator.
2. Extend feeds OCR aggregator with an initial phase of agreeing on a set of valid signers and the F value.
3. Add ReportContext field to FeedReport - Streams DON signs both fields together so we need to pass them to validate sigantures.
bolekk
force-pushed
the
feature/KS-196-signatures
branch
from
a9f0cbd
to
a45f8db
Compare
