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.

Sign up for GitHub

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

Jump to bottom

feat (#major); aave-forks; upgrade aave-forks to schema 3.0.1 and use sdk #1993

Merged
merged 62 commits into from
Jun 29, 2023
Merged

feat (#major); aave-forks; upgrade aave-forks to schema 3.0.1 and use sdk #1993

merged 62 commits into from
Jun 29, 2023

Conversation

tnkrxyz
Copy link
Collaborator

@tnkrxyz tnkrxyz commented Apr 10, 2023 •
edited
Loading

See #1519 a list of tasks in this PR.

Almost there. I have a few questions, @dmelotik, maybe you can help:

  • What should be the FeeType for Flashloan fees? FeeType.OTHER? Added FeeType.FLASHLOAN_FEE_LP and FeeType.FLASHLOAN_FEE_PROTOCOL

  • Currently risk type is set at protocol level, but aave-v3 allow both isolated and global risk type at user/account level. It seems it may be necessary to add a RiskType.MIXED? Noted in README

  • interestRateType info is available in Borrow events, but not in Repay events. Unless we use callHandler for Repay (but then it still won't work for chains not supporting callHandler), we don't know the interestRateType for Repays. This creates a problem that repays cannot find matched borrowing positions to subtract: https://github.com/aave/aave-v3-core/blob/29ff9b9f89af7cd8255231bc5faf26c3ce0fb7ce/contracts/protocol/libraries/logic/BorrowLogic.sol#L32-L47 . But if we ignore interestRateType for borrows, we may combine stable and variable interest positions (this may be fine?) There is a similar issue for Liquidations, we don't know the interestRateType for liquidated Borrow position and both Variable and Stable positions may be liquidated at the same time. came up with a solution to detect interestRateType for repays and liquidations

A couple possible improvements to lending sdk I hope to check whether makes sense:

  • use enum type to define function argument type, e.g. getOrCreateRewardType(rewardTokenType: string) ==> getOrCreateRewardType(rewardTokenType: RewardTokenType). We just need to add export type RewardTokenType = string after the enum definition. Similarily for InterestRateType, PositionSide, TransactionType

  • specify a tokenPricier object in the manger class, that basically calls getAssetPriceInUSDC() (this is how bridge sdk handle prices) so the sdk can determine best place to refresh prices.

Deployment & Dashboard

@melotik melotik linked an issue Apr 10, 2023 that may be closed by this pull request
Upgrade AAVE to 3.0.0 Schema #1519
Closed
9 tasks
@melotik
Copy link
Contributor

melotik commented Apr 18, 2023

We can either use fee type other and note in the readme or update the schema. Same w the interest rate type.

I am going to opt for the readme option, I don't think the extra work to update is worth it for 1 protocol.

@melotik
Copy link
Contributor

melotik commented Apr 18, 2023

We currently don't specify stable vs variable borrows.. do we need to add it?

I think it is valuable info, but we can skip for now and make a note in the readme. I think the logic we have is working regardless of borrow type.

Also, you would know which side based on the token, variable and stable borrow tokens have different contract addresses. You can add a field to the token for which side it is and compare

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Apr 19, 2023 •
edited
Loading

We currently don't specify stable vs variable borrows.. do we need to add it?

I think it is valuable info, but we can skip for now and make a note in the readme. I think the logic we have is working regardless of borrow type.

As I think of it, interest rate type can be found in matched position, e.g. using this query.

I can add an interestRateType field to Borrow entity if you think it still makes sense to add it now. It is possible to get it from

Also, you would know which side based on the token, variable and stable borrow tokens have different contract addresses. You can add a field to the token for which side it is and compare

I believe the asset arg in Borrow and Repay event is the underlying (input) token, not the vToken or sToken, so we cannot know whether it is stable or variable interest type. The info is available in the params struct, but not in the event.

https://github.com/aave/aave-v3-core/blob/29ff9b9f89af7cd8255231bc5faf26c3ce0fb7ce/contracts/protocol/libraries/logic/BorrowLogic.sol#L157 and https://github.com/aave/aave-v3-core/blob/29ff9b9f89af7cd8255231bc5faf26c3ce0fb7ce/contracts/protocol/libraries/logic/BorrowLogic.sol#L262

Similarly, the LiquidationCall event only provides the asset token:
https://github.com/aave/aave-v3-core/blob/29ff9b9f89af7cd8255231bc5faf26c3ce0fb7ce/contracts/protocol/libraries/logic/LiquidationLogic.sol#L233-L241

@tnkrxyz tnkrxyz marked this pull request as ready for review April 20, 2023 00:03
@tnkrxyz tnkrxyz requested a review from melotik April 20, 2023 00:04
@melotik
Copy link
Contributor

melotik commented Apr 20, 2023

@tnkrxyz

believe the asset arg in Borrow and Repay event is the underlying (input) token
Ah, this is true

Okay this is what I found (lmk if this doesn't work, but it makes sense in my head):

  • Like you said we can get the interest rate mode from the borrow event
  • That is what creates the position so it is already in the position entity (it is not possible for a user to have a variable and stable borrow from the same market, afaik), so no need to add an identifier for that in the position entitiy.
  • You could add a helper field in the Borrow entity if you would like, but I don't think it is necessary. Could be a nice to have.
  • BUT, a user can swap borrow modes. So we need to be watching the SwapBorrowRateMode event now.

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Apr 21, 2023

@tnkrxyz

believe the asset arg in Borrow and Repay event is the underlying (input) token
Ah, this is true

Okay this is what I found (lmk if this doesn't work, but it makes sense in my head):

  • Like you said we can get the interest rate mode from the borrow event
  • That is what creates the position so it is already in the position entity (it is not possible for a user to have a variable and stable borrow from the same market, afaik), so no need to add an identifier for that in the position entitiy.

I believe it is possible for a user to borrow in both variable and stable interest rate mode at the same time. For example, in this dune query, you can see user 0x7c04e80e5d8a19ef014e51e27b3d92cc4d87b515 borrowed 25000000000 with variable rate mode (mode=2) at block 27289290, and then another 25000000000 with the stable rate mode (mode = 1) at block 27289554. Neither position was repaid until block 27633344 and 27633420, according to this query. And there was no SwapBorrowRateMode event.

Since this is the case, if we remove the interestRateType suffix from positionID, we will be adding the later stable borrow in the above example into the previous variable borrow position. But may be this is OK?

  • You could add a helper field in the Borrow entity if you would like, but I don't think it is necessary. Could be a nice to have.

If we are not having the interest rate type in position, it'll be a good idea to have a helper field for interest rate type in Borrow entity.

  • BUT, a user can swap borrow modes. So we need to be watching the SwapBorrowRateMode event now.

If we are not tracking interestRateType for positions, Borrow, Repay, or Liquidation), then we don't need to watching this, right?

@melotik
Copy link
Contributor

melotik commented Apr 21, 2023

@tnkrxyz you are right!

  • As a note you have a deeper knowledge on this issue than I do rn, so feel free to do what you need to get it to work (ideally avoiding callHandlers)

Sounds like you can have 2 pos in a market with different interest rate types, so we should definitely separate by interest rate type and add any helper fields that are needed to enable this.

lmk if you have any other questions!

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Apr 21, 2023

@tnkrxyz you are right!

  • As a note you have a deeper knowledge on this issue than I do rn, so feel free to do what you need to get it to work (ideally avoiding callHandlers)

Sounds like you can have 2 pos in a market with different interest rate types, so we should definitely separate by interest rate type and add any helper fields that are needed to enable this.

lmk if you have any other questions!

I have to think how to achieve this without using callHandler.

@melotik melotik mentioned this pull request Apr 24, 2023
Closed
@melotik
Copy link
Contributor

melotik commented Apr 24, 2023

@tnkrxyz I think one of the biggest issues with the call handlers is not being supported on every chain and we need to support all chains.

One way I can think of would be:

  • Watch the Repay events emitted, then go up in the event logs to see which DebtToken was burned.
  • You will have to watch the useATokens params in the event to know which log index you are looking at for the repay type.

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Apr 25, 2023 •
edited
Loading

@tnkrxyz I think one of the biggest issues with the call handlers is not being supported on every chain and we need to support all chains.

One way I can think of would be:

  • Watch the Repay events emitted, then go up in the event logs to see which DebtToken was burned.
  • You will have to watch the useATokens params in the event to know which log index you are looking at for the repay type.

@dmelotik Yes, this can work! thx for helping brainstorming ideas!

I ended up adding an _AccountDebtBalance helper entity to keep track of vToken and sToken balance for borrower account in the market. Inside handleRepay and handleLiquidation, I check which balance(s) goes down. For repay, only one type of debt can be repaid in a repay call and it should be the one with lower balance. Similarly for liquidation, but here, both token balances can go down. I am running test deployment to see if it works as expected.

@melotik
Copy link
Contributor

melotik commented Apr 25, 2023

For repay, only one type of debt can be repaid in a repay call and it should be the one with lower balance

Doesn't the repayer specify in the call function parameters which side is being paid? So it could be either?

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Apr 26, 2023

For repay, only one type of debt can be repaid in a repay call and it should be the one with lower balance

Doesn't the repayer specify in the call function parameters which side is being paid? So it could be either?

@dmelotik yes, correct. I see that my sentence above may be confusing, what I meant is that, for each repay event, it is either Variable or Stable debt that's been repaid, not both types in one event. And the type should correspond to the (v or s) token with the lower balance. For liquidation, both v & s token (if both > 0) can be repaid/liquidated within the same event. sorry for the confusion.

This was referenced Apr 28, 2023
Closed
Closed
@melotik
Copy link
Contributor

melotik commented May 17, 2023

I am seeing crazy depositUSD/borrowUSD prices

Quick fix: we just need to normalize amountUSD with inputToken.decimals in each type of transaction:

subgraphs/subgraphs/aave-forks/src/mapping.ts

Line 672 in 6b7078b

const amountUSD = amount.toBigDecimal().times(market.inputTokenPriceUSD);

I am also noticing the new subgraph has less borrowCount and no RepayCount which indicates to me that positions are not being matched up in repays (and that is what looks like is happening in the logs)

Something I can tell is wrong: In both examples of checking for transfer events to get the interest rate type in Repay there are approvals that occur. But that is not always the case. In this example the Transfer event we want to see is only 4 before and not 5. To account for this we may want to search for the Transfer event in a range of 4-5.

However, it should still pickup the ones that are 5 away, yet there are no repays

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented May 17, 2023 •
edited
Loading

I am seeing crazy depositUSD/borrowUSD prices

Quick fix: we just need to normalize amountUSD with inputToken.decimals in each type of transaction:

subgraphs/subgraphs/aave-forks/src/mapping.ts

Line 672 in 6b7078b

const amountUSD = amount.toBigDecimal().times(market.inputTokenPriceUSD);

I am also noticing the new subgraph has less borrowCount and no RepayCount which indicates to me that positions are not being matched up in repays (and that is what looks like is happening in the logs)

Something I can tell is wrong: In both examples of checking for transfer events to get the interest rate type in Repay there are approvals that occur. But that is not always the case. In this example the Transfer event we want to see is only 4 before and not 5. To account for this we may want to search for the Transfer event in a range of 4-5.

However, it should still pickup the ones that are 5 away, yet there are no repays

ok, both should be fixed now.

@melotik melotik mentioned this pull request May 23, 2023
Closed
@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Jun 14, 2023 •
edited
Loading

@dmelotik , the negative revenue is caused by the deduction of flash loan premium from totalRevenue in handleReserveDataUpdate, but I am not sure why LiquidityIndex in reserveDataUpdate event does not include the flashloan premium to LP as we believed.

For example, according to these subgraph log

  {
    "timestamp": "2023-06-13T23:25:09.137907068Z",
    "text": "[_handleFlashLoan]premiumAmount=30000000000000000000 premiumUSDTotal=52620.9987 premiumUSDToProtocol=7914.19820448 premiumUSDToLP=44706.80049552 premiumUSDToDeduct=52599.95030052 market=0x4d5f47fa6a74757f35c14fd3a6ef8e3c9bc514e8 tx=0xeb87ebc0a18aca7d2a9ffcabf61aa69c9e8d3c6efade9e2303f8857717fb9eb7-809 timestamp=1686531995, data_source: LendingPool, component: UserMapping"
  },
  {
    "timestamp": "2023-06-13T23:25:09.153314086Z",
    "text": "Done processing trigger, gas_used: 11637673778, data_source: LendingPool, handler: handleFlashloan, total_ms: 83, transaction: 0xeb87…9eb7, address: 0x8787…a4e2, signature: FlashLoan(indexed address,address,indexed address,uint256,uint8,uint256,indexed uint16)"
  },
  {
    "timestamp": "2023-06-13T23:25:09.168726304Z",
    "text": "Applying 52 entity operation(s), block_hash: 0x101132f147d5d6103cfdcb3dbad67577a3486f9fb7de632a21778d71701b0360, block_number: 17460610"
  },
  {
    "timestamp": "2023-06-13T23:25:09.253221374Z",
    "text": "[_handleReserveDataUpdated]totalRevenueDeltaUSD=-52537.48083701298829788585188957448 protocolSideRevenueDeltaUSD=-5253.748083701298829788585188957448 supplySideRevenueDeltaUSD=-47283.732753311689468097266700617032 flashloandPremiumToDeduct=52599.95030052 reserveFactor
=0.1 market=0x23878914efe38d27c4d67ab83ed1b93a74d4086a tx=0x1a520aea1fa610109a332a7408aa8d595fd83a40328b4709438937ee83c5b989-256 timestamp=1686532175, data_source: LendingPool, component: UserMapping"
  },

At flashloan tx 0xeb87ebc0a18aca7d2a9ffcabf61aa69c9e8d3c6efade9e2303f8857717fb9eb7, the premium amount is 30 WETH, or $52620.9987, of which $52599.95030052 goes to the LP and supposedly should be deducted from total revenue. However, the ReserveDataUpdated event followed (0x1a520aea1fa610109a332a7408aa8d595fd83a40328b4709438937ee83c5b989), the calculated total revenue from LiquidityIndex is only $62.46. Revenue became negative after deducting flash loan premium of $52599.95030052. This makes me think that we don't need to deduct flashloan premium.

But from what I can see, the premium to LP is accrued in LiquidityIndex in the smart contract. I am not sure what is wrong and leads to the revenue not included in our totalRevenue. Any idea?

@melotik
Copy link
Contributor

melotik commented Jun 14, 2023

Let me ask the aave team. This is something we need to ensure we have right. And it may be the case that we don't need to subtract the revenue

@melotik
Copy link
Contributor

melotik commented Jun 22, 2023

@tnkrxyz A few things I found:

  • I looked at the largest flashloan in aave v3 ethereum (here) and there is a spike in revenue in the WETH market on the same day. It is definitely getting distributed to LPs through the liquidity Index. I just think we are doing something wrong in the subgraph code now.
  • Aave V2 also handles this differently, so we will need to have a way to switch between the different logics in flashloan distribution

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Jun 24, 2023

@tnkrxyz A few things I found:

  • I looked at the largest flashloan in aave v3 ethereum (here) and there is a spike in revenue in the WETH market on the same day. It is definitely getting distributed to LPs through the liquidity Index. I just think we are doing something wrong in the subgraph code now.
  • Aave V2 also handles this differently, so we will need to have a way to switch between the different logics in flashloan distribution

@dmelotik , I think I finally figured out what is causing this and am testing a fix. more update soon.

@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Jun 26, 2023

@dmelotik , ok, here is what I found so far: the reason we are getting negative revenue is due to mis-match of FlashLoan event and ReserveDataUpdated event in the current implementation. The ReserveDataUpdated event associated (with the updated liquidityIndex) with a FlashLoan event is emitted before the FlashLoan event. My current code assumes the ReserveDataUpdated event is after the FlashLoan event, which leads to the accrued LP revenue lower than flashloan premium.

I implemented a fix and it fixes the large negative revenue we have seen, but it still has some tiny negative revenues (the most negative number I see is totalRevenueDeltaUSD=$-0.00106344452993117741167457972125, most is about $-0.0002 to $-0.0005; see this query), which I suspect is due to rounding, but I am not certain.

@melotik
Copy link
Contributor

melotik commented Jun 27, 2023

Nice job catching this @tnkrxyz ! I reviewed the new code and it looks good, makes sense to me.

I would think those small, small discrepancies are also due to rounding. I see 2 ways to go about it.

  • Either we just leave it since it doesn't actually make any unexpected values
  • We make any small neg values 0

I would say 3 things before we merge:

  • Solve merge conflicts (apologies for that)
  • Come up with a solution for the above
  • Check out the versions, for some reason your latest aave-v3-ethereum deployment shows schemaVersion = 2.0.1

@tnkrxyz tnkrxyz requested a review from melotik June 28, 2023 04:43
@tnkrxyz
Copy link
Collaborator Author

tnkrxyz commented Jun 28, 2023

I would say 3 things before we merge:

  • Solve merge conflicts (apologies for that)

merged.

  • Come up with a solution for the above

$0 > negative revenue > $-0.01 is set to 0.

  • Check out the versions, for some reason your latest aave-v3-ethereum deployment shows schemaVersion = 2.0.1

fixed.

melotik
melotik approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@melotik melotik left a comment

Choose a reason for hiding this comment

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

Good work @tnkrxyz !

@melotik melotik merged commit 82b9b38 into messari:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melotik melotik melotik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade AAVE to 3.0.0 Schema
2 participants
@tnkrxyz @melotik