add registration status timeout to remote trigger config#1819
add registration status timeout to remote trigger config#1819
Conversation
✅ API Diff Results - No breaking changes |
578cb99 to
3d086a0
Compare
9d1adc8 to
7fea19a
Compare
7fea19a to
17e6dc6
Compare
17e6dc6 to
e393dd4
Compare
e393dd4 to
623f82b
Compare
be22f3e to
e00cf58
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new remote-trigger configuration field to control a “registration status update” timeout, wiring it through the protobuf schema and the capabilities registry client/server config translation layer.
Changes:
- Extend
RemoteTriggerConfig(proto + Go struct) withregistrationStatusUpdateTimeout/RegistrationStatusUpdateTimeout. - Serialize/deserialize the new duration field in
ConfigForCapabilityresponses (both top-level and per-method remote trigger configs). - Regenerate protobuf Go bindings to include the new field and accessor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/loop/internal/core/services/capability/capabilities_registry.go | Maps the new timeout field between internal capabilities.RemoteTriggerConfig and protobuf RemoteTriggerConfig for both capability-level and method-level configs. |
| pkg/capabilities/pb/registry.proto | Adds registrationStatusUpdateTimeout to RemoteTriggerConfig message. |
| pkg/capabilities/pb/registry.pb.go | Generated output reflecting the new proto field and getter. |
| pkg/capabilities/capabilities.go | Adds the new field to the Go RemoteTriggerConfig and attempts to apply a default for it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MessageExpiry: durationpb.New(cc.RemoteTriggerConfig.MessageExpiry), | ||
| MaxBatchSize: cc.RemoteTriggerConfig.MaxBatchSize, | ||
| BatchCollectionPeriod: durationpb.New(cc.RemoteTriggerConfig.BatchCollectionPeriod), | ||
| RegistrationStatusUpdateTimeout: durationpb.New(cc.RemoteTriggerConfig.RegistrationStatusUpdateTimeout), | ||
| }, |
There was a problem hiding this comment.
The new RegistrationStatusUpdateTimeout field is now serialized into the top-level remote trigger config, but there isn’t a focused test that verifies a non-zero value round-trips through the registry client/server conversion. Add a unit test that sets this field to a non-zero duration and asserts it is preserved after ConfigForCapability.
| MinResponsesToAggregate: mConfig.RemoteTriggerConfig.MinResponsesToAggregate, | ||
| MessageExpiry: durationpb.New(mConfig.RemoteTriggerConfig.MessageExpiry), | ||
| MaxBatchSize: mConfig.RemoteTriggerConfig.MaxBatchSize, | ||
| BatchCollectionPeriod: durationpb.New(mConfig.RemoteTriggerConfig.BatchCollectionPeriod), | ||
| RegistrationStatusUpdateTimeout: durationpb.New(mConfig.RemoteTriggerConfig.RegistrationStatusUpdateTimeout), | ||
| }, |
There was a problem hiding this comment.
The new RegistrationStatusUpdateTimeout field is now serialized for method-level remote trigger configs as well, but there isn’t a test asserting that a non-zero value round-trips correctly for CapabilityMethodConfig entries. Extend existing registry tests to cover a method config with a non-zero timeout and verify it survives encode/decode.
|
|
||
| if c.RegistrationStatusUpdateTimeout == 0 { | ||
| c.RegistrationStatusUpdateTimeout = DefaultRegistrationStatusUpdateTimeout | ||
| } |
There was a problem hiding this comment.
DefaultRegistrationStatusUpdateTimeout is defined as 0 and ApplyDefaults() assigns it when the field is 0, which is a no-op and makes it unclear whether 0 is meant to be a real default or an “unset/disabled” sentinel. Consider either (a) removing the constant + defaulting block entirely if 0 is the intended default, or (b) setting a non-zero default here (and documenting whether callers can explicitly disable by setting 0).
| if c.RegistrationStatusUpdateTimeout == 0 { | |
| c.RegistrationStatusUpdateTimeout = DefaultRegistrationStatusUpdateTimeout | |
| } |
Required for this change: smartcontractkit/chainlink#20973