fix(velocity): make balance limit start field mandatory#714
Conversation
Omitting start on NewBalanceLimit previously defaulted to created_at (wall-clock attachment time), silently skipping enforcement for any transaction with an effective time before that. This caused an overdraft bypass in staging when backdated transactions skipped velocity limits. Make start a required field on the builder so omitting it is a compile error. The else branch in attach_control_in_op now falls back to MIN_UTC for backwards compatibility with already-persisted limits that have no start expression. Co-Authored-By: Claude Opus 4.6 <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2b9e7c7. Configure here.
| pub amount: CelExpression, | ||
| pub enforcement_direction: CelExpression, | ||
| pub start: Option<CelExpression>, | ||
| pub start: CelExpression, |
There was a problem hiding this comment.
Breaking deserialization of existing persisted events with null start
High Severity
Changing start from Option<CelExpression> to CelExpression on the BalanceLimit struct breaks deserialization of existing events stored in the database. Since VelocityLimitEvent is event-sourced and persisted as JSON in cala_velocity_limit_events, any previously-created velocity limit where start was None will have "start": null in its stored JSON. Attempting to load such a record will now fail because CelExpression (backed by #[serde(try_from = "String")]) cannot deserialize from null. No data migration or #[serde(default)] fallback is provided.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2b9e7c7. Configure here.
There was a problem hiding this comment.
We aren't maintaining backward compatibility for this as yet
📊 Performance ReportCommit: 0270ef1 Cala Performance Benchmark Results (non-representative)Criterion Benchmark Results (single-threaded)
Load Testing Results (parallel-execution)
Note: Performance results may vary based on system resources and database state. Last updated by commit 0270ef1 |
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…imitBuilder Adds a discoverable builder method as an alternative to manually specifying the epoch timestamp for balance limits that should apply to all transactions regardless of effective time. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
2 participants
Summary
When
startwas omitted on aBalanceLimit, cala silently defaulted tocreated_at(wall-clock attachment time). Any limit wherestart > transaction_effective_timewas skipped during enforcement — velocity limits were ineffective for backdated transactions. This caused an overdraft bypass in staging (lana-bank#5380): simulation posted payments with 2023 effective times against a limit whosestartwas wall-clock 2026, driving a deposit to -$52,313.lana-bank#5380 shipped an immediate fix (explicit
.start()on all call sites). This PR fixes it at the source:startis now mandatory onBalanceLimit, so callers must consciously choose a start time rather than silently inheriting a potentially wrong default. Beyond preventing the original bug, this optimizes for clarity and reduces ambiguity — implicit behavior that could quietly produce incorrect enforcement is replaced with an explicit, reviewer-visible decision at every call site.Tradeoff analysis (change default vs. make mandatory):
ops/scratch/active/cala-velocity-start-default.md.Test plan
.start())🤖 Generated with Claude Code
Note
Medium Risk
This is a breaking schema/type change and alters limit activation behavior, which can affect enforcement for existing integrations that omitted
start. The change is localized and makes enforcement more explicit, reducing the risk of silent bypasses.Overview
BalanceLimit.startis now mandatory across Rust types and the emitted JSON schema, turning a previously optional field into a requiredCelExpression.Limit attachment/enforcement no longer falls back to
created_atwhenstartis omitted; the start time must be explicitly evaluated from the provided expression. Builders and internal conversions were updated accordingly, including a newNewBalanceLimitBuilder::always_active()helper, and existing tests/bench scaffolding were updated to set an explicit start.Reviewed by Cursor Bugbot for commit 0270ef1. Bugbot is set up for automated code reviews on this repo. Configure here.