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

fix: math error in tick updates during swaps #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mariorz
Copy link

@mariorz mariorz commented Sep 29, 2021

Hello,
It appears there's a bug in the tick section of handleSwap that result in ticks (and tickDayDatas) not getting updated when they should, causing a mismatch between subgraph and blockchain data, see [1].

Using small numbers to describe the issue, let's assume a pool's state in which the current tick is 19 and a swap occurs that pushes the current tick to 21, and let's also assume a tick spacing of 10 for this pool.

handleSwap in that example would compute modulo = newTick.mod(tickSpacing) == 1 and firstInitialized would thus be defined as firstInitialized = oldTick.plus(tickSpacing.minus(modulo)) == 28. The correct value for firstInitialized should of course be 20 instead.

I'm syncing a subgraph with the changes here, but it still has a long way to go.

[1] I've noticed bogus subgraph data while rendering charts of daily fees for positions, but I believe issue 44 is a good example. I wrote a test to compare subgraph data with blockchain data at the specified block, uploaded here. Use brownie to run it: brownie run tickstest.py --network mainnet.

Results:

Testing contract calls vs the graph values for
pool:0x4e68ccd3e89f51c3074ca5072bbac773960dfa36
tick:-195660
block:13010089
feeGrowthOutside0X128 from the blockchain: 1067292557275396938893002967958758719094
feeGrowthOutside0X128 from the graph     : 335883582520604181952009590658028448194028
Mismatch ✘

@mariorz mariorz changed the title fixes math error in tick updates during swaps fix: math error in tick updates during swaps Sep 29, 2021
@ianlapham
Copy link
Member

@mariorz Thanks for looking into this - this is definitely the type of bug that would go unnoticed if no one tested against accurate data

When your subgraph syncs Would love to view results - if all looks good can merge and sync this change

@mariorz
Copy link
Author

mariorz commented Oct 14, 2021

Thanks @ianlapham, so the subgraph is fully synced now at https://thegraph.com/hosted-service/subgraph/mariorz/uniswapv3

And running the above shared script does result in the test passing, results below. However I have still found mismatches with other parameters, e.g. (pool:0xc2e9f25be6257c210d7adf0d4cd6e3e881ba25f8, tick:-78060,block:12613933). So there seems to be some other issue. I'm going to investigate some more and hope to ping back later if that makes sense. Cheers.

Results:

-----------------------------
Testing contract calls vs the graph values for
pool:0x4e68ccd3e89f51c3074ca5072bbac773960dfa36
tick:-195660
block:13010089
feeGrowthOutside0X128 from the chain: 1067292557275396938893002967958758719094
feeGrowthOutside0X128 from the graph: 1067292557275396938893002967958758719094
Match ✓

@usgeeus
Copy link

usgeeus commented Sep 13, 2023

@mariorz Did you find what other issue was?

@ackvf
Copy link

ackvf commented Jan 17, 2024

Would it make sense to merge this fix regardless of the other issue? Fix one thing at a time in small discrete PRs.

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.

4 participants