Conversation
|
👋 wentzeld, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
✅ API Diff Results - No breaking changes |
There was a problem hiding this comment.
Pull request overview
This PR updates multiple error strings across limiters, consensus aggregators, config, and plugin code to provide more descriptive, user-actionable messages, along with updating the corresponding tests/examples that assert on those strings.
Changes:
- Expanded limiter error messages (rate/resource/time/bound/queue/not allowed) and updated tests/examples accordingly.
- Improved OCR3 consensus aggregator failure strings (identical + reduce) and updated test expectations.
- Updated config and plugin sentinel error strings to be more descriptive.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/settings/limits/resource_test.go | Updates example output strings for resource limiter errors. |
| pkg/settings/limits/rate_test.go | Updates example output strings for rate limiter errors. |
| pkg/settings/limits/gate_test.go | Updates example output string for “not allowed” limiter error. |
| pkg/settings/limits/errors_test.go | Updates expected strings for limiter error types. |
| pkg/settings/limits/errors.go | Updates limiter error Error() messages (rate/resource/time/bound/queue/not allowed). |
| pkg/settings/limits/bound_test.go | Updates example output string for bound limiter error. |
| pkg/loop/internal/goplugin/service.go | Makes ErrPluginUnavailable more descriptive. |
| pkg/config/errors.go | Improves KeyNotFoundError message clarity and quoting. |
| pkg/capabilities/consensus/ocr3/aggregators/reduce_test.go | Updates expected reduce-aggregator error strings. |
| pkg/capabilities/consensus/ocr3/aggregators/reduce_aggregator.go | Improves reduce-aggregator error messages with more context. |
| pkg/capabilities/consensus/ocr3/aggregators/identical_test.go | Updates expected identical-aggregator error substring. |
| pkg/capabilities/consensus/ocr3/aggregators/identical.go | Improves identical-aggregator consensus failure error message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| func (a *reduceAggregator) Aggregate(lggr logger.Logger, previousOutcome *types.AggregationOutcome, observations map[ocrcommon.OracleID][]values.Value, f int) (*types.AggregationOutcome, error) { | ||
| if len(observations) < 2*f+1 { | ||
| return nil, fmt.Errorf("not enough observations, have %d want %d", len(observations), 2*f+1) | ||
| return nil, fmt.Errorf("consensus failed: insufficient observations, received %d but need at least %d (2f+1, f=%d). Not enough DON nodes responded in time", len(observations), 2*f+1, f) |
There was a problem hiding this comment.
I don't think we should log "consensus failed" in some of those cases (like this one). Not enough observation means we can still get enough in the next round and succeed.
|
|
||
| func (e KeyNotFoundError) Error() string { | ||
| return fmt.Sprintf("unable to find %s key with id %s", e.KeyType, e.ID) | ||
| return fmt.Sprintf("key not found: no %s key exists with ID %q. Verify the key ID is correct and that the key has been created", e.KeyType, e.ID) |
There was a problem hiding this comment.
No, this is not helpful. Less is more.
There was a problem hiding this comment.
It is also not really accurate. This has to do with TOML config.
| ) | ||
|
|
||
| var ErrPluginUnavailable = errors.New("plugin unavailable") | ||
| var ErrPluginUnavailable = errors.New("plugin unavailable: the capability plugin is not running or has become unresponsive. This is typically a node operator issue - the plugin may need to be restarted") |
There was a problem hiding this comment.
Please do not change this message
| var ErrPluginUnavailable = errors.New("plugin unavailable: the capability plugin is not running or has become unresponsive. This is typically a node operator issue - the plugin may need to be restarted") | |
| var ErrPluginUnavailable = errors.New("plugin unavailable") |
| ) | ||
|
|
||
| var ErrPluginUnavailable = errors.New("plugin unavailable") | ||
| var ErrPluginUnavailable = errors.New("plugin unavailable: the capability plugin is not running or has become unresponsive. This is typically a node operator issue - the plugin may need to be restarted") |
There was a problem hiding this comment.
Please do not change this message
| var ErrPluginUnavailable = errors.New("plugin unavailable: the capability plugin is not running or has become unresponsive. This is typically a node operator issue - the plugin may need to be restarted") | |
| var ErrPluginUnavailable = errors.New("plugin unavailable") |
| func (e ErrorNotAllowed) Error() string { | ||
| which, who := errArgs(e.Key, e.Scope, e.Tenant) | ||
| return fmt.Sprintf("%slimited%s: not allowed", which, who) | ||
| return fmt.Sprintf("%slimited%s: operation not allowed. This action is restricted by current configuration and gate settings", which, who) |
There was a problem hiding this comment.
Why is this one different than the others? I think a "request..." suffix like the others would fit better
3 participants
Changes: Limits error strings (rate, resource, time, bound, queue, not allowed), consensus aggregator strings (identical + reduce), config error strings,
ErrPluginUnavailable
string, and corresponding test updates
Dependencies: None
See CRE Error Improvements for context