[KS-196] ReportCodec implementation for Streams trigger#13218
Conversation
bolekk
force-pushed
the
deploy_staging
branch
11 times, most recently
from
db12eea to
ac1bba1
Compare
bolekk
changed the title
core/capabilities/streams/codec.go
Outdated
There was a problem hiding this comment.
May I ask, why is the list of reports passed as a wrapped values.Value?
There was a problem hiding this comment.
values.Value is a universal wrapper for all data passed around during workflow execution. Trigger Service will wrap it and then the receiver DON will unwrap it here.
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
Worth implementing a StateMachine here?
There was a problem hiding this comment.
This Syncer implementation will soon be replaced with a real one reading stuff from the chain. Right now it's here just to allow easy local and staging tests. Please ignore all shortcomings ;)
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
Did you mean to delete this?
There was a problem hiding this comment.
I want to leave it for easier local tests - will be replaced within 2 weeks.
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
Is this a permanent failure that leaves the node in a broken state?
There was a problem hiding this comment.
broken meaning unable to send trigger events but otherwise functional.
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
Seems confusingly named, count doesn't appear to be a timer
There was a problem hiding this comment.
true, will remove the "seconds" thanks
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
Would this be cleaner using a backoff.Backoff with permanent retry?
There was a problem hiding this comment.
yes but probably not worth it - again this Syncer won't be used in prod.
bolekk
force-pushed
the
deploy_staging
branch
2 times, most recently
from
e42f85a to
aa9659d
Compare
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
Heads up, that this will change to be bytes32 onchain (https://github.com/smartcontractkit/chainlink/pull/13183/files#diff-d94c1796b3544ef8780af7e16b5361c8fb9e4d63f492faeda6a3362a8363f51bR45-R51). We can do decoding once the Syncer is up, but not sure the best place to do that, thread: https://chainlink-core.slack.com/archives/C064418MJHE/p1716308390423929
There was a problem hiding this comment.
That's fine. As long as the Syncer passes correct binary addresses to all services, we're good.
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
| s.lggr.Infow("trigger not found yet ...", "capabilityId", capId, "error", err) | |
| s.lggr.Infow("trigger not found yet ...", "capabilityId", capId, "error", err2) |
core/capabilities/syncer.go
Outdated
There was a problem hiding this comment.
In the future, this should only happen when DON isPublic, right? Currently, this assumes that the capability is always remote without actually checking for local existence, right?
There was a problem hiding this comment.
Shouldn't this rawReportCtx [3][32]byte, be based on a workflow spec config?
There was a problem hiding this comment.
How so? The context encodes OCR round and epoch in which the report was produced.
There was a problem hiding this comment.
I guess it's not clear what this context is.
There was a problem hiding this comment.
It should be pretty clear in the context of the Transmitter because it already uses it elsewhere.
1. Implement Codec, which validates report signatures and decodes needed fields. 2. Pass report context from Merucry Transmitter, which is needed to validate signatures. 3. Update fake Syncer to run successful e2e tests.
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.