Spike: escrow on-chain write path — recommend client-signed, prototype release builder - #186
Conversation
…ned release builder EscrowService.release()/raiseDispute()/create() only ever mutate an in-memory map — none of them has ever built, signed, or submitted a Soroban transaction, even though event-ingestion already reads chain state back into EscrowService. This resolves the ambiguity by recommending a client-signed design (backend builds/simulates unsigned XDR, never holds a signing key) over server-side custody, since no key-custody infrastructure exists anywhere in this repo and custody wouldn't even skip the existing event-ingestion round-trip. Prototypes the recommendation for the release action: EscrowReleaseTransactionBuilderService builds and simulates an unsigned release invocation via GET /escrows/:id/release/transaction, mirroring how SorobanEscrowChainStateClient already reads chain state. Closes trustflow-protocol#180
Reviewed the spike, the new transaction-builder prototype, tests, controller wiring, and the spike doc. This is a thoughtful, low-risk addition that correctly recommends client-signed transactions and provides a focused, well-tested prototype for the release flow. Merge after the small, targeted changes below (mostly testability/DI and a couple of validation/nit fixes). Before I can approve and merge, please address the following backend/src/escrow-write/escrow-release-transaction-builder.service.ts — The service instantiates SorobanRpc.Server(STELLAR_CONFIG.sorobanRpcUrl) inside its constructor, which forces module-level config/side-effect coupling and complicates testing. Make the rpc server (or a Soroban client abstraction) injectable via the constructor or accept an optional rpcServer parameter. Prefer a Nest provider so tests can supply a mocked rpcServer without jest.resetModules/process.env manipulation. Other Possible improvements, although non-blocking, but recommended: backend/src/escrow-write/escrow-release-transaction-builder.service.ts — Centralize the contract entrypoint name and expected argument shape into a single constant or config area (with a TODO to update when the contract repo is available). This makes it easier to update once the contract signature is confirmed. |
…lize address validation Addresses maintainer review on trustflow-protocol#186: - Inject the Soroban RPC server and Stellar config into EscrowReleaseTransactionBuilderService via Nest provider tokens instead of constructing them in the constructor, so tests supply mocks directly instead of mutating process.env and reloading modules. - Validate sourceAccount with a class-validator DTO (BuildReleaseTransactionQueryDto) reusing a shared STELLAR_ADDRESS_REGEX exported from escrow.dto.ts, run through the app's global ValidationPipe, instead of a controller-local regex check. Covered by a Supertest-driven integration test that boots a real Nest app, matching the pattern already used in reputation.controller.spec.ts. - Pull the assumed `release` entrypoint name into a documented constant next to the contract.call() site. - Flesh out the endpoint's OpenAPI docs with a response schema and the intended sign/submit client flow.
|
Thanks for the review — pushed a follow-up commit addressing the blocking items:
Left the logging as-is — the only thing logged is the internal All local checks pass (lint, format, typecheck, 339/339 tests, build) and CI is green. |
meshackyaro
left a comment
There was a problem hiding this comment.
Excellent follow-up commit — all five blocking items are now cleanly addressed. This PR is ready to merge.
What I verified:
-
DI for RPC/config:
SOROBAN_RPC_SERVERandESCROW_WRITE_STELLAR_CONFIGare now properly injected via Nest providers inescrow-write.module.ts. Tests supply mocked RPC directly — nojest.resetModules()orprocess.envmutation. -
Test refactoring: Both the unit tests in
escrow-release-transaction-builder.service.spec.tsand controller tests inescrow.controller.spec.tsare now clean. The new Supertest integration test matches the pattern inreputation.controller.spec.tsand properly verifies that invalidsourceAccountis rejected by the ValidationPipe before any service call. -
Centralized address validation:
STELLAR_ADDRESS_REGEXis now exported fromescrow.dto.tsand reused inBuildReleaseTransactionQueryDto. Validation runs through the globalValidationPipeconsistently. -
Entrypoint constant & docs:
RELEASE_ENTRYPOINTis pulled into a documented constant with the caveat that it's unverified. The OpenAPI docs are comprehensive — they show the intended client flow (build → sign → submit) and clearly explain why the backend never signs. -
Simulation/error handling: The current approach (return XDR + metadata, let caller handle simulation errors) is pragmatic for a spike. Consider (in a follow-up) exposing simulation result summaries if SDKs need to show clients resource costs before signing.
Architecture confidence:
The spike write-up is thorough and the recommendation for client-signed over server-side custody is sound. No custody infrastructure in the repo today, and server custody wouldn't even skip the existing event-ingestion round-trip — so the lower-blast-radius client-signed path is the right call.
Code quality:
- 339/339 tests passing, lint/format/typecheck all clean
- Follow-up issues are scoped and linked from #180
- Blocking unknowns (unverified contract shape, no live testnet, unknown auth) are explicitly flagged
Great job. This sets the pattern for the remaining actions (create, raiseDispute) and leaves the repository in a much clearer state on the escrow write-path question. Well done @kris-nana!
Summary
Closes #180
EscrowService.create/release/raiseDisputeare purely in-memory today — none of them ever builds, signs, or submits a Soroban transaction, even thoughevent-ingestionalready reads chain state back intoEscrowService. This spike:event-ingestionwould observe it (all three would).EscrowServicealready depends on.releaseaction:EscrowReleaseTransactionBuilderServicebuilds and simulates an unsigned Sorobanreleaseinvocation (GET /escrows/:id/release/transaction), mirroring howSorobanEscrowChainStateClientalready reads chain state. The backend never signs or submits — the caller's wallet does, and the resulting event flows back through the existing, unmodifiedevent-ingestionpipeline.stellar.service.ts/soroban.helper.ts.release).Full write-up:
backend/ESCROW_WRITE_PATH_SPIKE.md.Follow-up implementation issues filed and linked from #180.
Test plan
npm run lint:check— clean (pre-existing warnings only, unrelated to this change)npm run format:check— cleannpx tsc --noEmit— cleannpm run test:ci— 337/337 tests passing (5 new tests for the prototype, plus updatedEscrowControllertests)npm run build— clean