Skip to content
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

VAL-69 Constant-time, per-user "crank" #88

Merged
merged 24 commits into from
Nov 22, 2022
Merged

VAL-69 Constant-time, per-user "crank" #88

merged 24 commits into from
Nov 22, 2022

Conversation

ams9198
Copy link
Contributor

@ams9198 ams9198 commented Nov 14, 2022

Notes

For consideration / notes:

  • If this approach seems like the right path, I think we should invest the time to make sure that we all understand (and agree with) the formula described below before merging so it's not a black box (took me awhile to get comfortable with it).
  • One thing to look out for is identifying any edge cases that our tests might not cover, and I can add them to this PR before we land it
  • Since rounding was effected, and our tests mostly use not realistic, small values, the diff in the tests were littered with small adjustments (though mostly down).

Next step

I'm looking for feedback on this approach at a high-level, and if we're in consensus, then on the specifics of the PR. If we don't feel comfortable with this approach, I can revert back to the per-user for-loop approach in this branch: https://github.com/ams9198/valyria-core/compare/master...VAL-69-rebased

Formula

The formula used here is:

lenderEligibleBalance *( r1 + r2*(1 - r1) + r3 * (1 - r1) * 1 - r2) ... rN (1 - rN)...), where rN is the redeemableRate at snapshot N (the redeemableRate is the value [0, 1] indicating what % of eligible requested withdrawals can be fulfilled by that snapshot).

In plain English, each term that's added together in parentheses is the amount that the lender receives in snapshot N. So adding them together gives you the total balance at that period N (when multiplied by the lender's eligible balance). Let's call this aggregate total aggregateSumN (aggregate sum of 1..N).

Additionally, let's call the inner, accumulating terms (1 - r1) * (1 - r2) ... (1 - rn) as productDifferencesN (accumulation of terms up to period N).

This works for a base case where the lender has eligible shares from the get-go. However, if they request mid-way, then we need to offset it by the prior periods before they requested (or were last cranked at).

This ends up being, assuming a lender requested at period X:

lenderEligibleBalance * (aggregateSumN - aggregateSumX) * 1/productDifferencesX.

If you expand out the full formula, you see within the parentheses, the prior snapshots (that the lender wasn't "present" for) cancels out through the subtraction. Then, the division by 1/productDifferencesX divides out the prior ratios that the lender wasn't present for in the remaining terms.

In each crank, we accumulate both the aggregateSumN value and the productDifferencesN term with each successive snapshot. This is persisted as snapshot metadata.

Dust / math

One important edge case I ran into was the scenario where the snapshot can 100% fulfill all the requests (aka redeemRatio is 1.0). This has the effect of zero-ing out the productDifferencesX value from then on to eternity (which accumulates terms of (1 - redeemRatioN), which reduces to (1 - 1) = 0, which is then multiplied by all other terms), which is OK algebraically (since that zero term will cancel out through the division of 1/productDifferencesX, when you incorporate the offsets), but in practice using actual calculated numbers, throws everything off.

I solved for this by never actually allowing a redeemRatio of 1.0 -- only release 1 less than the available shares, in that case. This works out in practice, but leaves dust. This affected a bunch of our tests, which use small values so it was more apparent.

I think in practice with real numbers it's OK, but a consideration with this approach. Couldn't find another way to manage this.

venables
venables previously approved these changes Nov 14, 2022
Copy link
Contributor

@venables venables left a comment

Choose a reason for hiding this comment

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

wow this looks great

@ams9198 ams9198 marked this pull request as ready for review November 16, 2022 02:10
venables
venables previously approved these changes Nov 17, 2022
Copy link
Contributor

@venables venables left a comment

Choose a reason for hiding this comment

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

💪 This is a lot but it looks correct to me

@ams9198 ams9198 merged commit 75b05a5 into circlefin:master Nov 22, 2022
@ams9198 ams9198 deleted the VAL-69-constant branch November 22, 2022 14:51
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.

2 participants