[CM-380] Identical Aggregator #771

bolekk · 2024-09-16T23:16:40Z

No description provided.

pkg/capabilities/consensus/ocr3/aggregators/identical.go

nolag · 2024-09-17T13:09:31Z

pkg/capabilities/consensus/ocr3/aggregators/identical.go

+			outcome[fmt.Sprintf("%d", idx)] = highestObservation
+		}
+	}
+	valMap, err := values.NewMap(outcome)


I feel like there needs to be a special case for a single observation to be just the object that's not keyed. This is our use case and I think most will be that way.

I thought about it too but unfortunately the return type of outcome is a Map, not a Value... hence the key overrides.

right, but if there's only one observation, the single observation's value should be a value.Value. That way the one observation can be unwrapped.

What I meant is that EncodableOutcome's type is values.Map so I can't assign Value to a Map... It needs at least one key.

What I was thinking is that the outcome gets stored in a fixed struct:

struct EncoderOutcome { Value any }

Then you send the Value to the encoder.

pkg/capabilities/consensus/ocr3/aggregators/identical.go

bolekk · 2024-09-19T01:24:45Z

pkg/capabilities/consensus/ocr3/aggregators/identical.go

+	// If non-empty, must be of length ExpectedObservationsLen.
+	KeyOverrides []string
+	// If true, the encoder name and config will be overridden with the values
+	// provided under configured indices of the observations array (as long as consensus is reached).


Not too happy with this approach... suggestions welcome.

@cedric-cordenier this is slightly different to what we discussed but I don't see how we can make a generic aggregator with only a single observation.

nolag · 2024-09-19T14:02:42Z

pkg/capabilities/consensus/ocr3/aggregators/identical.go

+		return nil, err
+	}
+	return &types.AggregationOutcome{
+		EncodableOutcome: values.ProtoMap(valMap),


Alternatively, you can make this a value.Value. There's no reason someone can't encode a number, a list of numbers etc.

We actually assume that it needs to be a map since we add metadata to it. Any objection to leaving this as is and revisiting at a future date?

* [chore] Handle aliases in slices * More aliasing tests * Lint fix * Fix test --------- Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com>

…dashboard (#785)

* [CAPPL-58] Further cleanup * [CAPPL-58] Add support for compression

* Generic case to handle both pointer type and raw type and simplify int unwrap * Handling interface and default * Small test fix --------- Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com>

* Fix alias typing and tests * Fix ints * errors.new instead of fmt * Add array support to slice (#789)

nolag · 2024-10-09T14:57:17Z

pkg/capabilities/consensus/ocr3/ocr3cap/identical_consensus.go

I think the mock in ocr3cap mock might need to change for this to work with it. Should we expose another wrapper to get multiple consensus or just hide at that point? I'm good either way.

Also, if we're only returning one of the signed reports, but there's actually an array, then line 30 should be of type []SignedReport and you would need to wrap the capability in sdk.ToListDefinition and on line 31 use step.Index(0) instead of step.

Also, if we're only returning one of the signed reports, but there's actually an array, then line 30 should be of type []SignedReport and you would need to wrap the capability in sdk.ToListDefinition and on line 31 use step.Index(0) instead of step.

We only output one report atm, unless I'm misunderstanding the question?

bolekk had a problem deploying to integration September 16, 2024 23:16 — with GitHub Actions Failure

bolekk temporarily deployed to integration September 16, 2024 23:16 — with GitHub Actions Inactive

bolekk requested a review from nolag September 16, 2024 23:16

nolag reviewed Sep 17, 2024

View reviewed changes

bolekk added 3 commits September 18, 2024 18:00

[CM-380] Identical Aggregator

7b72471

[CAPPL-60] Dynamic encoder selection in OCR consensus aggregator

9c58d5c

extract encoder name and config

2080f7b

bolekk force-pushed the feature/CM-380-identical-aggregator branch from a141b0a to 2080f7b Compare September 19, 2024 01:24

bolekk temporarily deployed to integration September 19, 2024 01:24 — with GitHub Actions Inactive

bolekk commented Sep 19, 2024

View reviewed changes

bolekk requested a review from cedric-cordenier September 19, 2024 01:25

nolag reviewed Sep 19, 2024

View reviewed changes

Add more tests

332728e

cedric-cordenier had a problem deploying to integration October 4, 2024 10:49 — with GitHub Actions Failure

cedric-cordenier temporarily deployed to integration October 4, 2024 10:49 — with GitHub Actions Inactive

cedric-cordenier marked this pull request as ready for review October 4, 2024 12:33

cedric-cordenier requested a review from a team as a code owner October 4, 2024 12:33

dimkouv and others added 9 commits October 4, 2024 13:35

add limit to seq num range (#781)

86e8dfc

[chore] Handle aliases in slices (#784)

ee30401

* [chore] Handle aliases in slices * More aliasing tests * Lint fix * Fix test --------- Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com>

feat(observability-lib): legendoptions + improvement on node general …

fdf417c

…dashboard (#785)

[CAPPL-58] Correctly stub out clock_time_get and poll_oneoff (#778)

f88a9b7

* [CAPPL-58] Further cleanup * [CAPPL-58] Add support for compression

More alias handling in Unwrap functionality of Value (#792)

0554950

* Generic case to handle both pointer type and raw type and simplify int unwrap * Handling interface and default * Small test fix --------- Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com>

Fix alias typing and tests (#788)

1c89fdb

* Fix alias typing and tests * Fix ints * errors.new instead of fmt * Add array support to slice (#789)

Replace fmt.Errorf with errors.New where possible (#795)

dc37362

chore(workflows): adds unit test to utils (#782)

0c7cade

Have the mock runner register with capabilites (#783)

920340b

cedric-cordenier had a problem deploying to integration October 9, 2024 13:52 — with GitHub Actions Failure

Adjust tests

7abd311

cedric-cordenier force-pushed the feature/CM-380-identical-aggregator branch from 3affcec to 7abd311 Compare October 9, 2024 14:01

cedric-cordenier had a problem deploying to integration October 9, 2024 14:02 — with GitHub Actions Failure

nolag reviewed Oct 9, 2024

View reviewed changes

Fix mock

449d79f

cedric-cordenier had a problem deploying to integration October 9, 2024 16:22 — with GitHub Actions Failure

cedric-cordenier had a problem deploying to integration October 9, 2024 16:22 — with GitHub Actions Error

Fix mock

13de65e

cedric-cordenier had a problem deploying to integration October 9, 2024 16:23 — with GitHub Actions Failure

Update mock

ef71d0f

cedric-cordenier had a problem deploying to integration October 9, 2024 18:13 — with GitHub Actions Failure

nolag previously approved these changes Oct 9, 2024

View reviewed changes

Linting

3cf5d9f

cedric-cordenier dismissed nolag’s stale review via 3cf5d9f October 10, 2024 09:33

cedric-cordenier had a problem deploying to integration October 10, 2024 09:33 — with GitHub Actions Failure

cedric-cordenier requested a review from nolag October 10, 2024 09:41

nolag approved these changes Oct 10, 2024

View reviewed changes

agparadiso approved these changes Oct 10, 2024

View reviewed changes

cedric-cordenier merged commit 4637084 into main Oct 10, 2024
8 of 11 checks passed

cedric-cordenier deleted the feature/CM-380-identical-aggregator branch October 10, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CM-380] Identical Aggregator #771

[CM-380] Identical Aggregator #771

bolekk commented Sep 16, 2024

nolag Sep 17, 2024

bolekk Sep 17, 2024

nolag Sep 17, 2024

bolekk Sep 19, 2024

nolag Sep 19, 2024

bolekk Sep 19, 2024 •

edited

Loading

nolag Sep 19, 2024

cedric-cordenier Oct 4, 2024 •

edited

Loading

nolag Oct 9, 2024

cedric-cordenier Oct 9, 2024

[CM-380] Identical Aggregator #771

[CM-380] Identical Aggregator #771

Conversation

bolekk commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolekk Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedric-cordenier Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolekk Sep 19, 2024 •

edited

Loading

cedric-cordenier Oct 4, 2024 •

edited

Loading