refactor: unify duplicated route env checks into a shared requireEnv helper - #522
Merged
Conversation
…helper
The 503-on-missing-env logic was copy-pasted across seven routes
(threshold, invoice-settings, treasury, release-escrow, compliance,
invoices, disputes), each with its own local requireEnv or inline check.
A fix in one copy silently diverges from the rest on a surface that
third-party merchants integrate against directly.
Extract a single requireEnv(res, vars) into src/lib/env.ts that always
validates SOROBAN_RPC_URL and the network passphrase, returns 503 + null
when any required var is missing, and returns a fully typed
{ rpcUrl, networkPassphrase, ...vars } object. Migrate all seven routes
to it and delete the local copies. Behavior is unchanged: same 503
status, same error message, same passphrase default.
Also includes two pre-existing blockers the test suite needs (fallout
from the WHEELBACK#344 merge that caused this duplication):
- src/db/mongo.ts: remove the duplicated `invoices` index block and the
duplicate getInvoicesCollection export (broke tsc and vitest transform)
- src/routes/invoices.ts: import validateBody/validateParams and the
invoice schemas it already referenced (ReferenceError at module load)
Generated with Codebuff 🤖
Co-Authored-By: Codebuff <noreply@codebuff.com>
refactor: unify duplicated route env checks into a shared requireEnv helper
|
@petermuazu Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Closes #414
Closes #415
Closes #416
Closes #417
Summary
The "503 on missing env config" logic was copy-pasted across seven route files —
threshold,invoice-settings,treasury,release-escrow,compliance,invoices, anddisputes— each with its own localrequireEnvhelper or inline check. Four of them defined a near-identical localrequireEnv(res); the others inlined the same check. Because a fix to one copy isn't propagated to the others, behavior silently diverges.This matters here because the backend is the primary REST/webhook surface for both frontends and the documented OpenAPI surface (
docs/api-reference.md) that third-party merchants integrate against. The 503 misconfiguration response is effectively part of the public contract.Changes
src/lib/env.ts— onerequireEnv(res, vars)helper:SOROBAN_RPC_URL(returned asrpcUrl) and the network passphrase (returned asnetworkPassphrase).503withService misconfiguration: missing required environment variablesand returnsnull.{ rpcUrl, networkPassphrase, ...vars }object via a generic mapped return type (e.g.TREASURY_CONTRACT_ID→env.treasuryContractId), so call sites stay type-safe and the object remains assignable to the env shapes the exported functions (e.g.setThreshold,releaseEscrow) expect.src/db/mongo.ts— removed a duplicatedinvoicesindex-creation block and a duplicategetInvoicesCollectionexport, which broketscand vitest's transform for every test importing mongo. The two index blocks were complementary, so all five indexes are preserved (merged into one block).src/routes/invoices.ts— added the missingvalidateBody/validateParamsand invoice schema imports the file already referenced but never imported (aReferenceErrorat module load that broke every app-level test suite).Verification
npx tsc --noEmit: no new errors from changed files (the repo has many pre-existing type errors onmain, e.g. a conflicting duplicateInvoiceStatusinterface inmongo.ts,lib/soroban.tstypings).npm test(vitest): all env/503 route tests across the changed routes pass — e.g.compliance,invoice-settings,release-escrow,treasuryenv-check suites (70/71 in the directly affected set; the single failure is a pre-existing stellar-sdk 12.3 address-format issue in untouched business logic).mainbefore this change (mongo.ts transform error + invoices.ts ReferenceError).Remaining pre-existing test failures (not caused by this PR)
The full suite still shows ~24 failures on this branch, all pre-existing and unrelated:
env-validation.test.tsimports avalidateEnvexport that no longer exists insrc/index.tsindexer-redis-reconnect.test.ts— stellar-sdk 12.3allowHttp+ fake-timer issuesinvoices.test.tspagination — untouchedGET /invoiceslist route vs. test contract mismatchmongo-hardening.test.ts— mongodb driver constructor drift (no lockfile pins versions)treasury.test.ts/treasury-balances-cache.test.ts— stellar-sdk 12.3 strict address validation + a fake-timer clock-reset bug in the TTL testNotes for reviewers
src/db/mongo.tsstill contains a conflicting duplicateInvoiceStatusinterface from the same feat: resolve issues #209 #210 #211 #212 #344 merge — deliberately left untouched since resolving it requires deciding the canonical shape.