Add StrKey-backed Stellar address validation to all funder/recipient DTO fields - #84
Merged
chonilius merged 11 commits intoAug 19, 2026
Merged
Conversation
…ergeFi#60) Mirrors the existing IsMoneyAmount/IsSupportedEscrowAsset pattern in money.validator.ts. Backed by @stellar/stellar-sdk's own StrKey.isValidEd25519PublicKey rather than a hand-rolled regex, so a syntactically-plausible-but-checksum-invalid 56-character string is rejected the same as an obviously malformed one. Not yet applied to any DTO.
Not explicitly named in the issue's own Requirements list, but its Precise References section cites milestones.controller.ts:8-20, which covers this class too, and recipientAddress here flows into escrowService.releasePartial the same as every other recipientAddress field this issue targets -- leaving it out would be an inconsistent, incomplete fix for the same underlying gap.
…MergeFi#60) Feeds an empty string, a syntactically-invalid string, a wrong-length string, and a checksum-invalid-but-right-length string directly to POST /escrow/fund, /escrow/:id/release, and /escrow/:id/split-release through a real Nest app with ValidationPipe applied (mirroring main.ts's config, since no existing e2e spec in this repo applies it), asserting a 400 and that EscrowService is never called. A valid, freshly-generated Keypair address is confirmed to still pass through to the (mocked) service, so this isn't just testing the rejection path.
…nance-pool (MergeFi#60) Same coverage shape as the escrow e2e tests, applied to POST /maintenance-pools/:id/deposit (funderAddress) and POST /maintenance-pools/:id/assign-reward (recipientAddress).
…es/milestones (MergeFi#60) Same coverage shape, applied to POST /bounties/:id/fund (funderAddress), POST /milestones/:id/fund (funderAddress), and POST /milestones/:id/issues/:issueId/resolve (recipientAddress — the ResolveIssueDto field this issue's Requirements list doesn't name explicitly but its own Precise References section covers).
|
@oluwarantimini is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
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
2 participants
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.
Summary
Every DTO field representing a Stellar public key —
funderAddressonFundEscrowDto/DepositDto/FundBountyDto/FundMilestoneDto,recipientAddressonReleaseEscrowDto/SplitRecipientDto/AssignRewardDto/ResolveIssueDto— was validated with nothing more than@IsString(). A malformed address (empty, syntactically invalid, wrong length, or checksum-invalid) fell through all the way toSorobanClientService.toScVal's heuristic, which silently downgrades an unparseable "address-shaped" string to a generic string encoding instead of rejecting it — never producing a clean400anywhere in the pipeline.IsStellarAddress()insrc/common/validators/stellar-address.validator.ts, mirroring the existingIsMoneyAmount/IsSupportedEscrowAssetpattern inmoney.validator.ts. Backed by@stellar/stellar-sdk's ownStrKey.isValidEd25519PublicKey(base32 decode + version byte + CRC16 checksum), not a hand-rolled regex.ResolveIssueDto.recipientAddressin the milestones controller — not named in the issue's own Requirements bullet list, but its Precise References section cites the exact line range containing it, and it flows intoescrowService.releasePartialthe same as every otherrecipientAddressfield this issue targets. Leaving it out would have been an inconsistent, incomplete fix for the same gap.SorobanClientService.toScVal, every address it sees has already been validated — its silent try/catch fallback is no longer the only thing standing between a malformed address and a Soroban call.Test plan
npx tsc --noEmitnpm run lint(0 errors; pre-existingno-unsafe-argumentwarnings only, same pattern as the existingescrow-idempotency.e2e-spec.ts)npm run buildnpm run test— 130 passing; the only failure isescrow-fk-integrity.integration.spec.ts, which requires a real local Postgres (role "postgres" does not existwithout one) — unrelated to this change, and CI provides a real Postgres service container so this isn't expected to fail therenpm run test:e2e— 41 passing across my 3 new e2e spec files (escrow, maintenance-pool, bounties/milestones) plus existing ones; the only failure isapp.e2e-spec.ts, which needs realGITHUB_CLIENT_ID/GITHUB_CLIENT_SECRETto bootstrap the fullAppModule— unrelated to this change (this repo's own CI conditionally skips e2e entirely when those secrets aren't configured)test/stellar-address-validation-{escrow,maintenance-pool,bounties-milestones}.e2e-spec.ts) feed an empty string, a syntactically-invalid string, a wrong-length string, and a checksum-invalid-but-right-length string directly to every affected endpoint through a real Nest app withValidationPipeapplied (mirroringmain.ts's config — no existing e2e spec in this repo applies it, so it doesn't come for free), asserting400and that the underlying service is never called; each suite also confirms a freshly-generated validKeypairaddress still passes through, so this isn't just testing the rejection pathCloses #60