Add deposit policy enforcement and coin-based withdrawal constraints#35
Add deposit policy enforcement and coin-based withdrawal constraints#35
Conversation
Add optional `coins?: string[]` to `WithdrawRuleEntry` and create new `DepositRuleEntry` type with exchange, network, and coins fields. Update `PolicyConfig.deposit` from `Record<string, null>` to accept an optional array of deposit rules. Update Joi schema to validate the new coins arrays and deposit rule structure. Extend `normalizePolicyConfig()` to uppercase coin values in both withdraw and deposit rules. Backward compatible: policies without coins field and empty deposit objects remain valid.
Activate the previously-ignored ticker parameter to gate withdrawals by the rule's coins list. After matching the highest-priority withdraw rule by exchange/network, check the ticker against rule.coins when present. Behavior: coins absent, empty, or ["*"] allows any token. Otherwise only listed coins pass; mismatches return an error with the allowed set. No fallthrough to lower-priority rules.
Add getDepositRulePriority() mirroring the withdraw counterpart with wildcard support and priority tiers (exact > partial > wildcard). Rewrite validateDeposit() to check exchange, network, and coin against policy deposit rules. Highest-priority rule wins with no fallthrough on coin mismatch. Empty/absent rules preserve backward compatibility by allowing all deposits. Wire validation into the FetchDepositAddresses handler, rejecting with PERMISSION_DENIED when policy is violated. Replace stub tests with comprehensive suite covering coin filtering, wildcard handling, priority conflicts, case insensitivity, and backward-compatible empty policy cases.
Document withdraw `coins` support and the new deposit rule structure in `POLICY.md`. Refresh `policy.example.json` and `policy.backtest.json` to show token-scoped withdraw rules and deposit rules that match the current schema. This keeps the primary policy reference and shipped examples aligned with the latest validation behavior while leaving production and staging policies untouched.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
WalkthroughThe PR adds per-rule token/coin allowlists to withdraw and deposit policies, formalizes deposit.rules with exchange/network/coins matching and priority, enforces token checks in validateWithdraw/validateDeposit, and integrates deposit validation into the FetchDepositAddresses handler. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Server
participant PolicyValidator as Policy Validator
participant RuleMatcher as Rule Matcher
Client->>Server: FetchDepositAddresses(exchange, chain, symbol)
Server->>PolicyValidator: validateDeposit(policy, exchange, network, ticker)
PolicyValidator->>RuleMatcher: Find best-priority rule for (exchange, network)
RuleMatcher-->>PolicyValidator: matched rule or null
alt no deposit.rule OR matched rule allows all coins
PolicyValidator-->>Server: { valid: true }
else matched rule with coins list
PolicyValidator->>PolicyValidator: normalize ticker & coins (UPPER/trim)
alt ticker in coins
PolicyValidator-->>Server: { valid: true }
else
PolicyValidator-->>Server: { valid: false, error: "token not allowed" }
end
else no matching rule
PolicyValidator-->>Server: { valid: false, error: "exchange/network not allowed" }
end
alt validation.valid === true
Server->>Server: Fetch addresses from exchange
Server-->>Client: Address list
else
Server-->>Client: PERMISSION_DENIED + error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
src/helpers/index.ts (1)
378-398: Consider extracting common priority logic.
getDepositRulePriorityandgetWithdrawRulePriority(lines 356-376) have identical implementations. While duplication is acceptable for now, consider extracting to a shared helper if more rule types are added in the future.♻️ Optional refactor to reduce duplication
+function getRulePriority( + rule: { exchange: string; network: string }, + exchange: string, + network: string, +): number { + const exchangeMatch = rule.exchange === exchange || rule.exchange === "*"; + const networkMatch = rule.network === network || rule.network === "*"; + if (!exchangeMatch || !networkMatch) { + return 0; + } + if (rule.exchange === exchange && rule.network === network) { + return 4; + } + if (rule.exchange === exchange && rule.network === "*") { + return 3; + } + if (rule.exchange === "*" && rule.network === network) { + return 2; + } + return 1; +} + +const getWithdrawRulePriority = getRulePriority; +const getDepositRulePriority = getRulePriority;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/index.ts` around lines 378 - 398, getDepositRulePriority and getWithdrawRulePriority contain identical logic; extract the shared matching/prioritization into a single helper (e.g., computeRulePriority or getRulePriority) and have both getDepositRulePriority and getWithdrawRulePriority delegate to it. Update the helper to accept (rule, exchange, network) and return the same numeric priorities, keep existing behavior for wildcards and exact matches, and replace duplicate bodies in both functions with a call to the new helper to remove duplication while preserving current semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/helpers/index.ts`:
- Around line 378-398: getDepositRulePriority and getWithdrawRulePriority
contain identical logic; extract the shared matching/prioritization into a
single helper (e.g., computeRulePriority or getRulePriority) and have both
getDepositRulePriority and getWithdrawRulePriority delegate to it. Update the
helper to accept (rule, exchange, network) and return the same numeric
priorities, keep existing behavior for wildcards and exact matches, and replace
duplicate bodies in both functions with a call to the new helper to remove
duplication while preserving current semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f32282ca-b125-42fb-af44-30b68b04b6d4
📒 Files selected for processing (7)
POLICY.mdpolicy/policy.backtest.jsonpolicy/policy.example.jsonsrc/helpers/index.tssrc/server.tssrc/types.tstest/helpers.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T17:09:55.684Z
Learnt from: victorshevtsov
Repo: usherlabs/cex-broker PR: 23
File: src/client.dev.ts:72-76
Timestamp: 2026-01-29T17:09:55.684Z
Learning: In the cex-broker codebase, Action.FetchAccountId must remain implemented in src/server.ts to support external consumers like fiet-prover that depend on fetching account IDs from CEX endpoints; removing or replacing this handler breaks backward compatibility.
Applied to files:
src/server.ts
🪛 LanguageTool
POLICY.md
[grammar] ~311-~311: Use a hyphen to join words.
Context: ...lidates the token against it. #### Rule matching priority Same priority scheme ...
(QB_NEW_EN_HYPHEN)
[style] ~412-~412: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ch (or wildcard-match) the request. - Withdraw token rejected: ensure the matched `w...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (14)
policy/policy.example.json (1)
1-48: LGTM!The example policy file is well-structured, demonstrates the new
coinsallowlist feature for bothwithdrawanddepositrules, and correctly shows wildcard usage for exchange matching. The structure aligns with the updatedPolicyConfigtype and documentation.src/server.ts (1)
618-632: LGTM!The deposit validation integration is correctly placed after payload parsing and before the actual deposit address fetch. The validation follows the same pattern as
validateWithdrawin theWithdrawaction, usingPERMISSION_DENIEDstatus for policy violations. The parameter mapping (cex→ exchange,fetchDepositAddresses.chain→ network,symbol→ ticker) aligns with thevalidateDepositfunction signature.policy/policy.backtest.json (1)
1-65: LGTM!The backtest policy file provides a comprehensive configuration with multiple exchange/market combinations for testing. It correctly demonstrates the wildcard exchange pattern and maintains structural consistency with the example policy and documented schema.
src/types.ts (2)
4-15: LGTM!The type definitions correctly model the extended policy structure:
WithdrawRuleEntry.coinsis optional for backward compatibilityDepositRuleEntryappropriately omitswhitelistsince deposits don't have destination addresses to validate- Both types share the same
coinsconstraint pattern
31-33: LGTM!Making
deposit.ruleoptional (rule?: DepositRuleEntry[]) correctly enables backward compatibility — policies with"deposit": {}will allow all deposits, while policies with explicit rules will enforce them.POLICY.md (2)
133-151: LGTM!The
withdraw.rule[].coinsdocumentation clearly explains the optional token allowlist semantics, including wildcard behavior (["*"]or omitted means allow all), case-insensitive matching, and backward compatibility with rules written beforecoinswas added.
289-394: LGTM!The deposit policy documentation is comprehensive and well-structured. It correctly documents:
- Backward compatibility (
"deposit": {}allows all)- Rule matching priority (same scheme as withdraw)
- Optional
coinsconstraints with wildcard semantics- The important distinction that deposit validation gates
FetchDepositAddressesonlytest/helpers.test.ts (3)
145-157: LGTM!Good backward compatibility test ensuring existing policies without
coinsfield continue to work — any ticker is allowed when the field is absent.
239-401: LGTM!Comprehensive test coverage for withdraw coin restrictions including:
- Allowed/rejected ticker scenarios
- Wildcard
["*"]and empty[]array semantics- Case-insensitive matching
- Priority-based rule selection without fallthrough on coin mismatch
The error message assertions (
toContain) ensure user-facing messages include the rejected ticker and allowed coins list.
691-821: LGTM!Thorough test coverage for deposit validation including:
- Coin allowlist enforcement
- Backward compatibility with empty/missing rules
- Wildcard and empty array semantics
- Exchange/network rejection when rules exist but don't match
- Priority-based rule selection
- Case-insensitive matching across exchange, network, and coin
src/helpers/index.ts (4)
257-264: LGTM!The Joi schemas correctly define
coinsas an optional array for both withdraw and deposit rules, maintaining backward compatibility with existing policies.
325-342: LGTM!The normalization logic correctly handles optional fields:
coinsis only normalized when present, avoiding the addition of empty arrays to rules that don't specify themdeposit.rulenormalization is conditional, preserving the distinction between{}(no rules) and{ rule: [] }(empty rules array)
438-448: LGTM!The coin validation logic correctly handles all wildcard semantics:
coinsundefined → allow all (backward compat)coins: []→ allow all (empty array treated as no restriction)coins: ["*"]→ allow all (explicit wildcard)coins: ["ETH", "USDT"]→ only allow listed tokensThe case-insensitive comparison via
tickerNormis appropriate.
695-744: LGTM!The
validateDepositimplementation is correct and mirrors thevalidateWithdrawpattern appropriately:
- Returns valid when no rules exist (lines 703-708) for backward compatibility
- Uses priority-based rule selection (lines 714-722)
- Enforces coin restrictions with the same wildcard semantics (lines 731-742)
- Normalizes all inputs for case-insensitive matching (lines 710-712)
outerlook
changed the title
rsoury
left a comment
rsoury
left a comment
There was a problem hiding this comment.
Only consideration here is that a deposit rule is redundant.
If the CEX wallet address is exposed, there is no way to reject deposits (transfers) to that address.
Therefore, does not make sense operationally to concern with deposit gating.
However, withdrawal gating is definitely ok.
|
Also, please merge updates into |
I thought a bit about it as well, and I agree that it might be confusing because, really, the CEX-Broker has no power to prohibit the user from depositing whatever he wants... but since the way to get an address also stipulates a network + token before this change, it ends up being one more line of defense against bad behaviors. I think about tokens that Binance supports or not (thinking about weth example). I imagine Binance also has this kind of guardrail on their fetch address method, which may become redundant here Then it's more like a guardrail than an authorization in this case. If it's harmful for being confusing, I remove it, but if it's harmless, it could be ok |
|
Fair — happy for you to merge in that case. |
2 participants
related to https://linear.app/usherlabs/issue/FIET-718/cex-broker-add-token-restrictions-to-withdrawdeposit-policies
Summary by CodeRabbit
New Features
Documentation
Tests