Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds consensus cap config proto #1023

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

vyzaldysanchez
Copy link
Contributor

@vyzaldysanchez vyzaldysanchez commented Feb 6, 2025

This addresses the following stories:

For this to make sense, smartcontractkit/chainlink#16247 needs to be taken a look at, since it is what uses this.

Then all this will end up having a C-D PR to complete the cycle.

Related PRs:

bolekk
bolekk previously approved these changes Feb 11, 2025
pkg/capabilities/consensus/ocr3/factory.go Outdated Show resolved Hide resolved
pkg/capabilities/consensus/ocr3/factory.go Show resolved Hide resolved
bolekk
bolekk previously approved these changes Feb 11, 2025
Copy link
Contributor

@krehermann krehermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm confused by this change. How is it going to be used? how does it relate to configuring the contract in a deployment? is there a relationship with this

https://github.com/smartcontractkit/chainlink/blob/a9264ab9275052dd6a53b9d832753ea34b326a95/deployment/keystone/changeset/deploy_ocr3.go#L43

https://github.com/smartcontractkit/chainlink/blob/a9264ab9275052dd6a53b9d832753ea34b326a95/deployment/keystone/changeset/internal/ocr3config.go#L35

AFAICT, this change isn't used and doesn't do anything new. What am I missing?

@bolekk do you have something in your mind?

@bolekk
Copy link
Contributor

bolekk commented Feb 11, 2025

Yes, see smartcontractkit/chainlink#16247

@@ -88,11 +75,16 @@ func NewOCR3(config Config) *Capability {
return cp
}

func (o *Capability) SetRequestTimeout(timeout time.Duration) {
o.config.RequestTimeout = &timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks odd, why do we need to set in both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, we wanted to pass Capability around. I just pushed it as is to demonstrate to @bolekk that we might not need to pass it around and just access o.config.capability.setRequestTimeout() instead.

So I'm fully expecting to just remove this setter altogether and the passing around of Capability from this PR.

import "google/protobuf/duration.proto";

message ReportingPluginConfig {
uint32 maxQueryLengthBytes = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment that links to the libocr fields that these are shadowing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants