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

gasUsed and gasPrice in a Transaction + fixing id collisions in complex txs or in multiple txs in block + skipping snapshot when the user is null #46

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

benesjan
Copy link

Hello, I wanted to account for the transaction costs in trades and liquidity provisioning and I was missing the necessary information for that in the subgraph. I think there is no reason to not include this data as it's made readily available by the graph API.

The deployment of the modified version is accessible here.

subgraph.yaml Outdated Show resolved Hide resolved
subgraph.yaml Outdated Show resolved Hide resolved
@ianlapham
Copy link
Member

@benesjan requested minor changes - after that happy to merge this, definitely a useful feature.

@benesjan
Copy link
Author

@benesjan requested minor changes - after that happy to merge this, definitely a useful feature.

Hello, I removed the comments from the pull request 😌

@ianlapham
Copy link
Member

Before merging this, is there any reason to have these fields in the snapshot entity? Can we just have them in teh Transaction entity? Happy to merge if we can do that.

@benesjan
Copy link
Author

Before merging this, is there any reason to have these fields in the snapshot entity? Can we just have them in teh Transaction entity? Happy to merge if we can do that.

I added them to the Snapshot entity as well because it allowed me to calculate the gas costs of user's positions using 1 request instead of 2 (I need them to compute precise returns). I could first fetch the snapshots and then filter for tx but that is inefficient. But now that I am thinking about it a better solution might be to reference the Transaction entity directly.

@benesjan
Copy link
Author

benesjan commented Nov 26, 2020

Hello, now the gasUsed and gasPrice are only in the Transaction entity. I also added the tx reference to the LiquidityPositionSnapshot because that way I could calculate tx costs of user's position just by requesting the snapshots (I needed that for my project). The graph is currently syncing here and it seems to work fine 😌 .

@benesjan benesjan changed the title gasUsed and gasPrice in a Transaction gasUsed and gasPrice in a Transaction + fixing id collisions in complex txs or in multiple txs in block + skipping snapshot when the user is null Dec 6, 2020
@benesjan
Copy link
Author

Hello @ianlapham, I know that you guys have a lot of work with Uni v3 but it would be useful to have this merged into the official subgraph. The subgraph with the changes will soon be synced here. Everything works as expected and there are no breaking changes.

If you look at the subgraph log, it is now handling the null user situation which occurs in the handleBurn handler.
image

Have a good day, Jan

@ianlapham
Copy link
Member

Hey checking this out. One note - is it true that the subgraph started syncing 2 months ago? It may be stalled in that case.

@benesjan
Copy link
Author

If you look at the logs you will see that the subgraph is still syncing (blocks are being processed). I think the long sync time is simply caused by the hosted service being a bit overloaded and by the huge uniswap volume.

@benesjan
Copy link
Author

Hello @ianlapham, the subgraph is now fully synced: https://thegraph.com/explorer/subgraph/benesjan/uniswap-v2

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.

2 participants