-
Notifications
You must be signed in to change notification settings - Fork 6
feat(multi-collateral): impl apply interests #186
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: dev
Are you sure you want to change the base?
Conversation
9af18d0 to
5093262
Compare
MohammadNassar1
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.
@MohammadNassar1 resolved 2 discussions.
Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on @MohammadNassar1).
workspace/apps/perpetuals/contracts/src/core/components/positions/positions.cairo
Show resolved
Hide resolved
MohammadNassar1
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.
@MohammadNassar1 resolved 1 discussion.
Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on @MohammadNassar1).
5093262 to
53e84f1
Compare
53e84f1 to
f52ba0f
Compare
f52ba0f to
4fa0b69
Compare
|
I think that we need to add a time |
|
could you add an example |
|
full name please |
|
we need the funding here also |
|
I think the scale its a little bit too small with these numbers |
RoeeGross
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.
@RoeeGross reviewed 9 files and all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ishay-starkware, @MohammadNassar1, and @omri-ba-starkware).
4fa0b69 to
8d8ff00
Compare
MohammadNassar1
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.
@MohammadNassar1 made 6 comments.
Reviewable status: 7 of 9 files reviewed, 6 unresolved discussions (waiting on @ishay-starkware, @omri-ba-starkware, and @RoeeGross).
workspace/apps/perpetuals/contracts/src/core/core.cairo line 167 at r3 (raw file):
Previously, RoeeGross wrote…
could you add an example
in the doc
Done.
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1160 at r3 (raw file):
Previously, RoeeGross wrote…
we need the funding here also
Can you please explain?
I think funding is only needed if we wanna validate healthy/healthier.
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1161 at r3 (raw file):
Previously, RoeeGross wrote…
full name please
Done.
workspace/apps/perpetuals/contracts/src/core/interface.cairo line 173 at r3 (raw file):
Previously, RoeeGross wrote…
I think that we need to add a time
Will add it once your PR (that adds system time) is merged.
workspace/apps/perpetuals/contracts/src/tests/constants.cairo line 84 at r3 (raw file):
Previously, RoeeGross wrote…
I think the scale its a little bit too small with these numbers
Done
610cfb1 to
48d929b
Compare
48d929b to
f0bf228
Compare
MohammadNassar1
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.
@MohammadNassar1 made 1 comment.
Reviewable status: 3 of 10 files reviewed, 6 unresolved discussions (waiting on @ishay-starkware, @omri-ba-starkware, and @RoeeGross).
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1160 at r3 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Can you please explain?
I think funding is only needed if we wanna validate healthy/healthier.
added
f0bf228 to
b42bb6f
Compare
b42bb6f to
0828fd7
Compare
0828fd7 to
e9eabbf
Compare
e9eabbf to
6817b8a
Compare
omri-ba-starkware
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.
Just a general Q: I don't understand why we derive interest amount based on PnL (which is base collateral + synthetics)? The interests are supposed to fund loans of spot assets from external market makers (like btc eth...)- so it makes sense to look at the "PnL" of the spot assets exclusively no? Why should a user that only puts USDC as collateral and trades only with synthetics, profit or lose due to loans of assets completely unrelated to that user?
@omri-ba-starkware reviewed 7 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ishay-starkware, @MohammadNassar1, @remollemo, and @RoeeGross).
workspace/apps/perpetuals/contracts/src/core/core.cairo line 171 at r7 (raw file):
// Maximum interest rate per second (32-bit fixed-point with 32-bit fractional part). // Example: max_interest_rate_per_sec = 1000000 means the rate is 1000000 / 2^32 ≈ // 0.000232 per second, which is approximately 7.3% per year.
I am not sure how you got this number- did you do 0.000232 * 3600 * 24 * 365? I am guessing no so can you explain this calculation?
Code quote:
7.3% per year.workspace/apps/perpetuals/contracts/src/core/core.cairo line 1270 at r7 (raw file):
// Validate interest rate if pnl.is_non_zero() && previous_timestamp.is_non_zero() {
Can this parameter be zero? I see that it is initialised to current time when position is created and is updated every time we apply interest. If position does not exist it won't reach this line and will panic on the first line of this fn.
Code quote:
&& previous_timestamp.is_non_zero()workspace/apps/perpetuals/contracts/src/core/core.cairo line 1281 at r7 (raw file):
max_interest_rate_per_sec.into(), interest_rate_scale.into(), )
This might be a little out of scope but I will ask anyway. Interest is usually modelled with exponents and not linearly like we do here. Exponents make more sense because as your Pnl balance grows (or shrinks) the interest amount should also grow (shrink) because interest rate is given in percentages with regards to the current PnL balance. Calculating interest like we do here does not capture this effect (you can also think of it in a dynamic perspective: a function that is proportional to it's change rate --> must be an exponent). I would calculate max_allowed_change something like this:
max_pnl_after_interest = pnl_before_interest * exp(delta_time * max_interest_rate)
max_allowed_change = abs(max_pnl_after_interest - pnl_before_interest)
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1292 at r7 (raw file):
// If old balance is zero, only allow zero interest. // If `previous_timestamp` is zero, this indicates the first interest calculation, // and the interest amount is required to be zero.
I am not sure how you can find out if this is actually the first time we apply interest to a position. If we init to 0 than the code makes more sense.
Code quote:
// If `previous_timestamp` is zero, this indicates the first interest calculation,
// and the interest amount is required to be zero.6817b8a to
febe6f1
Compare
febe6f1 to
5aea263
Compare
MohammadNassar1
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.
Good question!
The system loan is in usdc, so any position with a positive USDC balance reduces system load, thus such positions would get positive interests, while position in debt (negative usdc amount) pay interest.
The effective usdc balance of a position is the amount of usdc plus the total value of the synthetic assets it holds.
@MohammadNassar1 reviewed 1 file and made 6 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ishay-starkware, @omri-ba-starkware, @remollemo, and @RoeeGross).
workspace/apps/perpetuals/contracts/src/core/core.cairo line 171 at r7 (raw file):
Previously, omri-ba-starkware wrote…
I am not sure how you got this number- did you do 0.000232 * 3600 * 24 * 365? I am guessing no so can you explain this calculation?
The calculation is in the formula below.
You're correct, the example is wrong, fixed it.
Code snippet:
max_interest_rate_per_sec * 3600 * 24 * 365 / 2**32workspace/apps/perpetuals/contracts/src/core/core.cairo line 1270 at r7 (raw file):
Previously, omri-ba-starkware wrote…
Can this parameter be zero? I see that it is initialised to current time when position is created and is updated every time we apply interest. If position does not exist it won't reach this line and will panic on the first line of this fn.
It can be zero in the first appy interest for "old positions" that was generated before this change.
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1281 at r7 (raw file):
Previously, omri-ba-starkware wrote…
This might be a little out of scope but I will ask anyway. Interest is usually modelled with exponents and not linearly like we do here. Exponents make more sense because as your Pnl balance grows (or shrinks) the interest amount should also grow (shrink) because interest rate is given in percentages with regards to the current PnL balance. Calculating interest like we do here does not capture this effect (you can also think of it in a dynamic perspective: a function that is proportional to it's change rate --> must be an exponent). I would calculate max_allowed_change something like this:
max_pnl_after_interest = pnl_before_interest * exp(delta_time * max_interest_rate)
max_allowed_change = abs(max_pnl_after_interest - pnl_before_interest)
Hmm, note that we do not want to calculate the interest, the max allowed change is a limit on the interest.
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1292 at r7 (raw file):
Previously, omri-ba-starkware wrote…
I am not sure how you can find out if this is actually the first time we apply interest to a position. If we init to 0 than the code makes more sense.
See the prev comment, for old positions that were created before this change, their timestamp would be zero.
workspace/apps/perpetuals/contracts/src/core/interface.cairo line 173 at r3 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Will add it once your PR (that adds system time) is merged.
a
MohammadNassar1
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.
@MohammadNassar1 resolved 1 discussion.
Reviewable status: 9 of 10 files reviewed, 9 unresolved discussions (waiting on @ishay-starkware, @omri-ba-starkware, @remollemo, and @RoeeGross).
omri-ba-starkware
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.
OK this makes sense! A follow up: We only loan USDC? We never loan any other assets?
@omri-ba-starkware reviewed 1 file and all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ishay-starkware, @MohammadNassar1, @remollemo, and @RoeeGross).
workspace/apps/perpetuals/contracts/src/core/core.cairo line 1281 at r7 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Hmm, note that we do not want to calculate the interest, the max allowed change is a limit on the interest.
I see what you mean, I still have follow up questions but I think I will take it face to face tomorrow.
MohammadNassar1
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.
That's correct.
At least for the current version, we only loan usdc.
@MohammadNassar1 made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ishay-starkware, @remollemo, and @RoeeGross).
5aea263 to
0a1acab
Compare
Note
Introduces operator-triggered interest application with rate limits and supporting accounting changes.
Adds
apply_interestsentrypoint inCoreto batch-apply per-position interest, validated againstmax_interest_rate_per_sec(Q32) and time sincelast_interest_applied_timeStores
max_interest_rate_per_secinCorestorage and enforces non-zero in constructor; plumbs config through tests/deploy pathsExtends
Positionwithlast_interest_applied_time; set onnew_positionAdds
_calculate_position_pnlinCoreandcalculate_position_pnlinvalue_risk_calculatorto compute PnL over synthetic assets + base collateral (with funding)New errors:
INVALID_INTEREST_RATE,ZERO_MAX_INTEREST_RATE; usesmul_wide_and_floor_divfor safe bound checksMinor: import updates and utilities (
Pow,Time, storage helpers)Written by Cursor Bugbot for commit 0a1acab. This will update automatically on new commits. Configure here.
This change is