Schema updates #7
Conversation
| addQuoteToken.index = event.params.index.toU32() | ||
| addQuoteToken.amount = wadToDecimal(event.params.amount) | ||
| addQuoteToken.lpAwarded = wadToDecimal(event.params.lpAwarded) | ||
| addQuoteToken.lup = wadToDecimal(event.params.lup) |
There was a problem hiding this comment.
Is there any case where we would want to access the raw integer data of the event instead of the parsed human friendly version?
There was a problem hiding this comment.
I was on the fence about LP values. My goal was to make events consistent with non-event entities, and LPs were already BigDecimal there, so I ran with it.
Should we use BigInt for all LP values across both entity types?
There was a problem hiding this comment.
I think LPs should probably be handled similarly to the other accounting. I don't have a strong opinion on it one way or the other. Consistency sounds good, and the conversion accuracy seems high so happy with your approach.
| } | ||
|
|
||
| // update the list of lends initiated by an account, if it hasn't been added already | ||
| export function updateAccountLends(account: Account, lend: Lend): void { | ||
| const lends = account.lends | ||
| // get current index of lend in account's list of lends | ||
| const index = lends.indexOf(lend.id) | ||
| if (index == -1) { | ||
| if (lend.lpb != ZERO_BD && index == -1) { |
There was a problem hiding this comment.
Is there any use case for tracking the closed out lend / loan positions for historical analytics purposes? Thinking specifically about displaying a historical positions tab
There was a problem hiding this comment.
The use case of historical data for analytics should be handled by BlockAnalytica. Our subgraph should only endeavor to retain history in places where it is needed for our UI, particularly for liquidations and burn rewards. It's a shame TheGraph doesn't let you query state at a particular block height like Vulcanize did.
There was a problem hiding this comment.
I was under the impression that BlockAnalytica would be using this subgraph for their analytics dashboard?
There was a problem hiding this comment.
Also, I believe that you can query state at a particular block height: https://thegraph.com/docs/en/querying/graphql-api/#time-travel-queries
There was a problem hiding this comment.
51c184a makes some adjustments and documents this topic in the README. Will share this with BlockAnalytica once we agree upon it to ensure there are no concerns.
src/utils/account.ts
Outdated
| @@ -32,15 +32,19 @@ export function updateAccountPools(account: Account, pool: Pool): void { | |||
| if (index == -1) { | |||
| account.pools = account.pools.concat([pool.id]) | |||
| } | |||
| // TODO: Determine whether account has any Lend or Loan in pool without O(n) iteration. | |||
There was a problem hiding this comment.
We could potentially use an ennumerable set for this - it might be a better way to track than raw filtering
|
|
||
| settle.save() | ||
| } | ||
|
|
||
| export function handleTransferLPs(event: TransferLPsEvent): void { |
There was a problem hiding this comment.
We probably want to call out to a loadOrCreateLend for the new owner, as well as add an account entity for them.
There was a problem hiding this comment.
Also, do we want to increment the pool and token tx counts in this method?
There was a problem hiding this comment.
Yes, done in 67577d0. Note that I'm not using incrementTokenTxCount because this action has no bearing on collateral. Also not sure if it's really the quote token or the LP token whose TX count should be incremented. Happy to discuss further.
There was a problem hiding this comment.
Yeah, those are really good questions. It would potentially be both if there was quote and collateral in the bucket. Might be a heavy computational lift to calculate that info given a potential requirement for additional RPC calls. RPC calls seem to be the real limiting factor.
There was a problem hiding this comment.
So, for now, should I just sprinkle incrementTokenTxCount for every lender operation?
There was a problem hiding this comment.
Yeah I think that's reasonable. It would reflect higher pool activity for that token
src/position-manager.ts
Outdated
| if (token == null) { | ||
| // create new account if account hasn't already been stored | ||
| token = new Token(id) as Token | ||
| token.name = getTokenName(tokenAddress) |
There was a problem hiding this comment.
These methods won't work for PositionManager ERC721 NFT's since they were intended only for ERC20.
There was a problem hiding this comment.
In 67577d0 I removed decimals and totalSupply, two methods which are not supported by ERC-721.
There was a problem hiding this comment.
That was an improvement, but getTokenName, and getTokenSymbol are both binding to the ERC20 contract, which may result in unexpected behavior. We should add an ERC721 contract and abi under the ERC721 pool factory and create methods binding to those. May be out of scope though.
There was a problem hiding this comment.
Although the ERC-721 pool is out-of-scope for this PR, It makes sense to add the ABI now to fix this. Done in 64bed6f.
| @@ -186,6 +190,24 @@ export function updatePoolLiquidationAuctions(pool: Pool, liquidationAuction: Li | |||
| } | |||
| } | |||
|
|
|||
| // if present, remove a settled liquidation from the pool's collection of active liquidations | |||
| export function removeLiquidationFromPool(pool: Pool, liquidationAuction: LiquidationAuction): void { | |||
There was a problem hiding this comment.
Similar question about leaving dead entities in storage for historical analytics purposes. Also, should this be splice instead of slice?
There was a problem hiding this comment.
Good catch on the splice vs slice; fixed in 67577d0.
Although we generally don't want to persist dead entities in our subgraph, there are use cases for storing historical liquidations, particularly for the UI. I am considering a separate collection in the schema for these.
Added this to the next epic for discussion.
|
@MikeHathaway I addressed many of the concerns. Happy to continue discussion about the others. |
|
Big picture, should we also handle Grant Fund as part of this subgraph as opposed to a separate repository? |
I would prefer this repo cover the whole protocol, but I am not ready to commit to that decision. |
I agree. I had added a paragraph in the README to the contrary that we might want to remove while a decision is pending.* Moved this here as I had added that text. Thanks for the README change. |
Several changes, notably to
PoolandLiquidationAuctionentities. Bucket indicies changed fromBigInttoInt. Event prices and amounts changed fromBigInttoBigDecimal, consistent with other entities. RemoveLendentity from account when lender has no LP in bucket. RemoveLoanentity from account when borrower has no debt.Unit tests have only been updated to keep them running; no new coverage has been implemented.