fix(deployment): skip initial funding terminally when the escrow account is closed - #3531
Conversation
…unt is closed The chain rejects the initial deposit with "account closed" when the deployment escrow account gets closed between lease start and the FundDeploymentCommand execution. The error was unmapped in both chain-error services, surfaced as a 500, and pg-boss retried the permanently terminal condition five times. Classify "account closed" as a 400 alongside "deployment closed" and make InitialDeploymentFundingService treat closed-deployment deposit failures as a terminal skip on both the thrown-simulation and landed-tx-with-non-zero-code paths, mirroring how the hourly top-up cron tolerates them. All other errors keep retrying.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughInitial funding now classifies closed-account failures as terminal, logs a skip, and avoids wallet reload scheduling. Chain-error mappings and tests cover the new classification. Fund deployment commands now use walletId instead of requiring userId. ChangesClosed deployment funding
Fund deployment command payload
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3531 +/- ##
==========================================
- Coverage 74.30% 73.54% -0.76%
==========================================
Files 1148 1084 -64
Lines 29815 27900 -1915
Branches 7465 7068 -397
==========================================
- Hits 22154 20520 -1634
+ Misses 6768 6515 -253
+ Partials 893 865 -28
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested fix that correctly classifies the "account closed" chain error as a terminal 400 instead of retrying against a permanently closed escrow account.
Extended reasoning...
Overview
The PR touches three services: apps/tx-signer/ChainErrorService and apps/api/ChainErrorService (both gain an account closed → 400 mapping), and apps/api/InitialDeploymentFundingService (now treats a deposit rejected for a closed deployment as a terminal skip on both the thrown-simulation-error path and the landed-non-zero-code path, instead of throwing and letting pg-boss retry against a dead escrow account).
Security risks
None. This is error classification/control-flow only — no new external input handling, no auth/permission changes, and no new data exposure. The regex /account closed|deployment closed/i only inspects an already-thrown error message string.
Level of scrutiny
Low-to-moderate is appropriate: the change is additive (new map entries, one new public helper, one new constructor dependency resolved via tsyringe DI) and mirrors an existing pattern (isMasterWalletInsufficientFundsError) already used by the top-up cron. InitialDeploymentFundingService is only ever instantiated through the DI container, so the new ChainErrorService constructor parameter is safe. I checked the ordering in the non-zero-code path — isDeploymentClosedError is checked before the INITIAL_FUNDING_TX_FAILED error log, so closed-account failures correctly skip that noisy error log and go to the terminal skip branch instead.
Other factors
Tests were added for both new chain-error branches and both funding-service skip paths (thrown error and landed tx with bad code), plus a regression test confirming unrelated errors (e.g. 503) still rethrow so pg-boss retry is preserved. The PR explicitly scopes out a REST pre-check as a documented follow-up rather than over-engineering this fix, which is reasonable given it would only shrink (not close) the race window.
…height fetch Remove the never-consumed userId field from FundDeploymentCommand and move getCurrentHeight below the early-return guards so it is not fetched on the not-indexed-yet or closed-deployment paths.
There was a problem hiding this comment.
LGTM — targeted, well-tested fix for a real production error.
What was reviewed: the new isDeploymentClosedError classification in both ChainErrorServices (api and tx-signer), the terminal-skip branches added to InitialDeploymentFundingService.fundOnLeaseStarted for both the thrown-error and landed-tx-with-nonzero-code paths, and the unrelated userId field removal from FundDeploymentCommand/handler (confirmed unused elsewhere). Verified unrelated errors (e.g. RPC 503) still rethrow so pg-boss retry is preserved for transient failures.
Extended reasoning...
Overview
This PR fixes error classification so that an "account closed" chain error (escrow account already closed by the time the async initial-funding job runs) is treated as a terminal, non-retryable 400 instead of an unmapped 500 that pg-boss retried 5 times against an account that can never accept the deposit. Changes touch ChainErrorService in both apps/api and apps/tx-signer (adding an account closed clue/pattern), a new public isDeploymentClosedError helper in the api's ChainErrorService, and two new terminal-skip branches in InitialDeploymentFundingService.fundOnLeaseStarted (one for the thrown simulation error, one for a landed tx with a non-zero code). It also carries an unrelated but small cleanup: dropping the now-unused userId field from FundDeploymentCommand/FundDeploymentHandler, and deferring the getCurrentHeight() RPC call until after the cheap closed-deployment check.
Security risks
None identified. This only changes error classification (500 → 400) and adds an early-return terminal-skip path; it does not touch auth, permissions, or any user-supplied input parsing beyond string matching on chain error messages.
Level of scrutiny
Moderate-light. This is a backend bug fix in billing/deployment funding code (not a config tweak), so it merits a real look, but the change is small, isolated to error-classification and one service method, follows an existing precedent in the codebase (isMasterWalletInsufficientFundsError), and is fully covered by new unit tests for every new branch (thrown error, landed-tx raw log, and the negative case of unrelated errors still rethrowing for retry).
Other factors
Coverage is 100% on all three touched service files per Codecov. The regex-based isDeploymentClosedError (/account closed|deployment closed/i) is applied consistently whether the error was pre-transformed by toAppError (message becomes "Deployment closed") or is a raw chain/rawLog string (contains "account closed"), which I verified by tracing both call sites. No CODEOWNERS or security-sensitive paths are touched, and CodeRabbit's automated review found no actionable comments.
2 participants
Why
Related to CON-735
The new initial-funding flow (#3527) produced this production error in tx-signer during gas simulation of the initial deposit:
The chain's escrow module rejects the deposit because the deployment's escrow account is already closed by the time the async
FundDeploymentCommandjob runs (the user closed the deployment right after creation, the provider closed it, or the small initial deposit fully drained). The pre-check infundOnLeaseStartedcan't reliably catch this: it reads a lagging REST snapshot (lease.closed_on), and its escrow-balance check counts already-transferredfunds.The actual defect is error classification: "account closed" was unmapped in both chain-error services, so a permanently terminal condition surfaced as a 500 and pg-boss retried it 5 times (exp backoff) against the still-closed account. The hourly top-up cron never has this problem because it tolerates per-deployment deposit failures instead of throwing.
What
ChainErrorService: mapaccount closed→ 400 (next to the existingdeployment closedpattern).ChainErrorService: add theaccount closedclue (→ 400 "Deployment closed") and a publicisDeploymentClosedErrorhelper, following theisMasterWalletInsufficientFundsErrorprecedent used by the top-up cron.InitialDeploymentFundingService: a deposit that fails because the deployment is closed is now a terminal skip (INITIAL_FUNDING_SKIPPED/DEPLOYMENT_CLOSED) on both failure paths — the thrown simulation error and a landed tx with non-zero code. All other errors keep throwing so pg-boss retry still covers transient failures (indexing lag, RPC 5xx).Side benefit: user-facing deposits into an already-closed deployment now return 400 "Deployment closed" instead of 500.
Deliberately out of scope: a pre-check on
escrow_account.state.statefrom chain REST — it reads the same lagging snapshot so it can only shrink the race window, and plumbing it throughRpcDeploymentInfo/DrainingDeploymentOutputtouches types shared with the top-up cron. Possible follow-up.Summary by CodeRabbit