feat: add SDK transaction build validation pipeline (#249) - #417
Merged
El-swaggerito merged 1 commit intoJul 28, 2026
Merged
Conversation
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.
Related Issue
Implementation Scope
Problem addressed: the duplication was literal, not hypothetical.
src/payments/index.tshand-chainedvalidateSecretKey,validatePublicKey,validateAmountandvalidateMemoInputinsendXLM, and the same four calls reappeared insendAssetwithvalidateAssetSpecappended. Neither path used the non-throwing family that already existed, and the seventh validator the issue lists — signer capability — had no call site at all:mapAuthRequirementsandassertAuthFullyMapped(src/transactions/auth.ts:162and:286) appear only in two barrel re-exports and one doc comment.The reason the existing helpers could not simply be called in sequence is that they return four different shapes:
safeValidateMemo(src/utils/memo.ts:217){ valid } | { valid, error }safeParseAmount(src/utils/amount.ts:261){ valid, amount } | { valid, error }safeValidateDestination(src/payments/destination-validation.ts:474)Promise<PocketPayResult<…>>validateSendXLMParams(src/payments/validation.ts){ ok } | { ok, errors: ValidationError[] }Normalising those into one result is the work here.
Modules changed:
src/transactions/build-validation.ts(new),src/payments/index.ts,src/transactions/index.ts,src/index.ts,tests/build-validation.test.ts(new),docs/transaction-build-validation.md(new)Public API impact: new exports only, no breaking change.
validateTransactionBuild,assertTransactionBuildValid,VALIDATION_ORDER, and the typesValidationStage,TransactionValidationIssue,TransactionValidationResult,TransactionBuildInput,TransactionValidationOptions. No published error code changes — see below.Design
Seven stages in a documented, exported order (
VALIDATION_ORDER): local andzero-cost first, anything reading configuration or an account last, so a
malformed key is reported without resolving config at all.
Stages are not short-circuited — three malformed fields produce three
issues, so a form can show all of them at once.
assertTransactionBuildValidis the throwing variant for call sites that wantonly the first failure.
Error codes are transported, never merged. An issue carries the code its
originating validator already produced.
ValidationErrorCode(
src/payments/validation.ts:29) and the published registry are separatetaxonomies on purpose, and both appear in the same
issuesarray unchanged.That constraint also decided one implementation detail worth flagging: the
amount stage uses
validateAmount, notsafeParseAmount. The safe parserbelongs to the safe-amount model and raises that model's codes, while the
payment surface publishes
INVALID_AMOUNTandINVALID_AMOUNT_PRECISION.Composing the safe parser would have been a silent breaking change — four tests
caught it during development.
Tests Added / Changed
tests/build-validation.test.ts— 22 testsThe block that matters most is the last one: adoption must not change a single
published error code. It asserts that
sendXLMstill reportsINVALID_SECRET_KEYandINVALID_AMOUNT, that each flow keeps its ownself-payment wording ("Cannot send XLM to yourself" vs "Cannot send asset to
yourself"), and that
sendAssetstill validates the asset spec.One ordering test exists because of a real bug found during development: with a
malformed secret and a valid destination, the self-payment check would still
call
Keypair.fromSecret, which throws a rawErrorrather than aPocketPayError. The source key is now derived only once the secret is known tobe well formed.
Verification
verifychain step by stepThe steps were run individually rather than through
npm run verify, becausethat script shells out to
npm runfor each sub-step and this repository'sdependency tree is managed with a different package manager locally.
test:coveragewas not run; every other step was:Baseline measured on a clean
git archiveofupstream/main(cf8e166):maintscerrorscheck:circularZero regressions, +22 tests.
The 2
tscerrors and the dependency cycle are both insrc/network/fee.ts,introduced by #415, and are present on
main:They are left alone deliberately — they are unrelated to this issue and belong
to the module's author.
pnpm buildfails because of them, onmainas well ashere. This PR does not claim otherwise.
CI Status
verify:prends red because its gate runs the whole suite and the type check,and both are already failing on
mainfor the reasons above.Acceptance Criteria Coverage
src/transactions/build-validation.ts, usable from any flowVALIDATION_ORDERis exported so the order is testable, anddocs/transaction-build-validation.mdexplains why local checks run before configurationTransactionValidationIssue[]from the non-throwing entry point; the originatingPocketPayErrorfrom the throwing onesendXLMandsendAssetboth route through the pipeline; the hand-chained calls are gone from bothReviewer Notes
src/payments/index.ts:33-36and:319-323. They are now at:34-37and:329-333— the lines moved when the transaction lifecycle orchestrator (Implement SDK transaction lifecycle orchestrator #305) landed. The chains themselves were exactly as described.mapAuthRequirementstakes a built transaction, so full threshold analysis cannot run before a transaction exists. ThesignerCapabilitystage validates what is knowable pre-build — whether the supplied account declares signing capability — and the docs say plainly that authorisation mapping remains a post-build step. The stage is skipped when no account is supplied.sendXLMandsendAssetskip thenetworkstage via thestagesoption, because they resolve configuration themselves inside their owntry. Including it would have moved when a configuration error surfaces relative to their error handlers — a behaviour change this PR has no reason to make.selfPaymentMessage. Both wordings are asserted by tests.Scope — what was deliberately left out
tscerrors and the dependency cycle insrc/network/fee.ts. Unrelated to this issue; fixing them would be scope creep into another contributor's module.safeValidateDestinationandsafeCheckDestinationTrustlineare not wired in. Both areasyncand perform network lookups; the pipeline's stages are synchronous and local by design, so a caller can validate a form without touching Horizon. Network-level destination checks stay where they are, in the preflight thatsendAssetalready runs.validateSendXLMParamsis left as-is. It remains the payment-shaped, form-facing entry point; the pipeline does not replace it, and unifying the two would have merged the taxonomies this PR deliberately keeps apart.