Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves challenger logic (deduping in-flight challenges, safer error handling) and adds substantial test coverage around transaction confirmation polling and ScribeOptimistic provider behaviors, plus small operational/config updates (metrics/logging/CLI).
Changes:
- Add polling-interval configurability for
WaitForTxConfirmationand introduce a comprehensive test suite for its edge cases. - Update challenger/provider logic (in-flight dedupe, tick-error handling refactor,
GetFromcaching, andPickUnchallengedPokesbug fixes) with new/expanded tests. - Adjust Prometheus error metric labels and enhance CLI configuration (log level, metrics bind address, password-file newline trimming, graceful metrics server shutdown).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/utils.go | Introduces txConfirmationPollInterval used by WaitForTxConfirmation. |
| core/utils_test.go | Adds thorough tests for WaitForTxConfirmation scenarios. |
| core/scribe_optimistic_provider.go | Adds cached GetFrom and replaces a panic with a returned error in GetChallengePeriod. |
| core/scribe_optimistic_provider_test.go | Expands provider tests (events fetching/decoding, signature validation, flashbot fallback, GetFrom caching). |
| core/challenger.go | Adds in-flight dedupe, refactors tick error handling, fixes PickUnchallengedPokes logic. |
| core/challenger_test.go | Adds extensive new tests for poke filtering, tick execution, run loop, and dedupe behavior. |
| core/metrics.go | Changes ErrorsCounter label set (drops error). |
| cmd/challenger/main.go | Adds --metrics-addr/--log-level, trims password-file newlines, improves metrics server lifecycle, updates cobra args handling. |
| .gitignore | Ignores /challenger binary/output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *ScribeOptimisticRpcProvider) GetFrom(ctx context.Context) types.Address { | ||
| accs, err := s.client.Accounts(ctx) | ||
| if err != nil { | ||
| logger.Errorf("failed to get accounts with error: %v", err) | ||
| return types.ZeroAddress | ||
| } | ||
| if len(accs) == 0 { | ||
| logger.Errorf("no accounts found") | ||
| return types.ZeroAddress | ||
| } | ||
| return accs[0] | ||
| s.fromOnce.Do(func() { | ||
| accs, err := s.client.Accounts(ctx) | ||
| if err != nil { | ||
| logger.Errorf("failed to get accounts with error: %v", err) | ||
| return | ||
| } | ||
| if len(accs) == 0 { | ||
| logger.Errorf("no accounts found") | ||
| return | ||
| } | ||
| s.fromAddr = accs[0] | ||
| }) | ||
| return s.fromAddr |
There was a problem hiding this comment.
GetFrom is cached via sync.Once, but the Once will still be marked as done even when Accounts returns an error or an empty list. That means a transient RPC error (or calling with a cancelled context) will permanently lock fromAddr to ZeroAddress for the lifetime of the provider. Consider only caching on success (e.g., guard with a mutex + fromLoaded boolean, or retry until a non-zero address is fetched).
1 participant
No description provided.