Skip to content

Conversation

@Kay-Zee
Copy link
Member

@Kay-Zee Kay-Zee commented Dec 1, 2025

Description

Deposit capacity tracking and implementation. If we're aligned, then I will build out some tests.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

❌ Patch coverage is 48.14815% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cadence/contracts/FlowALP.cdc 48.14% 14 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Kay-Zee Kay-Zee changed the base branch from main to kan/math-related-concerns December 1, 2025 04:51
Base automatically changed from kan/math-related-concerns to main December 1, 2025 22:54
@Kay-Zee Kay-Zee marked this pull request as ready for review December 10, 2025 23:25
@Kay-Zee Kay-Zee requested a review from a team as a code owner December 10, 2025 23:25
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Kay-Zee and others added 2 commits January 6, 2026 11:21
Co-authored-by: Bastian Müller <bastian@turbolent.com>
liobrasil

This comment was marked as outdated.

access(EImplementation) fun regenerateDepositCapacity() {
let currentTime = getCurrentBlock().timestamp
let dt = currentTime - self.lastDepositCapacityUpdate
let hourInSeconds = 3600.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic number should be a contract-level constant or a configurable parameter

@liobrasil
Copy link
Contributor

Unbounded Capacity Growth

Location: TokenState.regenerateDepositCapacity() (lines 815-835)

The depositCapacityCap grows indefinitely with no upper bound. If a token has no activity for extended periods, capacity can grow extremely large, defeating rate limiting.

Example: With depositRate = 1000 and initial cap of 10,000:

  • After 1 month of inactivity: cap grows to ~730,000
  • After 1 year: cap grows to ~8.77M

Suggested fix - add a maximum bound:

access(EImplementation) var maxDepositCapacityCap: UFix64  // New field

access(EImplementation) fun regenerateDepositCapacity() {
    // ... existing dt calculation ...
    if dt >= hourInSeconds {
        let multiplier = dt / hourInSeconds
        let uncappedNewCap = self.depositRate * multiplier + self.depositCapacityCap
        let newDepositCapacityCap = uncappedNewCap < self.maxDepositCapacityCap 
            ? uncappedNewCap 
            : self.maxDepositCapacityCap
        // ... rest unchanged ...
    }
}

@liobrasil
Copy link
Contributor

Clarification: Per-Position vs Per-Account Limit

Location: depositUsage tracking in TokenState

The documentation states (deposit_capacity_mechanism.md, line 11):

"Enforce per-user deposit limits to prevent single-user monopolization"

And line 83:

"Each user (position) has a limit..."

However, the implementation tracks limits by position ID (pid), not by user address:

let currentUsage = tokenState.depositUsage[pid] ?? 0.0

Since a single user can create multiple positions, is this the intended behavior? If the goal is to prevent "single-user monopolization," should limits be tracked per-address instead of per-position?

@liobrasil
Copy link
Contributor

User Usage Never Decreases on Withdrawal

Location: depositUsage tracking

When users deposit, their usage is incremented, but withdrawals don't decrement it. A user who deposits and withdraws the same amount remains at their "limit" until hourly regeneration resets usage.

Example:

  1. User deposits 500 (hits limit)
  2. User withdraws 500
  3. User tries to deposit 100 → queued (still "at limit")

Is this intentional? If not, suggested fix in recordWithdrawal:

// In TokenState, add:
access(EImplementation) fun decreaseDepositUsage(_ amount: UFix64, pid: UInt64) {
    let currentUsage = self.depositUsage[pid] ?? 0.0
    if amount >= currentUsage {
        self.depositUsage.remove(key: pid)
    } else {
        self.depositUsage[pid] = currentUsage - amount
    }
}

// Call from withdrawal flow when direction is Credit

@liobrasil
Copy link
Contributor

liobrasil commented Jan 7, 2026

Async Update Uses Only Per-Deposit Limit

Location: asyncUpdatePosition()

The async processor uses depositLimit() only. The per-user limit is still enforced inside depositAndPush, so this isn'''t a correctness issue, but it can cause repeated dequeue/re-queue and extra churn when a position is already at its per-user cap.

Example: If per-deposit = 1000 but user has only 100 remaining of their per-user limit, the current code attempts 1000, then depositAndPush re-queues 900. This repeats each cycle until the hourly reset.

Suggested fix (optional): cap maxDeposit by the remaining per-user limit before withdrawing from the queued vault.

let perDepositLimit = depositTokenState.depositLimit()
let userLimitCap = depositTokenState.getUserDepositLimitCap()
let currentUsage = depositTokenState.depositUsage[pid] ?? 0.0
let remainingUserLimit = userLimitCap > currentUsage ? userLimitCap - currentUsage : 0.0
let maxDeposit = perDepositLimit < remainingUserLimit ? perDepositLimit : remainingUserLimit

@liobrasil
Copy link
Contributor

Documentation/Code Mismatch - depositRate Semantics

Location: docs/deposit_capacity_mechanism.md lines 21-24 vs FlowCreditMarket.cdc line 632

Documentation says: depositRate is a flat amount added once per hour.

Actual behavior:

let multiplier = dt / hourInSeconds  // Can be 2.5 if 2.5 hours passed
let newDepositCapacityCap = self.depositRate * multiplier + self.depositCapacityCap

The rate is continuous (multiplied by fractional hours), not discrete. If 2.5 hours pass, cap increases by depositRate * 2.5.

Suggested fix: Update documentation to reflect continuous behavior, or clarify the code comment on line 632.

@liobrasil
Copy link
Contributor

Unused Entitlement Declaration

Location: FlowCreditMarket.cdc line 117

access(all) entitlement ETokenStateView

This entitlement is declared but never used anywhere in the codebase.

Suggested fix: Either remove it, or implement its intended use (likely for read-only access to TokenState fields without requiring EImplementation).

@liobrasil
Copy link
Contributor

Silent Capacity Clamping in consumeDepositCapacity

Location: TokenState.consumeDepositCapacity() (lines 702-708)

if amount > self.depositCapacity {
    // Safety check: this shouldn't happen if depositLimit() is working correctly
    self.depositCapacity = 0.0
} else {
    self.depositCapacity = self.depositCapacity - amount
}

The comment states "this shouldn't happen" - indicating an invariant violation. However, the code silently clamps to 0 instead of alerting.

Suggested fix: Either panic (fail-fast on invariant violation) or emit a warning event for off-chain monitoring:

if amount > self.depositCapacity {
    // This indicates a bug in depositLimit() logic
    emit DepositCapacityInvariantViolation(expected: self.depositCapacity, actual: amount)
    self.depositCapacity = 0.0
} else {
    self.depositCapacity = self.depositCapacity - amount
}

@liobrasil
Copy link
Contributor

No Events for Deposit Capacity State Changes

Location: TokenState capacity-related functions

The following state changes occur without emitting events:

  • Capacity regeneration in regenerateDepositCapacity()
  • User usage tracking in consumeDepositCapacity()
  • Capacity consumption on deposits

This makes off-chain monitoring and debugging difficult.

Suggested fix: Add events for key state changes:

access(all) event DepositCapacityRegenerated(
    tokenType: Type,
    oldCap: UFix64,
    newCap: UFix64,
    newCapacity: UFix64
)

access(all) event DepositCapacityConsumed(
    tokenType: Type,
    pid: UInt64,
    amount: UFix64,
    remainingCapacity: UFix64
)

@liobrasil
Copy link
Contributor

liobrasil commented Jan 7, 2026

Rate Change Applies Retroactively

Location: TokenState.setDepositRate / Pool.setDepositRate

Updating depositRate doesn’t reset lastDepositCapacityUpdate. The next regeneration uses the new rate across the full elapsed dt since the old timestamp, effectively applying the new rate to past time and causing a sudden cap jump.

Example (initial rate = 1000, cap = 100,000):

Time Action Result
T=0 Initial cap = 100,000
T=5h Set rate = 5000 (no regeneration yet)
T=6h First deposit cap = 100,000 + 5000×6 = 130,000

Expected: 100,000 + (1000×5) + (5000×1) = 110,000

Suggested fix: settle under the old rate, then reset the timestamp when changing the rate.

access(EImplementation) fun setDepositRate(_ rate: UFix64) {
    self.regenerateDepositCapacity() // settle using old rate
    self.depositRate = rate
    self.lastDepositCapacityUpdate = getCurrentBlock().timestamp
}

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

Unbounded Capacity Growth

This is not a implementation issue, but a design issue, if you have suggestions there, let's talk it over with @Gornutz, but this is implemented as speced.

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

Clarification: Per-Position vs Per-Account Limit

Location: depositUsage tracking in TokenState

The documentation states (deposit_capacity_mechanism.md, line 11):

"Enforce per-user deposit limits to prevent single-user monopolization"

And line 83:

"Each user (position) has a limit..."

However, the implementation tracks limits by position ID (pid), not by user address:

let currentUsage = tokenState.depositUsage[pid] ?? 0.0

Since a single user can create multiple positions, is this the intended behavior? If the goal is to prevent "single-user monopolization," should limits be tracked per-address instead of per-position?

This has already been discussed, as per Alex's comments. There is no real advantage in per user vs per position long term. The real catch all will just be the total deposit limit. @Gornutz is fine with a per position limit.

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

User Usage Never Decreases on Withdrawal

Location: depositUsage tracking

When users deposit, their usage is incremented, but withdrawals don't decrement it. A user who deposits and withdraws the same amount remains at their "limit" until hourly regeneration resets usage.

Example:

  1. User deposits 500 (hits limit)
  2. User withdraws 500
  3. User tries to deposit 100 → queued (still "at limit")

Is this intentional? If not, suggested fix in recordWithdrawal:

// In TokenState, add:
access(EImplementation) fun decreaseDepositUsage(_ amount: UFix64, pid: UInt64) {
    let currentUsage = self.depositUsage[pid] ?? 0.0
    if amount >= currentUsage {
        self.depositUsage.remove(key: pid)
    } else {
        self.depositUsage[pid] = currentUsage - amount
    }
}

// Call from withdrawal flow when direction is Credit

It's a deposit limit, not an account size limit. So this controls amount of deposit. I'm not aware of the desired interaction with withdrawl, is this a well know behaviour of deposit limits? cc @Gornutz

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

Async Update Uses Only Per-Deposit Limit

Location: asyncUpdatePosition()

The async processor uses depositLimit() only. The per-user limit is still enforced inside depositAndPush, so this isn'''t a correctness issue, but it can cause repeated dequeue/re-queue and extra churn when a position is already at its per-user cap.

Example: If per-deposit = 1000 but user has only 100 remaining of their per-user limit, the current code attempts 1000, then depositAndPush re-queues 900. This repeats each cycle until the hourly reset.

Suggested fix (optional): cap maxDeposit by the remaining per-user limit before withdrawing from the queued vault.

let perDepositLimit = depositTokenState.depositLimit()
let userLimitCap = depositTokenState.getUserDepositLimitCap()
let currentUsage = depositTokenState.depositUsage[pid] ?? 0.0
let remainingUserLimit = userLimitCap > currentUsage ? userLimitCap - currentUsage : 0.0
let maxDeposit = perDepositLimit < remainingUserLimit ? perDepositLimit : remainingUserLimit

is this not the desired behaviour? cc @Gornutz My impression is that this queue was exactly for retrying the value above the limit.

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

deposit_capacity_mechanism

Ah, yes, @Gornutz had suggested i change to an "exact" amount, via a multiplier rather than a flat amount, but i never updated the docs, will update

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

ETokenStateView

fair, i've updated to an assert

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Jan 7, 2026

Rate Change Applies Retroactively

Location: TokenState.setDepositRate / Pool.setDepositRate

Updating depositRate doesn’t reset lastDepositCapacityUpdate. The next regeneration uses the new rate across the full elapsed dt since the old timestamp, effectively applying the new rate to past time and causing a sudden cap jump.

Example (initial rate = 1000, cap = 100,000):

Time Action Result
T=0 Initial cap = 100,000
T=5h Set rate = 5000 (no regeneration yet)
T=6h First deposit cap = 100,000 + 5000×6 = 130,000
Expected: 100,000 + (1000×5) + (5000×1) = 110,000

Suggested fix: settle under the old rate, then reset the timestamp when changing the rate.

access(EImplementation) fun setDepositRate(_ rate: UFix64) {
    self.regenerateDepositCapacity() // settle using old rate
    self.depositRate = rate
    self.lastDepositCapacityUpdate = getCurrentBlock().timestamp
}

I've gone ahead and made this change, but @Gornutz could we get some guidance on if we feel like it's okay to have the "regeneration" timing move around based on when we set the rate?

@Gornutz
Copy link

Gornutz commented Jan 7, 2026

User Usage Never Decreases on Withdrawal

Location: depositUsage tracking

It's a deposit limit, not an account size limit. So this controls amount of deposit. I'm not aware of the desired interaction with withdrawl, is this a well know behaviour of deposit limits? cc @Gornutz

We shouldn't worry about them withdrawing for the deposit limits to increase back the capacity; since in reality if a person is withdrawing their funds it would be creating a brand new position anyway.

@Gornutz
Copy link

Gornutz commented Jan 7, 2026

Rate Change Applies Retroactively

Location: TokenState.setDepositRate / Pool.setDepositRate

I've gone ahead and made this change, but @Gornutz could we get some guidance on if we feel like it's okay to have the "regeneration" timing move around based on when we set the rate?

It's okay for this timing to move around, since in reality we don't need to hyper optimize for the specific regeneration event as we have enough other levers to allow use to get to a desired outcome.

@vishalchangrani vishalchangrani added QS FCM Audit First Round Task to address QuantStamp Audit comments from their first round of review of the FCM contracts. and removed QS FCM Audit First Round Task to address QuantStamp Audit comments from their first round of review of the FCM contracts. labels Jan 7, 2026

// Time-based state is handled by the tokenState() helper function
// Deposit rate limiting: prevent a single large deposit from monopolizing capacity.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These limits run before we know if the deposit is actually a debt repayment. Because recordDeposit handles Debit balances too, this path throttles repayments/top‑ups the same as new collateral. That means a borrower can be blocked from repaying (queued excess) and become liquidatable even when they have funds. Consider exempting the repayment portion (current debit) from capacity limits, or add a dedicated repay/top‑up path that bypasses these checks and only caps net new credit.

@liobrasil
Copy link
Contributor

LGTM

@vishalchangrani
Copy link

fixes: #83

Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only reviewed the contract changes to FlowCreditMarket

tsRef.setDepositLimitFraction(fraction)
}

/// Updates the deposit rate for a given token (rate per second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we state a per second rate, but depositRate field documentation (and usage) assumes per-hour rate

access(all) var insuranceRate: UFix64
access(EImplementation) var insuranceRate: UFix64

/// Per-deposit limit fraction of capacity (default 0.05 i.e., 5%)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how useful this is. What is preventing one user from multiplexing a large deposit over many positions?

If there isn't anything stopping a user from bypassing this check, I would argue for removing it.

Comment on lines +707 to +710
// If current capacity exceeds the new cap, clamp it to the cap
if self.depositCapacity > cap {
self.depositCapacity = cap
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If current capacity exceeds the new cap, clamp it to the cap
if self.depositCapacity > cap {
self.depositCapacity = cap
}

Given line 706 above, I don't think this conditional can ever be true

}

/// Updates the deposit rate for a given token (rate per second)
access(EGovernance) fun setDepositRate(tokenType: Type, rate: UFix64) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
access(EGovernance) fun setDepositRate(tokenType: Type, rate: UFix64) {
access(EGovernance) fun setDepositRate(tokenType: Type, hourlyRate: UFix64) {

// Set the deposit capacity to the new deposit capacity cap, i.e. regenerate the capacity
self.setDepositCapacity(newDepositCapacityCap)

// If capacity cap increased (regenerated), reset all user usage for this token type
Copy link
Member

@jordanschalm jordanschalm Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only reset per-user usage if capacity cap increased?

My understanding from the regeneration implementation is that the deposit capacity acts as a limit to the flow of deposits (how much I can deposit per unit time) rather than the stock of deposits (how much can I deposit total). If the deposit rate is arbitrarily close to zero (ie. deposit capacity grows arbitrarily slowly), this is how it works. But once the deposit rate is actually zero, the behaviour changes substantially and imposes a hard limit forever.

This isn't bad necessarily, but it's not obvious to me why there is this change in behaviour from hourly limits to forever limits. It would be useful to document this behaviour and why we are choosing it somewhere, if it is desired.

Comment on lines +697 to +698
/// Sets the deposit rate for this token state after settling the old rate
access(EImplementation) fun setDepositRate(_ rate: UFix64) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would document here that this is an absolute hourly increase, not a percentage rate.

access(all) var insuranceRate: UFix64
access(EImplementation) var insuranceRate: UFix64

/// Per-deposit limit fraction of capacity (default 0.05 i.e., 5%)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Per-deposit limit fraction of capacity (default 0.05 i.e., 5%)
/// Per-position limit fraction of capacity (default 0.05 i.e., 5%)

I think per-position is more precise. A position can accept many deposits.

access(all) var depositLimitFraction: UFix64
access(EImplementation) var depositLimitFraction: UFix64

/// The rate at which depositCapacity can increase over time. This is per hour. and should be applied to the depositCapacityCap once an hour.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would document here that this is an absolute hourly increase, not a percentage rate.

if dt >= hourInSeconds { // 1 hour
let multiplier = dt / hourInSeconds
let oldCap = self.depositCapacityCap
let newDepositCapacityCap = self.depositRate * multiplier + self.depositCapacityCap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a long time while reviewing this PR I assumed depositRate was a percentage rate of the existing depositCapacityCap (should have read the docs first!). I think it is safe to assume that will be the default interpretation of the term rate in this context, so would suggest documenting very clearly in the contract code that this rate is denominated in units of the deposit token.

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.

8 participants