-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
AFAICT, this change isn't used and doesn't do anything new. What am I missing?
@bolekk do you have something in your mind?
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
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: