Retry fee receiver validation forever and surface its status via /call - #101
Open
janezpodhostnik wants to merge 1 commit into
Open
Retry fee receiver validation forever and surface its status via /call#101janezpodhostnik wants to merge 1 commit into
janezpodhostnik wants to merge 1 commit into
Conversation
Previously validateFeeReceivers gave up for good after 5 quick attempts, so a flaky access node at startup plus a stale config could leave the server running with an unvalidated fee set indefinitely, and the only signal was a single log line. - Retry forever: quick backoff for the first 5 attempts, then a slow one-minute poll. Treat malformed script results as retryable instead of giving up. - Re-check every 10 minutes after a definitive result, so receivers added on chain while the server is running are also detected. - Downgrade a config mismatch from a fatal exit to a logged error that is surfaced via the new fee_receiver_validation_status /call method, matching the balance_validation_status pattern. - Replace the stringly-typed validation status with a validationStatus enum shared by balance and fee validation. - Add tests for the fee validation state machine, including a concurrent access test for the race detector.
📝 WalkthroughWalkthroughFee receiver validation now uses typed, synchronized state. It retries Access Node failures indefinitely, rechecks successful validations, preserves terminal results during transient retries, and exposes status details through ChangesFee receiver validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CallService as /call
participant Validator as validateFeeReceivers
participant AccessNode
participant Server
Validator->>AccessNode: validate configured fee receivers
AccessNode-->>Validator: receivers or transient error
Validator->>Server: store validation status
Client->>CallService: request fee_receiver_validation_status
CallService->>Server: read validation state
Server-->>CallService: status and receiver details
CallService-->>Client: validation response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
api/call_service.go (1)
215-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reporting the time of the last definitive check.
setFeeValidationRetryingpreserves a terminal result, so repeated access node failures do not change the reported status. An operator cannot then distinguish a freshsuccessfrom one recorded hours ago while every recheck since has failed. Store the timestamp of the last definitive result and the last attempt, then include them in this response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/call_service.go` around lines 215 - 230, Update the fee validation status flow around Server.feeReceiverValidationStatus and setFeeValidationRetrying to track both the timestamp of the last definitive validation result and the timestamp of the most recent check attempt, preserving terminal results across access-node failures. Include both timestamps in the returned result map so callers can distinguish a fresh status from a stale preserved result.api/api.go (2)
373-388: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAn unknown
validationStatusterminates the process from the/callrequest path. Both default branches calllog.Fatalf, which callsprocess.Exit(1)(log/log.go:78-81). The branches are unreachable for the current four enum values. If a fifth status is added later without updating both switches, one API request stops the server. Handle the unknown value without exiting.
api/api.go#L373-L388: return a fallback string such asfmt.Sprintf("unknown(%d)", int(v))instead of callinglog.Fatalfand panicking.api/call_service.go#L209-L211: replace the fatal default withlog.Errorfplus a response that carriesv.status.String(), so the handler degrades instead of exiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 373 - 388, The unknown validationStatus handling must degrade without terminating the server. In api/api.go at lines 373-388, update validationStatus.String to return a formatted fallback containing the numeric value instead of calling log.Fatalf or panicking; in api/call_service.go at lines 209-211, replace the fatal default with log.Errorf and return a response carrying v.status.String(), preserving normal handling for known statuses.
255-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the mismatch only on a status transition.
checkFeeReceiversre-runs everyfeeValidateRecheckInterval, so an unresolved mismatch logs the identical error every 10 minutes for the lifetime of the process.setFeeValidationSuccessalready logs only on transitions. Apply the same rule here to keep the log volume symmetric. The/callmethod still reports the persisted failure.♻️ Proposed change to log only on transitions
- log.Errorf("%s", msg) s.feeValidationMu.Lock() - defer s.feeValidationMu.Unlock() + prev := s.feeValidation.status s.feeValidation = &feeValidation{ err: msg, missing: missing, onchain: onchain, status: validationFailure, } + s.feeValidationMu.Unlock() + // We only log on transitions so that the periodic re-checks don't flood + // the logs. + if prev != validationFailure { + log.Errorf("%s", msg) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 255 - 270, Update setFeeValidationFailure to log the mismatch only when the existing fee validation status transitions into validationFailure, while continuing to persist the latest failure details on every check. Match the transition-aware behavior of setFeeValidationSuccess and preserve /call reporting through the stored feeValidation state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/api.go`:
- Around line 373-388: The unknown validationStatus handling must degrade
without terminating the server. In api/api.go at lines 373-388, update
validationStatus.String to return a formatted fallback containing the numeric
value instead of calling log.Fatalf or panicking; in api/call_service.go at
lines 209-211, replace the fatal default with log.Errorf and return a response
carrying v.status.String(), preserving normal handling for known statuses.
- Around line 255-270: Update setFeeValidationFailure to log the mismatch only
when the existing fee validation status transitions into validationFailure,
while continuing to persist the latest failure details on every check. Match the
transition-aware behavior of setFeeValidationSuccess and preserve /call
reporting through the stored feeValidation state.
In `@api/call_service.go`:
- Around line 215-230: Update the fee validation status flow around
Server.feeReceiverValidationStatus and setFeeValidationRetrying to track both
the timestamp of the last definitive validation result and the timestamp of the
most recent check attempt, preserving terminal results across access-node
failures. Include both timestamps in the returned result map so callers can
distinguish a fresh status from a stale preserved result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bbec699-e874-4e94-88ec-c2804a4266d8
📒 Files selected for processing (5)
README.mdapi/api.goapi/call_service.goapi/validate.goapi/validate_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fee receiver validation added in #100 gave up for good after 5 quick attempts and only logged a single line. A flaky access node at startup plus a stale config could leave the server running with an unvalidated fee set indefinitely. A config mismatch also killed the whole server via
log.Fatalf, even though a missing receiver only degrades operation classification, never indexed balances.Changes
fee_receiver_validation_status/callmethod (mirroringbalance_validation_status).validationStatusenum shared by balance and fee validation.Related: #100
Summary by CodeRabbit
New Features
/callinterface.Bug Fixes
Documentation