Skip to content

feat: support priced ERC-20 and native escrows - #9

Merged
g4titanx merged 7 commits into
mainfrom
feat/priced-single-escrows
Aug 7, 2026
Merged

feat: support priced ERC-20 and native escrows#9
g4titanx merged 7 commits into
mainfrom
feat/priced-single-escrows

Conversation

@g4titanx

@g4titanx g4titanx commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

The SDK now deploys EscrowERC20 for a single ERC-20 transfer, EscrowNative for a single native transfer, and EscrowBatch for multiple rows. The selected type is passed consistently to pricing and obfuscation, stored in TransferSecrets, and reused during compliance and resume. A quote returning a different artifact type is rejected before deployment.

@g4titanx

g4titanx commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @g4titanx's task in 3m 36s —— View job


Claude finished reviewing this PR

  • Gather context (diff, changed files)
  • Understand the request (code review)
  • Review core logic changes (transfer.ts, bond.ts, api.ts, types.ts, errors.ts)
  • Review test coverage changes
  • Post final review feedback

Note: npm run check/npm test could not be run — Bash tool calls beyond read-only git/grep operations require interactive approval that isn't available in this context. If you'd like me to verify the build/tests, update the --allowedTools config to permit npm run *.


Summary

Clean implementation of EscrowERC20/EscrowNative/EscrowBatch selection. selectEscrowType (src/transfer.ts:94-97), the bond.ts rename to deriveBlindedSigners, and the EscrowKind threading through api.ts/nomad.ts compliance calls are all consistent with the stated policy in CLAUDE.md (single ERC-20/native row vs. multi-row batch). The fresh-quote path correctly rejects a mismatched artifact from pricing (INVALID_PRICING_QUOTE, covered by a new test in test/transfer-pricing.test.ts). The mock API's per-type constructor encoding looks correct and mirrors the real contracts' expected ABI shapes.

Finding: resume validation doesn't cross-check escrowType: "batch" against row count

quoteFromResume (src/transfer.ts:207-237) builds escrowMatchesRows like this:

const escrowMatchesRows =
  resume.escrowType === "batch" ||
  (rows.length === 1 &&
    ((resume.escrowType === "native" && isNativeToken(rows[0].tokenAddress)) ||
      (resume.escrowType === "erc20" && !isNativeToken(rows[0].tokenAddress))));

For "native"/"erc20" this correctly requires rows.length === 1 and a matching token kind. But for "batch" it short-circuits to true unconditionally — there's no check that rows.length > 1, and no check that the resumed row count matches whatever rows.length was used to derive the original batch signers. A resume call with escrowType: "batch" passes validation regardless of how many rows are supplied this time (even a single row).

This isn't just theoretical — the test suite's own VALID_RESUME fixture (test/transfer-pricing.test.ts:14-29) sets escrowType: "batch" while being exercised against single-row params (tokenAddress/recipientAddress/amount, not transfers[]) throughout the "rejects resume data missing %s" parameterized tests, which only works because this branch is unguarded.

Consequences beyond the validation gap itself: buildContext uses escrowType = params.resume?.escrowType ?? selectEscrowType(rows) and skips selectEscrowType/row-count reconciliation entirely on resume, and context.rows.length (the row count as seen on this call) later feeds polling's total (src/transfer.ts:621). If a caller resumes a batch transfer with a different row count than was actually deployed (typo, stale params, partial transfers[]), nothing raises INVALID_RESUME — it can silently proceed into compliance/signal/poll with a row count that doesn't match the deployed EscrowBatch.

Suggested fix — require the row count to actually match for batch too, e.g. via a rows.length > 1 check for the batch branch (exact row-count parity can't be verified from TransferSecrets alone since it doesn't persist the original row count, but the rows.length > 1 guard at least matches the invariant selectEscrowType enforces on the fresh-quote path):

const escrowMatchesRows =
  (resume.escrowType === "batch" && rows.length > 1) ||
  (rows.length === 1 &&
    ((resume.escrowType === "native" && isNativeToken(rows[0].tokenAddress)) ||
      (resume.escrowType === "erc20" && !isNativeToken(rows[0].tokenAddress))));

Fix this →

Minor observations (no action needed)

  • The bond.ts rename (deriveBatchBlindedSignersderiveBlindedSigners) is applied consistently; no stale references remain in src/ or test/.
  • INVALID_PRICING_QUOTE reuses the free-form MirageError code string pattern already used elsewhere (code: string, no enum), so it's consistent with existing error handling.
  • Doc updates in CLAUDE.md/README.md accurately reflect the new escrow-selection policy.

Branch: feat/priced-single-escrows

@ozwaldorf

Copy link
Copy Markdown
Member

ci failing

@g4titanx

g4titanx commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

ci failing

would need to merge escrow pr first

@ozwaldorf

Copy link
Copy Markdown
Member

ci failing

Comment thread src/types.ts
@g4titanx
g4titanx merged commit 0fd31af into main Aug 7, 2026
5 checks passed
@ozwaldorf
ozwaldorf deleted the feat/priced-single-escrows branch August 7, 2026 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants