feat(deposit): auto-clear Binance travel-rule frozen deposits (0.2.17) - #53
Conversation
AUSTRAC's travel rule (live 2026-07-01) also freezes INBOUND Binance deposits — credited but held in getUserAsset.freeze, invisible to free+locked balances — until a per-deposit questionnaire is answered. The maker's funding churn re-freezes capital every cycle. This adds the deposit counterpart to the already-shipped withdraw leg. A broker-internal reconciler polls localentity/deposit/history and submits PUT localentity/deposit/provide-info ONLY for deposits whose on-chain sender (eth_getTransactionByHash().from, via a configured RPC) is a policy-declared originator. Undeclared origin, unresolvable sender, non-AU entity and FAILED status all fail closed (skip + surface), never auto-attesting an origin we cannot prove is ours. The core (reconcileAccountOnce) takes injected I/O so these invariants are unit-tested without a network. - travel-rule.ts: australiaDepositQuestionnaireSchema (self-owned only; distinct shape from the withdraw questionnaire so a copy-paste fails at policy-load), getEnabledTravelRuleDepositConfig, resolveDepositOriginatorQuestionnaire, registerBinanceTravelRuleDepositEndpoints. - travel-rule-deposit-reconciler.ts (new): reconciler + on-chain origin proof + env config. Idempotent by tranId, per-tranId backoff (no hot-loop), account-wide rate-limit cooldown, active/idle cadence, AU entity gate. Metrics: frozen gauge + submissions/anomalies counters; every submission audit-logged. - ccxt patch: sign localentity/deposit/provide-info raw (else -1022), mirroring the withdraw fix; guarded by a real-ccxt regression test. - Wired into applyCommonExchangeConfig (endpoints) and CEXBroker run()/stop() (lifecycle, restarts on policy hot-reload). Self-disables when no policy has travelRule.rule[].deposits.enabled. - Bump to 0.2.17; document TRAVEL_RULE_RPC_URL_<NETWORK> in .env.sample.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds Binance travel-rule deposit support: new deposit types and policy validation, Binance endpoint wiring, a polling reconciler that clears frozen deposits after on-chain and questionnaire checks, broker lifecycle integration, ccxt patch updates, docs, version bump, and tests. ChangesTravel-rule deposit reconciliation feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CEXBroker
participant TravelRuleDepositReconciler
participant Binance API
participant On-chain RPC
CEXBroker->>TravelRuleDepositReconciler: start()
TravelRuleDepositReconciler->>Binance API: fetch localentity/deposit/history
TravelRuleDepositReconciler->>On-chain RPC: eth_getTransactionByHash(txHash)
On-chain RPC-->>TravelRuleDepositReconciler: sender address or null
TravelRuleDepositReconciler->>Binance API: submit localentity/deposit/provide-info
CEXBroker->>TravelRuleDepositReconciler: stop()
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
258-276: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winShutdown ordering can race an in-flight reconciler tick against
otelMetrics.close().
depositReconciler.stop()is fire-and-forget and, per its implementation, only clears the timer/flag — it does not wait for a currently in-flight#tick()to finish:stop(): void { this.#stopped = true; if (this.#timer) { clearTimeout(this.#timer); this.#timer = null; } }If
stop()(inCEXBroker.stop(), lines 263-266) is called while a tick is awaitingfetchDepositHistory/resolveSender/submitProvideInfo, that tick keeps running to completion and still calls#emit(), which doesvoid metrics?.recordGauge(...)/recordCounter(...)— right afterthis.otelMetrics.close()has already run a few lines later (line 270-272). If the OTel exporter throws/rejects post-close, thevoid-discarded promise becomes an unhandled rejection, and on Node 15+ the default behavior is to terminate the process with a non-zero exit code — turning a graceful shutdown into a crash. The same overlap window exists more briefly inrun()'s hot-reload teardown (lines 285-290), though without theclose()step it's lower risk there (just a possible double metrics emission during the handoff).A minimal mitigation is to make the reconciler's
stop()return a promise that resolves once any in-flight tick settles, and await it before closing metrics:🔧 Proposed fix
public async stop(): Promise<void> { if (this.#policyFilePath) { unwatchFile(this.#policyFilePath); log.info(`Stopped watching policy file: ${this.#policyFilePath}`); } if (this.depositReconciler) { - this.depositReconciler.stop(); + await this.depositReconciler.stop(); this.depositReconciler = undefined; } if (this.server) { await this.server.forceShutdown(); }This requires
TravelRuleDepositReconciler.stop()(insrc/helpers/travel-rule-deposit-reconciler.ts, not in this cohort's files) to becomeasyncand await its own in-flight tick promise before returning.Also applies to: 285-290, 318-328
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 258 - 276, The shutdown flow in CEXBroker.stop() can close otelMetrics while an in-flight TravelRuleDepositReconciler tick is still emitting metrics, so update the reconciler lifecycle to wait for any active tick to settle before proceeding. Make depositReconciler.stop() async (or otherwise await its in-flight work) and change CEXBroker.stop() to await that stop before calling otelMetrics.close() and otelLogs.close(); use the existing symbols depositReconciler.stop(), CEXBroker.stop(), and TravelRuleDepositReconciler.#tick/#emit to locate the change.
🧹 Nitpick comments (2)
src/helpers/travel-rule.ts (1)
171-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate entry-lookup logic vs
resolveTravelRuleDecision.This re-implements the same "find
TravelRuleEntryby uppercased exchange name" logic already inresolveTravelRuleDecision(lines 96-100). Extracting a sharedfindTravelRuleEntry(policy, exchange)helper would remove the duplication and keep both lookups in sync if the matching semantics ever change.♻️ Proposed refactor
+function findTravelRuleEntry( + policy: PolicyConfig, + exchange: string, +): TravelRuleEntry | undefined { + const rules = policy.travelRule?.rule ?? []; + const exchangeNorm = exchange.trim().toUpperCase(); + return rules.find((rule) => rule.exchange.trim().toUpperCase() === exchangeNorm); +} + export function getEnabledTravelRuleDepositConfig( policy: PolicyConfig, exchange: string, ): TravelRuleDepositConfig | null { - const rules = policy.travelRule?.rule ?? []; - const exchangeNorm = exchange.trim().toUpperCase(); - const entry = rules.find( - (rule) => rule.exchange.trim().toUpperCase() === exchangeNorm, - ); + const entry = findTravelRuleEntry(policy, exchange); const deposits = entry?.deposits; if (!deposits || !deposits.enabled) { return null; } return deposits; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/travel-rule.ts` around lines 171 - 185, The exchange-entry lookup in getEnabledTravelRuleDepositConfig duplicates the same matching logic already used by resolveTravelRuleDecision. Extract a shared helper such as findTravelRuleEntry(policy, exchange) in travel-rule.ts that normalizes and finds the matching TravelRuleEntry, then reuse it in both resolveTravelRuleDecision and getEnabledTravelRuleDepositConfig so the matching behavior stays consistent.test/travel-rule-deposit.test.ts (1)
1-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the pure helpers; scheduling class and
#emitmetrics/log mapping are untested.The suite thoroughly covers
australiaDepositQuestionnaireSchema,getEnabledTravelRuleDepositConfig,resolveDepositOriginatorQuestionnaire,registerBinanceTravelRuleDepositEndpoints,parseLocalEntityDeposit,resolveOnChainSender,loadTravelRuleDepositReconcilerConfigFromEnv, and the compliance-criticalreconcileAccountOnce— all consistent with the upstream contracts (e.g.australiaDepositQuestionnaireSchemarestrictingdepositOriginator/receiveFromto1,getEnabledTravelRuleDepositConfig's enabled-gating). There's no coverage, however, forTravelRuleDepositReconcileritself:hasEnabledExchange,#targets()(per-account fan-out including secondary brokers), the active/idle poll cadence switch in#tick(), or#emit()'s mapping from outcomekindto log level/metric name/tags. The#emitmapping in particular is the audit-log/metrics contract this PR's objectives call out ("Metrics and audit logging for frozen deposits and submissions") — a bug there (wrong metric name, missing tag, wrong log level forundeclared-originvssubmitted) wouldn't be caught by any current test. Givenmetricsandlogare easy to stub, adding a few targeted tests around#emit(or exposing it for testing) would close this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/travel-rule-deposit.test.ts` around lines 1 - 536, Add targeted tests for TravelRuleDepositReconciler because the suite currently covers only the pure helpers and reconcileAccountOnce, leaving hasEnabledExchange, `#targets`(), `#tick`(), and especially `#emit`() unverified. Stub the metrics and log dependencies and assert that `#emit` maps each outcome kind to the correct metric name, tags, and log level, and add a couple of tests for the active/idle cadence in `#tick`() plus per-account fan-out from `#targets`() using the TravelRuleDepositReconciler class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/travel-rule-deposit-reconciler.ts`:
- Around line 58-67: The `parseLocalEntityDeposit` helper is incorrectly
accepting `id` as a fallback for provide-info tranIds, which can map to the
wrong Binance id space. Update `parseLocalEntityDeposit` to only read the
provide-info identifier from `tranId` (and keep the null return when it is
missing), removing the `id` fallback from the `depositField` lookup while
preserving the existing `trId`/tranId handling if needed for that flow.
- Around line 355-411: The candidate-processing loop in
travel-rule-deposit-reconciler should stop once a rate-limit is detected so the
same tick does not keep hammering Binance/RPC. Update the logic around
resolveSender and submitProvideInfo in the candidate iteration to break or
return immediately after isRateLimitError sets state.rateLimitedUntil, while
preserving the existing backoff, submittedTranIds, and outcome handling for
non-rate-limit cases.
- Around line 111-120: The RPC call inside resolveOnChainSender currently has no
timeout, so a stalled fetch can block the reconciler tick. Update the fetch to
use an AbortController-based timeout (or equivalent) around the
eth_getTransactionByHash request, and ensure the timeout is cleared/aborted
after the request completes. Keep the change localized to resolveOnChainSender
in travel-rule-deposit-reconciler.ts so the existing polling flow can recover if
the RPC hangs.
In `@src/helpers/travel-rule.ts`:
- Around line 220-232: The `defineRestApi` mapping for
`localentity/deposit/provide-info` is using an incorrect request cost of `1`
instead of the UID-600 weight expected by Binance docs and the sibling
`localentity/withdraw/apply` endpoint. Update the `sapi.put` registration in
`travel-rule.ts` so `localentity/deposit/provide-info` uses the same cost value
as `localentity/withdraw/apply` (`4.0002`), keeping the rest of the API
registration unchanged.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 258-276: The shutdown flow in CEXBroker.stop() can close
otelMetrics while an in-flight TravelRuleDepositReconciler tick is still
emitting metrics, so update the reconciler lifecycle to wait for any active tick
to settle before proceeding. Make depositReconciler.stop() async (or otherwise
await its in-flight work) and change CEXBroker.stop() to await that stop before
calling otelMetrics.close() and otelLogs.close(); use the existing symbols
depositReconciler.stop(), CEXBroker.stop(), and
TravelRuleDepositReconciler.#tick/#emit to locate the change.
---
Nitpick comments:
In `@src/helpers/travel-rule.ts`:
- Around line 171-185: The exchange-entry lookup in
getEnabledTravelRuleDepositConfig duplicates the same matching logic already
used by resolveTravelRuleDecision. Extract a shared helper such as
findTravelRuleEntry(policy, exchange) in travel-rule.ts that normalizes and
finds the matching TravelRuleEntry, then reuse it in both
resolveTravelRuleDecision and getEnabledTravelRuleDepositConfig so the matching
behavior stays consistent.
In `@test/travel-rule-deposit.test.ts`:
- Around line 1-536: Add targeted tests for TravelRuleDepositReconciler because
the suite currently covers only the pure helpers and reconcileAccountOnce,
leaving hasEnabledExchange, `#targets`(), `#tick`(), and especially `#emit`()
unverified. Stub the metrics and log dependencies and assert that `#emit` maps
each outcome kind to the correct metric name, tags, and log level, and add a
couple of tests for the active/idle cadence in `#tick`() plus per-account fan-out
from `#targets`() using the TravelRuleDepositReconciler class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e60e0972-843c-4bb0-8d75-b33a9a4dfac1
📒 Files selected for processing (10)
.env.samplepackage.jsonpatches/@usherlabs%2Fccxt@0.0.14.patchsrc/helpers/broker.tssrc/helpers/index.tssrc/helpers/travel-rule-deposit-reconciler.tssrc/helpers/travel-rule.tssrc/index.tssrc/types.tstest/travel-rule-deposit.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-14T06:47:01.283Z
Learnt from: csmithington
Repo: usherlabs/cex-broker PR: 38
File: src/client.dev.ts:0-0
Timestamp: 2026-04-14T06:47:01.283Z
Learning: In this codebase, gRPC action constants such as `FetchTicker`, `FetchFees`, and `FetchAccountId` should be sourced from `src/helpers/constants.ts` and imported from there (e.g., used by both `src/client.dev.ts` and `src/server.ts`). Do not import or reference generated proto TypeScript artifacts (for example `./proto/cex_broker/Action`), since those generated files are git-ignored and won’t be available/committed consistently.
Applied to files:
src/helpers/broker.tssrc/types.tssrc/helpers/index.tssrc/index.tssrc/helpers/travel-rule.tssrc/helpers/travel-rule-deposit-reconciler.ts
🪛 ast-grep (0.44.0)
test/travel-rule-deposit.test.ts
[warning] 217-217: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(tempPath, JSON.stringify(policy))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🪛 dotenv-linter (4.0.0)
.env.sample
[warning] 31-31: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (10)
.env.sample (1)
19-31: LGTM!package.json (1)
3-3: LGTM!patches/@usherlabs%2Fccxt@0.0.14.patch (1)
1-29: LGTM!src/helpers/travel-rule-deposit-reconciler.ts (1)
51-56: LGTM!Also applies to: 141-184, 190-262, 426-500, 506-589, 591-609, 611-689, 691-835
src/types.ts (1)
51-97: LGTM!src/helpers/index.ts (1)
17-20: LGTM!Also applies to: 37-51, 112-126
src/helpers/travel-rule.ts (1)
3-8: LGTM!Also applies to: 138-219
src/helpers/broker.ts (1)
7-10: LGTM!Also applies to: 58-63
src/index.ts (1)
9-11: LGTM!Also applies to: 49-49, 318-328
test/travel-rule-deposit.test.ts (1)
1-249: LGTM!Also applies to: 299-536
…limit break, provide-info weight
- parseLocalEntityDeposit: accept only `tranId`, drop the `id`/`trId`
fallback. `id` is the capital/deposit/hisrec id space and provide-info
rejects it ("Deposit request not found"); tranId is the sole valid id.
- resolveOnChainSender: bound the fetch with a 10s AbortController timeout
so a hung RPC can't stall the reconciler tick (candidates resolve
sequentially → a hang blocks the next poll).
- reconcileAccountOnce: break the candidate loop once a rate-limit signal
sets the account-wide cooldown, instead of compounding the -1003 across
the remaining candidates.
- registerBinanceTravelRuleDepositEndpoints: provide-info is a 600-weight
UID write; declare its ccxt cost as 4.0002 (same as capital/withdraw/apply)
instead of 1, which under-throttles the write path.
Adds regression tests for the tranId-only parse and the mid-loop
rate-limit break. Full suite: 329 pass.
1 participant
Why
AUSTRAC's travel rule (live 2026-07-01) freezes inbound Binance deposits too: the deposit is credited but held in
getUserAsset.freeze— invisible to thefree+lockedbalances thatFetchBalances, the maker, HB lanes, and diagnostics read, so the capital looks gone. The maker's funding churn (master-withdraw → owner wallet → sub-redeposit) re-freezes capital every cycle. The withdraw leg was solved in v0.2.15/v0.2.16 (#51, #52); this adds the deposit counterpart.What
A broker-internal origin-proved reconciler that auto-answers the deposit questionnaire so churn-frozen capital self-releases within minutes — policy-driven and fail-closed.
GET /sapi/v1/localentity/deposit/historyforrequireQuestionnaire && travelRuleStatusV2=PENDING, and submitsPUT /sapi/v1/localentity/deposit/provide-infoonly when a deposit's on-chain sender (eth_getTransactionByHash().from, via a configured RPC) is a policy-declared originator.FAILEDstatus all fail closed (skip + surface) — it never attests an origin it cannot prove is ours.reconcileAccountOnceis a pure core with all I/O injected, so these invariants are unit-tested without a network.Changes
src/helpers/travel-rule.ts—australiaDepositQuestionnaireSchema(self-owned only; distinct shape from the withdraw questionnaire —depositOriginator/receiveFromvsisAddressOwner/sendTo— so a copy-paste fails at policy-load),getEnabledTravelRuleDepositConfig,resolveDepositOriginatorQuestionnaire,registerBinanceTravelRuleDepositEndpoints.src/helpers/travel-rule-deposit-reconciler.ts(new) — reconciler + on-chain origin proof + env config. Idempotent bytranId, per-tranId backoff (no hot-loop on content errors), account-wide rate-limit cooldown, active 60s / idle 600s cadence, AU entity gate viaquestionnaire-requirements.localentity/deposit/provide-infosigns the questionnaire raw (else-1022), mirroring the withdraw fix. Guarded by a real-ccxt regression test.applyCommonExchangeConfig(endpoint registration) andCEXBroker.run()/stop()(lifecycle; restarts on policy hot-reload). Self-disables when no policy hastravelRule.rule[].deposits.enabled→ byte-identical current behavior.travel_rule_frozen_depositsgauge +travel_rule_deposit_submissions_total/travel_rule_deposit_anomalies_totalcounters; every submission audit-logged. Chose metrics over changing theFetchBalancesgRPC contract.TRAVEL_RULE_RPC_URL_<NETWORK>in.env.sample.Deploy note
TRAVEL_RULE_RPC_URL_ARBITRUMis deliberately notCEX_BROKER_-prefixed (the credential scan skips it), so under SGX it must be added to the Gramine manifest passthrough allowlist — done on the fiet-maker side.Tests
bun test— 327 pass, 0 fail. New deposit-signing regression guards the ccxt patch across version bumps; existing withdraw regression still green.tsc --noEmitclean.Summary by CodeRabbit
New Features
Bug Fixes