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

feat(#major); kwenta; new subgraph #2297

Merged
merged 7 commits into from
Nov 5, 2023

Conversation

amritkumarj
Copy link
Contributor

@amritkumarj amritkumarj commented Jul 24, 2023

This was linked to issues Jul 31, 2023
@ishraq8
Copy link
Contributor

ishraq8 commented Jul 31, 2023

this version with tvl passed QA

subgraphs/kwenta/README.md Outdated Show resolved Hide resolved
@melotik melotik changed the title feat: kwenta feat(#major); kwenta; new subgraph Aug 9, 2023
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.

@amritkumarj a few notes before a deeper PR:

  • I notice there are a lot of files/functions that are unused. These should be removed to simplify the code and make the review smaller
  • The SDK should cover a lot of the code
  • My comments are a starting place, I do see some more things that are unused
  • Can you add the deployment link to the header comment in the PR

Thank you!

Copy link
Collaborator

@dhruv-chauhan dhruv-chauhan left a comment

Choose a reason for hiding this comment

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

As @dmelotik mentioned, you don't really need most files in src/common and src/utils since these are included in the sdk

subgraphs/kwenta/package.json Show resolved Hide resolved
subgraphs/kwenta/schema.graphql Outdated Show resolved Hide resolved
subgraphs/kwenta/package.json Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Outdated Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@dhruv-chauhan dhruv-chauhan left a comment

Choose a reason for hiding this comment

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

also, please fix lint errors 😃

subgraphs/kwenta/src/mappings/handlers.ts Outdated Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Outdated Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Show resolved Hide resolved
subgraphs/kwenta/src/mappings/handlers.ts Outdated Show resolved Hide resolved
deployment/deployment.json Outdated Show resolved Hide resolved
@amritkumarj
Copy link
Contributor Author

Hey @dhruv-chauhan

I think there is some confusion between working of kwenta and gmx

In gmx the collateral is deposited when we are opening the position, and when we close the position we receive the collateral back in our account

But this is not the case with kwenta - we first deposit collateral i.e. sUSD for all markets and then we open any position, and on close just the collateral value is increased, which can be withdrawn by the user in a different transaction

And there are no liquidity pools in Kwenta like GMX

So, I think instead of "deposit()" and "withdraw()" in "handleMarginTransferred" we should use collateral in and out, and not use these functions in "handlePositionModified"

And "deposit()" & "withdraw()" function should be removed as they are for liquidity pools

What do you think?

@dhruv-chauhan
Copy link
Collaborator

dhruv-chauhan commented Aug 31, 2023

If I understand correctly,

  1. User deposits sUSD as liquidity to a sTOKENPERP [handleMarginTransferred]
  2. Opens a position with part of deposited sUSD as collateral [handlePositionModified]
    ...
  3. Closes position, gets back collateral [handlePositionModified]
  4. Exits market, withdraws liquidity [handleMarginTransferred]

So imo,
deposit/withdraw of liquidity in handleMarginTransferred and,
collateralIn/collateralOut of trading collateral in handlePositionModified works fine as currently implemented.

@ishraq8 can overrule

@ishraq8
Copy link
Contributor

ishraq8 commented Sep 7, 2023

he case with kwenta - we first deposit collateral i.e. sUSD for all markets and then we open any position, and on close just the collateral value is increased, which can be withdrawn by the user in a different transaction

This makes sense. sUSD becomes the collateral instead of other input tokens.

@amritkumarj
Copy link
Contributor Author

This makes sense. sUSD becomes the collateral instead of other input tokens.

Yes, sUSD is used as collateral but what I wanted to say is there is no withdrawal or deposit of collateral at time of positionModified that got me into some confusion

But I think what @dhruv-chauhan said makes sense and fixed the issues according to his solution

Copy link
Collaborator

@dhruv-chauhan dhruv-chauhan left a comment

Choose a reason for hiding this comment

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

Overall the code looks good. 🤙

Some of the data looks a little odd to me, but I'll leave verification for QA. Example,

  • (derivPerpProtocols) longPositionCount ~ 19k vs shortPositionCount ~ 200
  • (derivPerpProtocols) cumulativeInflowVolume ~ 4.7b vs cumulativeOutflowVolume ~ 470m

** Needs to be fixed,

  • (usageMetricsDailySnapshots) dailyActiveLiquidators > cumulativeUniqueLiquidators query

cc @ishraq8

@ishraq8
Copy link
Contributor

ishraq8 commented Oct 31, 2023

Data looks good and passed QA.

looks like the longpositioncount and shortpositioncount makes sense long is preferred positions. The short and long open interest seem similar in value which is where matching really matters.

Cumulative inflow will always be greater than outflow as the protocol relies on higher inflow. if it is reveresed it indicates there is a bankrun or unhealthy protocol (kwenta is quite healthy).

@dhruv-chauhan
Copy link
Collaborator

** Needs to be fixed,

  • (usageMetricsDailySnapshots) dailyActiveLiquidators > cumulativeUniqueLiquidators query

Hey @amritkumarj, if you could just fix cumulativeUniqueLiquidators/dailyActiveLiquidators calculations, we'd be ready to merge the PR!

@amritkumarj
Copy link
Contributor Author

Fixed it in the new commit

This issue is also present in gmx - query

Copy link
Collaborator

@dhruv-chauhan dhruv-chauhan left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@dhruv-chauhan dhruv-chauhan merged commit ad546dd into messari:master Nov 5, 2023
4 checks passed
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.

Kwenta - OPTIMISM Build Kwenta Subgraph
4 participants