-
Notifications
You must be signed in to change notification settings - Fork 2
Deposit rate and capacity implementation #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
turbolent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Co-authored-by: Bastian Müller <[email protected]>
| access(EImplementation) fun regenerateDepositCapacity() { | ||
| let currentTime = getCurrentBlock().timestamp | ||
| let dt = currentTime - self.lastDepositCapacityUpdate | ||
| let hourInSeconds = 3600.0 |
There was a problem hiding this comment.
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
Unbounded Capacity GrowthLocation: The Example: With
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 ...
}
} |
Clarification: Per-Position vs Per-Account LimitLocation: The documentation states (deposit_capacity_mechanism.md, line 11):
And line 83:
However, the implementation tracks limits by position ID ( let currentUsage = tokenState.depositUsage[pid] ?? 0.0Since 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? |
User Usage Never Decreases on WithdrawalLocation: 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:
Is this intentional? If not, suggested fix in // 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 |
Async Update Uses Only Per-Deposit LimitLocation: The async processor uses Example: If per-deposit = 1000 but user has only 100 remaining of their per-user limit, the current code attempts 1000, then Suggested fix (optional): cap 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 |
Documentation/Code Mismatch - depositRate SemanticsLocation: Documentation says: Actual behavior: let multiplier = dt / hourInSeconds // Can be 2.5 if 2.5 hours passed
let newDepositCapacityCap = self.depositRate * multiplier + self.depositCapacityCapThe rate is continuous (multiplied by fractional hours), not discrete. If 2.5 hours pass, cap increases by Suggested fix: Update documentation to reflect continuous behavior, or clarify the code comment on line 632. |
Unused Entitlement DeclarationLocation: access(all) entitlement ETokenStateViewThis 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 |
Silent Capacity Clamping in consumeDepositCapacityLocation: 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
} |
No Events for Deposit Capacity State ChangesLocation: The following state changes occur without emitting events:
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
) |
Rate Change Applies RetroactivelyLocation: Updating Example (initial rate = 1000, cap = 100,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
} |
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. |
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. |
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 |
is this not the desired behaviour? cc @Gornutz My impression is that this queue was exactly for retrying the value above the limit. |
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 |
fair, i've updated to an assert |
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? |
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. |
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. |
|
|
||
| // Time-based state is handled by the tokenState() helper function | ||
| // Deposit rate limiting: prevent a single large deposit from monopolizing capacity. |
There was a problem hiding this comment.
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.
|
LGTM |
|
fixes: #83 |
jordanschalm
left a comment
There was a problem hiding this 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) |
There was a problem hiding this comment.
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%) |
There was a problem hiding this comment.
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.
| // If current capacity exceeds the new cap, clamp it to the cap | ||
| if self.depositCapacity > cap { | ||
| self.depositCapacity = cap | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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 |
There was a problem hiding this comment.
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.
| /// Sets the deposit rate for this token state after settling the old rate | ||
| access(EImplementation) fun setDepositRate(_ rate: UFix64) { |
There was a problem hiding this comment.
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%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// 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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
Description
Deposit capacity tracking and implementation. If we're aligned, then I will build out some tests.
For contributor use:
masterbranchFiles changedin the Github PR explorer