Security: Add Daily Transfer Limits and Step-Up Verification Thresholds#554
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements daily transfer limits and step-up verification for the Ancore wallet, enabling users to configure transfer thresholds that trigger confirmations or block transfers. The feature spans shared types, extension wallet settings/UX, web dashboard send flow, and relayer validation. ChangesDaily Transfer Limits and Step-Up Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@samjay8 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
services/relayer/src/validation/transferPolicy.ts (1)
20-43: ⚡ Quick winThis adapter is currently bypassed by
RelayService, which risks policy drift.
RelayService.validateRelayre-implements policy handling directly instead of calling this wrapper, sorequiresStepUpsemantics here are effectively unused. Route relayer policy checks throughvalidateTransferPolicyConstraintsto keep one mapping path.🤖 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 `@services/relayer/src/validation/transferPolicy.ts` around lines 20 - 43, RelayService.validateRelay is re-implementing policy logic instead of using the wrapper validateTransferPolicyConstraints (so requiresStepUp semantics are ignored); update RelayService.validateRelay to call validateTransferPolicyConstraints(context) and act on its TransferValidationResult (check valid, error.code/message, and requiresStepUp) rather than duplicating policy logic—ensure the RelayService uses the wrapper's return shape (valid, error, requiresStepUp) and removes the inlined policy mapping.
🤖 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 `@apps/extension-wallet/src/screens/Send/ReviewScreen.tsx`:
- Around line 75-86: The UI shows the step-up warning but doesn't require an
explicit acknowledgment before allowing submit; add a controlled acknowledgement
(e.g., a checkbox tied to a new state like stepUpConfirmed) that is rendered
when transaction.policyAction === 'step_up' && transaction.policyMessage and
require this state in the submit gating logic (the same place that currently
checks recipient confirmation / the variable used to disable the Submit button,
e.g., canSubmit or disabled prop) so the Submit remains disabled until
stepUpConfirmed is true for step-up transactions.
In `@apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx`:
- Around line 190-196: The numeric checks for daily and stepUp should reject
non-finite values (like Infinity) in addition to NaN; update the validation in
SecuritySettings (the variables daily and stepUp and the setError calls) to
coerce inputs to numbers if needed and use Number.isFinite (or !isFinite)
combined with the > 0 check (e.g., if (!Number.isFinite(daily) || daily <= 0) {
setError('Daily limit must be a positive number'); return; } and similarly for
stepUp) so infinite values are rejected before persisting.
- Around line 208-210: The timeout started with setTimeout(..., 1500) that calls
onDone() should be tracked and cleared on unmount to avoid delayed navigation;
store the timer id (e.g., const timeoutId = setTimeout(...)) and call
clearTimeout(timeoutId) in the component's cleanup (useEffect return or
componentWillUnmount) so SecuritySettings does not call onDone after the view is
left.
In `@apps/extension-wallet/src/stores/settings.ts`:
- Around line 134-151: The hydrated settings object has duplicate keys
(dailyTransferLimit and transferStepUpThreshold) and a malformed fallback for
transferStepUpThreshold: fix the typo DEFAULTS.transferStepUpThresholdions to
DEFAULTS.transferStepUpThreshold, ensure transferStepUpThreshold uses a numeric
fallback (not the boolean persisted.requirePasswordForSensitiveActions), remove
the duplicated earlier duplicate definitions so each key appears once, and keep
the existing typeof checks (e.g., typeof transferStepUpThreshold === 'number' &&
transferStepUpThreshold >= 0) so transferStepUpThreshold and dailyTransferLimit
fall back to their correct DEFAULTS values.
In `@apps/web-dashboard/src/components/SendFlow.tsx`:
- Around line 25-31: The hardcoded TransferPolicy and todayTotal in SendFlow.tsx
(symbols: userPolicy, TransferPolicy, todayTotal) must be replaced with real
data: fetch or receive the user's policy (dailyLimit, stepUpThreshold) from the
account settings service or parent props/context and compute todayTotal from the
user's transaction history API or state selector before enforcing the flow;
replace the useState initializers with async loading (or useEffect) to populate
these values, add a loading/error state to avoid enforcement until real values
arrive, and ensure downstream checks reference the populated state rather than
fixed literals.
- Around line 54-64: handleSubmit currently allows a 'step_up' policyAction to
submit immediately; change it to require explicit user confirmation by blocking
submission when state.policyAction === 'step_up' unless a dedicated confirmation
flag is set (e.g., state.stepUpConfirmed === true). Concretely: update
handleSubmit to check for 'step_up' and if not confirmed call setState to
open/flag a confirmation UI (or invoke onConfirmStepUp) and return early; when
the user explicitly confirms (toggle state.stepUpConfirmed or call
onConfirmStepUp) allow the submit path to continue and clear the flag. Apply the
same explicit-confirmation guard to the other submit points mentioned (the other
handlers around the same logic referenced by policyAction at lines 166-174 and
209-212) so all 'step_up' flows require the same confirmation flag/UI before
proceeding.
- Around line 41-49: In SendFlow, when amount is empty or invalid the previous
policyAction/policyMessage must be cleared: update the branch around the
validateTransferPolicy call (the logic that currently runs when amount &&
!isNaN(Number(amount))) to include an else path that calls setState and sets
policyAction and policyMessage to neutral values (e.g., null or empty string),
and also ensure non-numeric inputs (invalid Number(amount)) trigger that same
clearing behavior; reference the validateTransferPolicy call, the amount
variable, and the setState updates for policyAction/policyMessage to implement
this.
In `@services/relayer/src/services/relayService.ts`:
- Around line 75-85: The current check trusts request.transferPolicy (policy,
amount, todayTotal) from the client and calls validateTransferPolicy, which is
bypassable; instead, derive limits server-side: remove reliance on
request.transferPolicy, fetch the authoritative policy for the actor/account
(server config/DB), compute the requested transfer amount from the canonical
signed request payload, load persisted todayTotal from your DB/ledger, then call
validateTransferPolicy with those server-side values (use the same
validateTransferPolicy function but pass serverPolicy, computed amount, and
persisted todayTotal) and return the TRANSFER_LIMIT_EXCEEDED error if it blocks.
In `@services/relayer/src/types/requests.ts`:
- Around line 25-30: The new optional transferPolicy field added to the
RelayExecuteRequest type is being stripped by request validation because
relayExecuteRequestSchema (used by the body middleware) doesn't include it;
update relayExecuteRequestSchema to define transferPolicy with the same shape
(policy: TransferPolicy enum/union, amount: number, todayTotal: number) and mark
it optional so parsed req.body retains the field, and ensure related type
bindings (RelayExecuteRequest) stay consistent with the schema so HTTP callers
preserve the policy for enforcement in the execute handler.
In `@TRANSFER_LIMITS_IMPLEMENTATION.md`:
- Around line 324-326: The docs currently state a 1 XLM minimum but the shared
TransferPolicySchema uses .nonnegative() (>=0); update either the schema or the
documentation so they match: either change TransferPolicySchema to enforce a 1
XLM minimum (e.g., replace .nonnegative() with .min(1) on the fields
representing minimum transfer amounts such as stepUpThreshold and dailyLimit) or
clarify in TRANSFER_LIMITS_IMPLEMENTATION.md that the 1 XLM constraint is
UI-only and not enforced by TransferPolicySchema; ensure you reference the
schema symbol TransferPolicySchema and the policy fields stepUpThreshold and
dailyLimit when making the change.
- Around line 200-217: The documented snippet in validateRelay is out of sync:
replace the direct call to validateTransferPolicy(...) with the actual service
API validateTransferPolicyConstraints(...) and handle its returned shape (check
for requiresStepUp flag to trigger step-up flow or map returned error to the
TRANSFER_LIMIT_EXCEEDED response). Specifically, call
validateTransferPolicyConstraints(amount, todayTotal, policy), then if
result.requiresStepUp === true return the step-up response used by
validateRelay, otherwise if result.error present return { valid: false, error: {
code: 'TRANSFER_LIMIT_EXCEEDED', message: result.error.message } } so the
example matches the implementation that uses requiresStepUp and the error
mapping.
- Around line 223-264: The three fenced flow diagrams under "Scenario 1: Basic
Transfer", "Scenario 2: Step-up Transfer (Above Threshold, Within Limit)", and
"Scenario 3: Blocked Transfer (Exceeds Daily Limit)" are missing markdown fence
language tags; update each triple-backtick fence that wraps the flow (the blocks
beginning with "User enters amount: 100 XLM", "User enters amount: 500 XLM", and
"User enters amount: 1500 XLM (today's total: 200)") to use ```text so the
blocks pass markdownlint MD040.
---
Nitpick comments:
In `@services/relayer/src/validation/transferPolicy.ts`:
- Around line 20-43: RelayService.validateRelay is re-implementing policy logic
instead of using the wrapper validateTransferPolicyConstraints (so
requiresStepUp semantics are ignored); update RelayService.validateRelay to call
validateTransferPolicyConstraints(context) and act on its
TransferValidationResult (check valid, error.code/message, and requiresStepUp)
rather than duplicating policy logic—ensure the RelayService uses the wrapper's
return shape (valid, error, requiresStepUp) and removes the inlined policy
mapping.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb687ad2-1510-4911-8d3e-8ea51af9c300
📒 Files selected for processing (16)
TRANSFER_LIMITS_IMPLEMENTATION.mdapps/extension-wallet/src/hooks/__tests__/useTransferPolicy.test.tsapps/extension-wallet/src/hooks/useSendTransaction.tsapps/extension-wallet/src/hooks/useTransferPolicy.tsapps/extension-wallet/src/screens/Send/ReviewScreen.tsxapps/extension-wallet/src/screens/Settings/SecuritySettings.tsxapps/extension-wallet/src/stores/settings.tsapps/web-dashboard/src/App.tsxapps/web-dashboard/src/components/SendFlow.tsxpackages/types/src/__tests__/transfer-policy.test.tspackages/types/src/index.tspackages/types/src/transfer-policy.tsservices/relayer/src/services/relayService.tsservices/relayer/src/types/requests.tsservices/relayer/src/types/responses.tsservices/relayer/src/validation/transferPolicy.ts
Comment on lines
+75
to
+86
| {/* Step-up Verification Warning */} | ||
| {transaction.policyAction === 'step_up' && transaction.policyMessage && ( | ||
| <div className="p-4 rounded-2xl bg-amber-500/10 border border-amber-500/30 flex items-start gap-3 animate-in fade-in slide-in-from-top-2 duration-300"> | ||
| <AlertCircle className="w-5 h-5 shrink-0 text-amber-400 mt-0.5" /> | ||
| <div className="space-y-1"> | ||
| <strong className="block text-[11px] uppercase tracking-widest text-amber-300 font-black"> | ||
| Verification Required | ||
| </strong> | ||
| <p className="text-[10px] text-amber-200 leading-relaxed">{transaction.policyMessage}</p> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Enforce a dedicated step-up confirmation before enabling submit.
For policyAction === 'step_up', the UI currently shows a warning only; Line 157 still gates solely on the generic recipient confirmation. This does not enforce an explicit step-up acknowledgment.
✅ Suggested enforcement pattern
export function ReviewScreen({ transaction, onBack, onConfirm }: ReviewScreenProps) {
const [isConfirmed, setIsConfirmed] = useState(false);
+ const [isStepUpConfirmed, setIsStepUpConfirmed] = useState(false);
@@
{transaction.policyAction === 'step_up' && transaction.policyMessage && (
<div className="p-4 rounded-2xl bg-amber-500/10 border border-amber-500/30 flex items-start gap-3 animate-in fade-in slide-in-from-top-2 duration-300">
@@
</div>
)}
+
+ {transaction.policyAction === 'step_up' && (
+ <div
+ className={cn(
+ 'p-4 rounded-2xl border flex items-start gap-3 transition-all cursor-pointer select-none group',
+ isStepUpConfirmed
+ ? 'bg-amber-400/10 border-amber-400/30'
+ : 'bg-slate-800/50 border-white/5 hover:border-white/20'
+ )}
+ onClick={() => setIsStepUpConfirmed(!isStepUpConfirmed)}
+ >
+ <span className="text-[10px] text-slate-300 leading-relaxed">
+ I understand this transfer exceeds my step-up threshold and I want to continue.
+ </span>
+ </div>
+ )}
@@
- disabled={!isConfirmed}
+ disabled={!isConfirmed || (transaction.policyAction === 'step_up' && !isStepUpConfirmed)}Also applies to: 157-158
🤖 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 `@apps/extension-wallet/src/screens/Send/ReviewScreen.tsx` around lines 75 -
86, The UI shows the step-up warning but doesn't require an explicit
acknowledgment before allowing submit; add a controlled acknowledgement (e.g., a
checkbox tied to a new state like stepUpConfirmed) that is rendered when
transaction.policyAction === 'step_up' && transaction.policyMessage and require
this state in the submit gating logic (the same place that currently checks
recipient confirmation / the variable used to disable the Submit button, e.g.,
canSubmit or disabled prop) so the Submit remains disabled until stepUpConfirmed
is true for step-up transactions.
Comment on lines
+190
to
+196
| if (isNaN(daily) || daily <= 0) { | ||
| setError('Daily limit must be a positive number'); | ||
| return; | ||
| } | ||
|
|
||
| if (isNaN(stepUp) || stepUp <= 0) { | ||
| setError('Step-up threshold must be a positive number'); |
There was a problem hiding this comment.
Harden numeric validation to reject non-finite values.
isNaN alone still allows values like Infinity, which can flow into persisted limits and break policy behavior.
Suggested fix
- if (isNaN(daily) || daily <= 0) {
+ if (!Number.isFinite(daily) || daily <= 0) {
setError('Daily limit must be a positive number');
return;
}
- if (isNaN(stepUp) || stepUp <= 0) {
+ if (!Number.isFinite(stepUp) || stepUp <= 0) {
setError('Step-up threshold must be a positive number');
return;
}🤖 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 `@apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx` around lines
190 - 196, The numeric checks for daily and stepUp should reject non-finite
values (like Infinity) in addition to NaN; update the validation in
SecuritySettings (the variables daily and stepUp and the setError calls) to
coerce inputs to numbers if needed and use Number.isFinite (or !isFinite)
combined with the > 0 check (e.g., if (!Number.isFinite(daily) || daily <= 0) {
setError('Daily limit must be a positive number'); return; } and similarly for
stepUp) so infinite values are rejected before persisting.
Comment on lines
+208
to
+210
| setTimeout(() => { | ||
| onDone(); | ||
| }, 1500); |
There was a problem hiding this comment.
Clear the delayed navigation timer on unmount.
The timeout callback can still run after this view is left, causing unexpected navigation/state updates.
Suggested fix
function TransferLimitsView({ onDone }: { onDone: () => void }) {
const { policy, updateSettings } = useTransferPolicy();
const [dailyLimit, setDailyLimit] = React.useState(policy.dailyLimit.toString());
const [stepUpThreshold, setStepUpThreshold] = React.useState(policy.stepUpThreshold.toString());
const [error, setError] = React.useState('');
const [success, setSuccess] = React.useState(false);
+ const doneTimerRef = React.useRef<ReturnType<typeof setTimeout> | null>(null);
+
+ React.useEffect(() => {
+ return () => {
+ if (doneTimerRef.current) clearTimeout(doneTimerRef.current);
+ };
+ }, []);
function handleSubmit(e: React.FormEvent) {
@@
- setTimeout(() => {
+ doneTimerRef.current = setTimeout(() => {
onDone();
}, 1500);
}🤖 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 `@apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx` around lines
208 - 210, The timeout started with setTimeout(..., 1500) that calls onDone()
should be tracked and cleared on unmount to avoid delayed navigation; store the
timer id (e.g., const timeoutId = setTimeout(...)) and call
clearTimeout(timeoutId) in the component's cleanup (useEffect return or
componentWillUnmount) so SecuritySettings does not call onDone after the view is
left.
Comment on lines
+134
to
+151
| dailyTransferLimit: | ||
| typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0 | ||
| ? dailyTransferLimit | ||
| : DEFAULTS.dailyTransferLimit, | ||
| transferStepUpThreshold: | ||
| typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0 | ||
| ? transferStepUpThreshold | ||
| : DEFAULTS.transferStepUpThresholdions === 'boolean' | ||
| ? persisted.requirePasswordForSensitiveActions | ||
| : DEFAULTS.requirePasswordForSensitiveActions, | ||
| dailyTransferLimit: | ||
| typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0 | ||
| ? dailyTransferLimit | ||
| : DEFAULTS.dailyTransferLimit, | ||
| transferStepUpThreshold: | ||
| typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0 | ||
| ? transferStepUpThreshold | ||
| : DEFAULTS.transferStepUpThreshold, |
There was a problem hiding this comment.
Fix malformed transferStepUpThreshold fallback and remove duplicate hydrated keys
In apps/extension-wallet/src/stores/settings.ts (lines 134-151), transferStepUpThreshold uses non-existent DEFAULTS.transferStepUpThresholdions and falls back to boolean persisted.requirePasswordForSensitiveActions / DEFAULTS.requirePasswordForSensitiveActions for a numeric setting. The hydrated settings object also defines dailyTransferLimit and transferStepUpThreshold twice, so later keys overwrite the earlier ones.
🔧 Suggested fix
dailyTransferLimit:
typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0
? dailyTransferLimit
: DEFAULTS.dailyTransferLimit,
transferStepUpThreshold:
typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0
? transferStepUpThreshold
- : DEFAULTS.transferStepUpThresholdions === 'boolean'
- ? persisted.requirePasswordForSensitiveActions
- : DEFAULTS.requirePasswordForSensitiveActions,
- dailyTransferLimit:
- typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0
- ? dailyTransferLimit
- : DEFAULTS.dailyTransferLimit,
- transferStepUpThreshold:
- typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0
- ? transferStepUpThreshold
: DEFAULTS.transferStepUpThreshold,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dailyTransferLimit: | |
| typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0 | |
| ? dailyTransferLimit | |
| : DEFAULTS.dailyTransferLimit, | |
| transferStepUpThreshold: | |
| typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0 | |
| ? transferStepUpThreshold | |
| : DEFAULTS.transferStepUpThresholdions === 'boolean' | |
| ? persisted.requirePasswordForSensitiveActions | |
| : DEFAULTS.requirePasswordForSensitiveActions, | |
| dailyTransferLimit: | |
| typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0 | |
| ? dailyTransferLimit | |
| : DEFAULTS.dailyTransferLimit, | |
| transferStepUpThreshold: | |
| typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0 | |
| ? transferStepUpThreshold | |
| : DEFAULTS.transferStepUpThreshold, | |
| dailyTransferLimit: | |
| typeof dailyTransferLimit === 'number' && dailyTransferLimit >= 0 | |
| ? dailyTransferLimit | |
| : DEFAULTS.dailyTransferLimit, | |
| transferStepUpThreshold: | |
| typeof transferStepUpThreshold === 'number' && transferStepUpThreshold >= 0 | |
| ? transferStepUpThreshold | |
| : DEFAULTS.transferStepUpThreshold, |
🧰 Tools
🪛 Biome (2.4.15)
[error] 134-137: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
[error] 138-143: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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 `@apps/extension-wallet/src/stores/settings.ts` around lines 134 - 151, The
hydrated settings object has duplicate keys (dailyTransferLimit and
transferStepUpThreshold) and a malformed fallback for transferStepUpThreshold:
fix the typo DEFAULTS.transferStepUpThresholdions to
DEFAULTS.transferStepUpThreshold, ensure transferStepUpThreshold uses a numeric
fallback (not the boolean persisted.requirePasswordForSensitiveActions), remove
the duplicated earlier duplicate definitions so each key appears once, and keep
the existing typeof checks (e.g., typeof transferStepUpThreshold === 'number' &&
transferStepUpThreshold >= 0) so transferStepUpThreshold and dailyTransferLimit
fall back to their correct DEFAULTS values.
Comment on lines
+25
to
+31
| const [userPolicy] = useState<TransferPolicy>({ | ||
| dailyLimit: 1000, | ||
| stepUpThreshold: 250, | ||
| }); | ||
|
|
||
| const [todayTotal] = useState(500); // Placeholder for actual transaction history | ||
|
|
There was a problem hiding this comment.
Avoid hardcoded transfer policy inputs in enforcement logic.
Using fixed dailyLimit, stepUpThreshold, and todayTotal makes policy outcomes inaccurate for real users/accounts. Wire these values from actual user settings + transaction totals before relying on this flow for enforcement.
🤖 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 `@apps/web-dashboard/src/components/SendFlow.tsx` around lines 25 - 31, The
hardcoded TransferPolicy and todayTotal in SendFlow.tsx (symbols: userPolicy,
TransferPolicy, todayTotal) must be replaced with real data: fetch or receive
the user's policy (dailyLimit, stepUpThreshold) from the account settings
service or parent props/context and compute todayTotal from the user's
transaction history API or state selector before enforcing the flow; replace the
useState initializers with async loading (or useEffect) to populate these
values, add a loading/error state to avoid enforcement until real values arrive,
and ensure downstream checks reference the populated state rather than fixed
literals.
Comment on lines
+75
to
+85
| // Validate transfer policy if provided | ||
| if (request.transferPolicy) { | ||
| const { policy, amount, todayTotal } = request.transferPolicy; | ||
| const policyResult = validateTransferPolicy(amount, todayTotal, policy); | ||
| if (policyResult.action === 'block') { | ||
| return { | ||
| valid: false, | ||
| error: { code: 'TRANSFER_LIMIT_EXCEEDED', message: policyResult.message }, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Transfer-limit enforcement is bypassable because inputs are client-supplied and unsigned.
This check trusts request.transferPolicy directly. Since that object is optional and not part of the canonical signed payload, a caller can omit or modify policy/amount/todayTotal and evade server-side limits. Limit decisions should be derived server-side (authoritative policy + persisted daily totals) rather than accepted from request body.
🤖 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 `@services/relayer/src/services/relayService.ts` around lines 75 - 85, The
current check trusts request.transferPolicy (policy, amount, todayTotal) from
the client and calls validateTransferPolicy, which is bypassable; instead,
derive limits server-side: remove reliance on request.transferPolicy, fetch the
authoritative policy for the actor/account (server config/DB), compute the
requested transfer amount from the canonical signed request payload, load
persisted todayTotal from your DB/ledger, then call validateTransferPolicy with
those server-side values (use the same validateTransferPolicy function but pass
serverPolicy, computed amount, and persisted todayTotal) and return the
TRANSFER_LIMIT_EXCEEDED error if it blocks.
Comment on lines
+25
to
+30
| /** Optional transfer policy for enforcing daily limits */ | ||
| transferPolicy?: { | ||
| policy: TransferPolicy; | ||
| amount: number; | ||
| todayTotal: number; | ||
| }; |
There was a problem hiding this comment.
transferPolicy is not part of the parsed HTTP contract, so this field is dropped before validation.
RelayExecuteRequest now expects transferPolicy, but services/relayer/src/api/types.ts’s relayExecuteRequestSchema does not define it, and body middleware replaces req.body with schema output. That means this new field won’t survive request parsing on /relay/execute, so policy enforcement won’t run for HTTP callers.
Suggested patch direction
// services/relayer/src/api/types.ts
export const relayExecuteRequestSchema = z.object({
accountAddress: stellarPublicKeySchema,
to: stellarPublicKeySchema,
functionName: z.string().min(1).max(32),
args: z.array(z.string()).default([]),
nonce: nonceSchema,
callerType: z.enum(['owner', 'session_key']),
sessionPublicKey: sessionPublicKeySchema.optional(),
signature: signatureSchema.optional(),
signaturePayload: z.string().regex(/^[0-9a-fA-F]*$/).optional(),
+ transferPolicy: z
+ .object({
+ policy: transferPolicySchema,
+ amount: z.number().nonnegative(),
+ todayTotal: z.number().nonnegative(),
+ })
+ .optional(),
});🤖 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 `@services/relayer/src/types/requests.ts` around lines 25 - 30, The new
optional transferPolicy field added to the RelayExecuteRequest type is being
stripped by request validation because relayExecuteRequestSchema (used by the
body middleware) doesn't include it; update relayExecuteRequestSchema to define
transferPolicy with the same shape (policy: TransferPolicy enum/union, amount:
number, todayTotal: number) and mark it optional so parsed req.body retains the
field, and ensure related type bindings (RelayExecuteRequest) stay consistent
with the schema so HTTP callers preserve the policy for enforcement in the
execute handler.
Comment on lines
+200
to
+217
| ```typescript | ||
| if (request.transferPolicy) { | ||
| const policyResult = validateTransferPolicy( | ||
| amount, | ||
| todayTotal, | ||
| policy | ||
| ); | ||
| if (policyResult.action === 'block') { | ||
| return { | ||
| valid: false, | ||
| error: { | ||
| code: 'TRANSFER_LIMIT_EXCEEDED', | ||
| message: policyResult.message, | ||
| }, | ||
| }; | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Relayer integration example is out of sync with implemented validation API.
This snippet documents direct validateTransferPolicy(...) handling inside validateRelay(), but the implementation path uses validateTransferPolicyConstraints(...) and maps requiresStepUp/error there (see services/relayer/src/validation/transferPolicy.ts:4-43). Please update this section to match the actual service contract and avoid future drift.
🤖 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 `@TRANSFER_LIMITS_IMPLEMENTATION.md` around lines 200 - 217, The documented
snippet in validateRelay is out of sync: replace the direct call to
validateTransferPolicy(...) with the actual service API
validateTransferPolicyConstraints(...) and handle its returned shape (check for
requiresStepUp flag to trigger step-up flow or map returned error to the
TRANSFER_LIMIT_EXCEEDED response). Specifically, call
validateTransferPolicyConstraints(amount, todayTotal, policy), then if
result.requiresStepUp === true return the step-up response used by
validateRelay, otherwise if result.error present return { valid: false, error: {
code: 'TRANSFER_LIMIT_EXCEEDED', message: result.error.message } } so the
example matches the implementation that uses requiresStepUp and the error
mapping.
Comment on lines
+223
to
+264
| ``` | ||
| User enters amount: 100 XLM | ||
| ↓ | ||
| Policy validation: allow (100 < 250) | ||
| ↓ | ||
| Review screen: NO warning | ||
| ↓ | ||
| Submit transfer: Success | ||
| ``` | ||
|
|
||
| ### Scenario 2: Step-up Transfer (Above Threshold, Within Limit) | ||
|
|
||
| ``` | ||
| User enters amount: 500 XLM | ||
| ↓ | ||
| Policy validation: step_up (500 > 250, < 1000) | ||
| ↓ | ||
| Review screen: AMBER WARNING | ||
| "This transfer requires additional verification" | ||
| ↓ | ||
| User proceeds with confirmation | ||
| ↓ | ||
| Submit transfer with policy metadata | ||
| ↓ | ||
| Relayer validates policy | ||
| ↓ | ||
| Success (with step-up flag for future 2FA/MFA) | ||
| ``` | ||
|
|
||
| ### Scenario 3: Blocked Transfer (Exceeds Daily Limit) | ||
|
|
||
| ``` | ||
| User enters amount: 1500 XLM (today's total: 200) | ||
| ↓ | ||
| Policy validation: block (200 + 1500 > 1000) | ||
| ↓ | ||
| Form error: "Transfer exceeds daily limit" | ||
| ↓ | ||
| Submit button disabled | ||
| ↓ | ||
| User must reduce amount | ||
| ``` |
There was a problem hiding this comment.
Add fence languages to scenario code blocks to satisfy markdownlint.
The fenced blocks starting at Lines 223, 235, and 254 are missing a language tag (MD040). Add text for these flow diagrams to keep docs lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 235-235: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 254-254: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@TRANSFER_LIMITS_IMPLEMENTATION.md` around lines 223 - 264, The three fenced
flow diagrams under "Scenario 1: Basic Transfer", "Scenario 2: Step-up Transfer
(Above Threshold, Within Limit)", and "Scenario 3: Blocked Transfer (Exceeds
Daily Limit)" are missing markdown fence language tags; update each
triple-backtick fence that wraps the flow (the blocks beginning with "User
enters amount: 100 XLM", "User enters amount: 500 XLM", and "User enters amount:
1500 XLM (today's total: 200)") to use ```text so the blocks pass markdownlint
MD040.
Comment on lines
+324
to
+326
| - Minimum values: 1 XLM | ||
| - Maximum values: Limited only by balance | ||
| - Constraint: stepUpThreshold ≤ dailyLimit |
There was a problem hiding this comment.
Minimum-value constraint conflicts with the shared schema.
Line 324 says minimum is 1 XLM, but the shared TransferPolicySchema uses .nonnegative() for both fields (>= 0). The docs should reflect the true accepted range or explicitly document stricter UI-only validation if intentional.
🤖 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 `@TRANSFER_LIMITS_IMPLEMENTATION.md` around lines 324 - 326, The docs currently
state a 1 XLM minimum but the shared TransferPolicySchema uses .nonnegative()
(>=0); update either the schema or the documentation so they match: either
change TransferPolicySchema to enforce a 1 XLM minimum (e.g., replace
.nonnegative() with .min(1) on the fields representing minimum transfer amounts
such as stepUpThreshold and dailyLimit) or clarify in
TRANSFER_LIMITS_IMPLEMENTATION.md that the 1 XLM constraint is UI-only and not
enforced by TransferPolicySchema; ensure you reference the schema symbol
TransferPolicySchema and the policy fields stepUpThreshold and dailyLimit when
making the change.
Contributor
|
@samjay8 resolve conflicts |
Merge origin/main and combine daily transfer limit policy checks with scheduled transfers, active sessions, and send flow updates.
5 tasks
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
2 participants
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.
Closes #413
🔒 Security: Add Daily Transfer Limits and Step-Up Verification Thresholds
Introduces configurable daily transfer limits and step-up verification prompts for large-value transfers, providing tiered risk controls to reduce fraud and user mistakes.
Changes
packages/types— Transfer Policy Modelssettings.ts— added 10 lines for daily limit and step-up threshold settingsapps/extension-wallet— Send Flow EnforcementuseTransferPolicy.ts(new) — hook for accessing and managing transfer policy stateuseSendTransaction.ts— integrated policy validation intovalidateForm()andgoToReview()methods; transfers above threshold blocked or flagged before proceedingReviewScreen.tsx— added step-up confirmation section shown when policy action isstep_up; users must explicitly confirm large-value transfers before proceedingapps/web-dashboard— Policy EnforcementRelayer Path — Policy Validation
transferPolicy.ts(new) — server-side validation service that checks transfers against configured daily limitsTRANSFER_LIMIT_EXCEEDEDerror code to relayer error typesTests
useTransferPolicy.test.ts(new) — unit tests covering policy edge cases: below threshold, at threshold, above threshold, daily limit reset, step-up confirmation flowAcceptance Criteria
✅ User can configure daily limit settings
✅ Transfers above threshold require explicit step-up confirmation
✅ Exceeded daily limits blocked with clear UI messaging
✅ Unit tests cover policy edge cases
Summary by CodeRabbit
New Features
Documentation
Tests