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-129 [Refactor] Switch to per-lender "crank" that iterates over snapshots #179

Merged
merged 11 commits into from
Feb 17, 2023
Merged

VAL-129 [Refactor] Switch to per-lender "crank" that iterates over snapshots #179

merged 11 commits into from
Feb 17, 2023

Conversation

ams9198
Copy link
Contributor

@ams9198 ams9198 commented Feb 14, 2023

The diff looks huge, but the vast majority of that is tweaking existing tests and adding new ones.

Per convo on Slack, this is a refactor (actually simplification) of the Pool "snapshot" and the per-lender mechanism for "claiming" their portion of the snapshotted funds. Instead of a direct computation using aggregated values, this iterates over the interim snapshots from the time a lender requested and accumulates shares / assets according to each snapshot "redeemableRate" and "fxRate". The global snapshot is still constant-time and run lazily on mutating events (as before).

There are two new functions exposed to the Pool/WithdrawController (very open to feedback on naming and the signatures of these):

claimSnapshots(uint256 limit) --> iterates of "n == limit" snapshots to "catch up" the lender. This updates the lender's withdrawState -- moving eligibleShares over to redeemableShares according to each snapshot.
needsClaim() view returns bool --> signals whether a lender is "caught up" or not.

The semantics change slightly, as there's less lazy magic as before. A simple pseudo code scenario to explain:

// Period 1. LenderA requests withdrawal
pool.requestRedeem(1000)

// Period 2, 3, etc. 
// pool gets cranked lazily on any mutating function (this is still constant-time).

// Period n 
maxWithdrawal() == 0 // lenderA hasn't "claimed" funds from the snapshots yet 

pool.claimSnapshots(100); // now they're up-to-date
maxWithdrawal() == 1000

Basically, a lender will now explicitly need to claimSnapshots before their redeemableAssets get allocated. This seems simpler to reason about -- money moves explicitly, and things like maxRedeem() simply read from the state vs. doing an invisible computation.

This fixes the specific audit issue, and also eliminates some of the "dust" quirks of the direction computation approach.

@ams9198 ams9198 marked this pull request as ready for review February 15, 2023 22:23
venables
venables previously approved these changes Feb 16, 2023
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 looks correct to me!

bricestacey
bricestacey previously approved these changes Feb 16, 2023
@ams9198 ams9198 dismissed stale reviews from bricestacey and venables via d11da72 February 16, 2023 23:51
@ams9198 ams9198 merged commit a8f157b into circlefin:master Feb 17, 2023
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.

3 participants