-
Notifications
You must be signed in to change notification settings - Fork 4
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
Subgraph Improvements #55
Conversation
@@ -151,6 +151,8 @@ type Bucket @entity { | |||
deposit: BigDecimal! | |||
# total LP for all lenders in the bucket | |||
lpb: BigDecimal! | |||
# list of lends associated with the bucket | |||
lends: [Lend!]! |
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.
Need a way to know all lends in a bucket whose LPB should be zeroed out on bankruptcy
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.
Possible issue managing depositTime
on moveQuoteToken
operations.
src/mappings/base/base-pool.ts
Outdated
@@ -220,8 +222,10 @@ export function _handleMoveQuoteToken(erc20Event: MoveQuoteTokenERC20Event | nul | |||
// update to bucket lend state | |||
const toBucketLendId = getLendId(toBucketId, lender) | |||
const toBucketLend = loadOrCreateLend(toBucketId, toBucketLendId, pool.id, moveQuoteToken.lender) | |||
toBucketLend.depositTime = getDepositTime(blockTimestamp, toBucketLend) |
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.
Seems like this will always set depositTime
to blockTimestamp
(toBucketLend
could not have a greater time) rather than setting to the max of fromBucketLend.depositTime
and toBucketLend.depositTime
- see logic here.
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.
Updated in: 039811a
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.
LGTM
src/mappings/base/base-pool.ts
Outdated
|
||
// add new lend | ||
const newLend = loadOrCreateLend(bucketId, newLendId, pool.id, transferLP.newOwner) | ||
// TODO: should we simply use lenderInfo.depositTime instead of getDepositTime? |
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.
Since we're already calling it on line 438, it seems safer to take the value. But the logic on line 437 seems to jive with the contract.
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.
Should we also remove the getLenderInfo
call for oldLend.lpb
?
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.
Updated to use lenderInfo for depositTime in 5fd73b5
// Redeem - transfer from PositionManager to lender, creating the lender's Lend | ||
|
||
// event does not reveal LP amounts transferred for each bucket, so query the pool and update | ||
// remove old lend |
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.
This comment says "remove old lend" but the old lends aren't currently being removed
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.
On line 431, updateAccountLends
removes the lend if LPB is zero:
Line 52 in d3806c0
lends.splice(index, 1) |
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.
At least in the unit tests the Lends aren't being spliced even with zero lpb: https://github.com/ajna-finance/subgraph/pull/55/files#diff-865148f3208c67be9e4400503f5768dcc16d6880abb57e369ed24558c6eb8a22R1471
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.
Basically the Lends are still in the store, even though they are no longer being associated with the Account
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.
Indeed; we're not doing a store.remove
anywhere, and probably should be.
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.
Lends
are attached to Accounts, Buckets, and Positions. Seems when LPB=0 they are removed from Accounts and Positions, but not Buckets. We'll need to fix that before doing a store.remove
.
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.
Added an additional updateBucketLends
to _handleTransferLP
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.
So when someone redeems a position NFT, _handleTransferLP
takes care of Account.lends
and Bucket.lends
, and handleRedeemPosition
takes care of Position.indexes
.
_handleTransferLP
to reduce complexityhandleTransferLP
&MoveLiquidity