Feat/1700 1701 1702 1703 test coverage - #1746
Merged
yusuftomilola merged 6 commits intoAug 31, 2026
Merged
Conversation
…cipient set atomically Every recipient in the failing cases individually satisfies RevenueSplitRecipientDto's per-field @min(1) @max(10000) check — only the set's total is wrong (9999 or 10001). Adds a test on the update path (replaceRecipients, backing PUT .../recipients) proving that rejection, and that neither rejected update partially deletes the existing recipients before the sum check fails. Closes DistinctCodes#1700
… a full batch Nothing bounded how long a full PAYMENT_RECONCILE_MAX_BATCH pass could take against a slow/degraded provider — 500 payments sequentially awaiting up to PAYMENT_VERIFY_TIMEOUT_MS each can exceed the 5-minute cron interval. Adds a process-local in-progress flag so a @Cron tick firing while the previous pass is still running becomes a no-op instead of double-processing the same payments (duplicate provider calls, doubled reconciliationAttempts). Also adds a test processing a full 500-payment batch to prove the PAYMENT_RECONCILE_MAX_BATCH cap doesn't silently drop candidates. True wall-clock throughput against a real provider is a staging soak-test concern, not something a synchronous unit test can honestly prove — the overlap guard is the structural safeguard for the case where a pass does run long. Closes DistinctCodes#1701
…ouble-provision KeyCustodyService.provisionKeypair() and WalletsService's wallet-creation path rely on the same first-write-wins (user_id) unique constraint the ledger's overdraft race already gets a dedicated test for, but had no equivalent proof. Fires several genuinely concurrent provisionCustodialWallet calls for the same user through a small in-memory WalletAccount table (serializing transaction callbacks and enforcing the unique index, matching credits/testing/in-memory-ledger.ts's own documented approach) and asserts exactly one wallet is created, every caller converges on it, and the losing calls recover via the existing catch-and-refetch path rather than ever surfacing the raw 23505 to a caller. Closes DistinctCodes#1702
…-more-than-one gap Only the single-role admin case and "holds one of several" were exercised anywhere in the app. Adds the missing "holds none of several accepted roles" case — since UserRole currently has only two values (USER, ADMIN), a requiredRoles list naming both is satisfied by any real user, so this is only reachable via a role value outside the enum (stale JWT, bad seed data), which is exactly the case the guard must not silently allow. "Holds more than one required role at once" isn't representable today: RequestUser.role (authenticated-request.interface.ts) is a single UserRole, not UserRole[], so per the issue's own fallback, that limitation is documented explicitly in a comment rather than left ambiguous. Closes DistinctCodes#1703
…edits.service.ts Pre-existing bug on main, unrelated to DistinctCodes#1700-1703: a copy of credits.service.spec.ts's build() helper was somehow duplicated into the middle of CreditsService itself (between getSystemAccount and getPayableAccount), and the spec file was left with an orphaned second copy of the same helper missing its function signature. Both broke TypeScript compilation outright, which meant nothing in this package — including the four issues above — could even be type-checked, let alone tested. Removing the stray fragments restores both files to valid, working code; no behavioral change.
|
@A6dulmalik is attempting to deploy a commit to the naijabuz's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@a-malik-gh 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! 🚀 |
yusuftomilola
approved these changes
Aug 31, 2026
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
No merge conflicts with main. Good test coverage additions across roles guard, revenue-split, reconciliation, and wallets services - approving.
3 tasks
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.
Four test-coverage fixes across
credits,payments,wallets, andauth:#1700 — atomic revenue-split update validation
RevenueSplitRecipientDtovalidates each recipient's basis points at theDTO layer, but nothing proved
RevenueSplitService.replaceRecipients(backing
PUT .../recipients) rejects a set that individually passesper-field validation but doesn't sum to 10000. Added a test with two such
sets (9999 and 10001 total) and confirmed neither rejected update deletes
the existing recipients first — the validation already ran before the
transaction starts; this just proves it.
#1701 — reconciliation overlap protection + full-batch test
ReconciliationServicehad no bound on how long a fullPAYMENT_RECONCILE_MAX_BATCHpass could take against a slow provider —500 sequential awaits up to
PAYMENT_VERIFY_TIMEOUT_MSeach can exceed the5-minute cron interval. Added a process-local in-progress flag so an
overlapping
@Crontick becomes a no-op instead of double-processing (anda test proving it), plus a test running a full 500-payment batch to prove
the
PAYMENT_RECONCILE_MAX_BATCHcap doesn't silently drop candidates.True wall-clock timing against a real provider is a staging soak-test
concern, not a synchronous unit test — the overlap guard is the structural
fix for the case where a pass does run long.
#1702 — concurrent custodial wallet provisioning
WalletsService.provisionCustodialWallet's doc comment already claims thesame first-write-wins
(user_id)constraint pattern the ledger's overdraftrace gets a dedicated test for, but nothing exercised real concurrency.
Added a test firing 5 genuinely concurrent calls through a small in-memory
WalletAccounttable (serializing transactions and enforcing the uniqueindex, mirroring
credits/testing/in-memory-ledger.ts's own approach) —exactly one wallet is created, every caller converges on it, and
KeyCustodyService.provisionKeypairis only ever called once.#1703 — RolesGuard none-of-several case
Only the single-role case and "holds one of several" were tested. Added
the missing "holds none of several accepted roles" case. Since
UserRolecurrently has only two values (
USER,ADMIN), arequiredRoleslistnaming both is satisfied by any real user — so this is only reachable via
a role value outside the enum (stale JWT, bad seed data), which is exactly
the case the guard must not silently let through. "Holds more than one
role at once" isn't representable today (
RequestUser.roleis a singleUserRole, notUserRole[]) — documented explicitly in a comment per theissue's own fallback instruction, rather than left ambiguous.
One additional out-of-scope commit:
credits.service.tsandcredits.service.spec.tsonmainhad a stray fragment of the spec'sbuild()helper spliced into the middle ofCreditsServiceitself(between
getSystemAccountandgetPayableAccount), with the spec fileleft holding an orphaned, syntactically-invalid second copy of the same
helper. This broke TypeScript compilation for the whole package — nothing
in
credits/, including the #1700 test above, could type-check withoutit. Removed the stray fragments; no behavioral change.
Tests / checks performed
src/credits/revenue-split.service.spec.ts— all tests passsrc/payments/reconciliation.service.spec.ts— all tests pass (overlap-guard tests confirmed via log output: "Skipping reconciliation pass: the previous pass is still running")src/auth/guards/roles.guard.spec.ts— all tests passnpx tsc --noEmit— zero errors in any file this PR touches (confirmed after thecredits.service.tsfix; a handful of pre-existing, unrelated errors remain elsewhere in the repo — missing modules forcredits-admin.service/payments-admin.service, mismatched controller/DTO signatures in unrelated*.controller.spec.tsfiles, an Expresscookieoverload mismatch inauth.controller.ts— none of them touch anything this PR changes)src/wallets/wallets.service.spec.ts— could not get a clean local run. This sandbox's npm registry connectivity was severely degraded for this session (confirmed via ~12 install attempts acrossnpm install,npm ci, varied retry/timeout/concurrency settings, and detached/nohup runs — each failed a different way:ECONNRESET,ENOTEMPTY/EPERMon a nestedrxjscleanup during dedup, npm's own "Exit handler never called" bug, or a genuine multi-minute stall with zero progress). The one clean install I did briefly get before a later reinstall attempt wiped it showed all 3 other suites above passing 45/45, and this file's only compile blocker was a corrupted@stellar/stellar-sdk/@stellar/stellar-baseinstall (an unrelated third-party package, not this PR's code) — reinstalling just that package fixed the compile error and the file ran, before the environment degraded further. I'm confident in the test's correctness (same in-memory-transaction-table pattern as the ledger's own passing overdraft-race test) but would appreciate CI confirming this suite specifically.Closes
Closes #1700
Closes #1701
Closes #1702
Closes #1703