How do you handle rounding errors between querySwap and swap?

[jamescarter-le]

It's only a small issue, especially when used for back testing and your not going to make any live transactions from the results, and you query the chain for its state at the time, however I've noticed an issue with regards to rounding with querySwap vs swap when sqrtPrice is applied, however I would expect it to resolve to the same as the Amounts in the EventLog of Swap?

This is how you determine amountRequested from your tests: https://github.com/Bella-DeFinTech/uniswap-v3-simulator/blob/dev/scripts/Events.ts

Sorry its not in your unit test format, I just wanted to verify it was working for my desired pool before hooking it up:

import JSBI from 'jsbi';
import { CorePool } from './src/core/CorePool';
import { FeeAmount } from './src/enum/FeeAmount';

function main()
{
    // Pool is placed in the Initialized state, with no operations yet invoked apart from it being Initialized.
    var pool = new CorePool("", "", FeeAmount.MEDIUM, 60, JSBI.BigInt(0), JSBI.BigInt(0), JSBI.BigInt("2505290050365003892876723467"),
    JSBI.BigInt(0), -69082, JSBI.BigInt(0), JSBI.BigInt(0));

    // Block 13578816   Tx 0x2f0461509914dbd045ae6946a26f69850b914f97a67dd6078fa5d871959fb105  LogIndex  16
    pool.mint("0xC36442b4a4522E871399CD717aBDD847Ab11FE88", 49800, 64020, JSBI.BigInt("556973545490136947176"));
    // Block 13578904   Tx 0x28fe258a62cf142fced434c5eb3ff051970470333b5770118c8df225e374849b  LogIndex  460
    pool.burn("0xC36442b4a4522E871399CD717aBDD847Ab11FE88", 49800, 64020, JSBI.BigInt("529124868215630099817"));
    // Block 13578904   Tx 0x28fe258a62cf142fced434c5eb3ff051970470333b5770118c8df225e374849b  LogIndex  462
    pool.burn("0xC36442b4a4522E871399CD717aBDD847Ab11FE88", 49800, 64020, JSBI.BigInt("0"));

    // Block 13578932   Tx 0x4d8a042cae977aa124cda85419461093fb87b04814126e024a05dc4f8252e27a  LogIndex  187
    var amount0 = JSBI.BigInt("-927248711787417535");
    var amount1 = JSBI.BigInt("226019397190954581334");
    var sqrtPrice = JSBI.BigInt("1596559182082899146010277864392");

    var zeroForOne: boolean = JSBI.greaterThan(amount0, JSBI.BigInt("0")) ? true : false;

    var queryResult = pool.querySwap(zeroForOne, amount1);
    if(!JSBI.equal(queryResult.amount0, amount0))
        console.log("Amount1: Amount 0 Wrong");
    if(!JSBI.equal(queryResult.amount1, amount1))
        console.log("Amount1: Amount 1 Wrong");

    var applyResult = pool.swap(zeroForOne, amount0, sqrtPrice);
    if(!JSBI.equal(applyResult.amount0, amount0))
        console.log("Apply: Amount 0 Wrong");
    if(!JSBI.equal(applyResult.amount1, amount1))
        console.log("Apply: Amount 1 Wrong: " + applyResult.amount1.toString() + "  vs expected  " + amount1.toString());// <---- Error, is out by 1 LSB.
}

main();

[jamescarter-le]

It only checks as below, and so test passes, but the actual result is off by 1 LSB (at least in 18 decimal token pools):

uniswap-v3-simulator/test/ConfigurableCorePool.test.ts

Lines 139 to 143 in cf3bc45

	let res = JSBI.equal(
	configurableCorePool.getCorePool().sqrtPriceX96,
	(param as SwapEvent).sqrt_price_x96
	);

It should check as per try run:

uniswap-v3-simulator/test/ConfigurableCorePool.test.ts

Lines 115 to 118 in cf3bc45

	let dryRunRes =
	JSBI.equal(amount0, param.amount0) &&
	JSBI.equal(amount1, param.amount1);

Are you saying with that test, that the amount outs don't matter, the Pool and therefore Position and Tick liquidities are all correct?

[kafeikui]

@jamescarter-le Good question. Actually check on sqrtPriceX96 after one swap is stricter than check on amount outs. If two attempts of dry run(querySwap) fail, swap will not be executed, instead it will throw an error.

I agree that we should also provide the sqrtPrice as EventLog records in querySwap, but just to clarify so far with USDC-WETH pool we haven't got an error by that. Because we give the value of amountSpecified from amount outs of the EventLog, the influence of sqrtPrice should not be obvious to amount outs. But if we don't specify that in following swap, the result of sqrtPrice can differ from EventLog. In fact I'm quite interested in 1 LSB error(in amount outs right?) you mentioned. Could you please reproduce that more specifically?

[kafeikui]

Thank you @jamescarter-le . I see your point. Let me check that.

[jamescarter-le]

Thanks @kafeikui If I do not use the sqrtPrice when doing the swap (letting the pool execute the best swap) for when running through a Back Test (as we wouldn't be able to use that Price, as the tokens locked in the pool will change, hence the price the user got will change), it also ends up with an LSB error.

I was under the impression that assuming you make no change to the order of operations, taken directly from the chain, it should wrap up to the current state of the chain?

Here is an example when you go further down that ETH/ENS pool:
0x92560C178cE069CC014138eD3C2F5221Ba71f58a

https://gist.github.com/jamescarter-le/0f815e6134bd8f299e5c4a9e3a34ae52

[jamescarter-le]

I think I'll pull down the USDC/ETH pool so I'm testing against the same data set as you for validation.

Am I correct in thinking it is this pool?
https://etherscan.io/address/0x8ad599c3a0ff1de082011efddc58f1908eb6e6d8

[jamescarter-le]

So the first rounding issue I found when opening the issue looks like a rounding issue with sqrtRatioTarget.

When you pass no limit onto HandleSwap, it uses the full step.sqrtPriceNextX96 for computeStepSwap (because each step price will never be greater than MAX_RATIO_SQRT), however when you have a price that is between the steps, it uses the supplied price limit for this function.

Then with this limit rather that MAX_RATIO_SQRT, it ends up missing a rounding on the Amount.

Here is the handleSwap with the price limit for the original top issue.:

This gives us the following AmountIn and Fee's calculated:

Whereas when we pass MAX_RATIO_SQRT into this, we get into the ""//we didn't reach the target, so take the remainder of the maximum input as fee"" section. Resulting in this:

This is the correct amount for the EventLog Swap.

Now on the Limit version, it knows it is exactIn, but we also have reached the sqrtRatioNextX96 and sqrtRatioTargetX96 (because we input these, and they've been precalculated by Uniswap already as we are re-processing).

It runs the FeeAmount with the mulDiv rounding, I assume to ensure the user doesn't over spend on fees?

I'm not a math genius and so I'm having trouble to figure what to change here, but mulDivRoundingUp calculates 2 LSB off, then rounds up one. We end up being one out at the end.

Hopefully we can get this sorted? I'm hoping this error is compounding through the chain, as if I ignore it I can process quite a few events before it blows up again on this pool.

TLDR: When passing the matching SqrtPriceLimitX96 to ComputeSwap, it will in some circumstances provide the wrong Fee amount, hence the output not matching the Ethereum Event Log.

[jamescarter-le]

Also, ran this on your ETH/USDC 3000 pool, and got this rounding problem at this point:

Swap Block 12403791 Tx 150 Log 297
0x3d9dd52a8e903b0fc02b2c393dc975b0c726d556410f20a690746086575d9cd5

[neptune-v]

@jamescarter-le Thanks for the detailed explanation of your findings, let me check with @kafeikui to see what exactly we can do to fix this.
Please note tho, we had rounding/precision issues before, and we had to fix some logic, so what you are seeing might be because you are on a branch without the fix (needs confirmation), but we will get back to you soon.
Thanks again!!

[kafeikui]

Impressive. You're right @jamescarter-le . The problem has been verified with our test data set and I have commited a fix by refining try-error process. Please see #53 . Besides, you know what we are doing is to reproduce logic and result exactly as Uniswap v3 contract do, so I do think the rounding problem is against that but will have little influence in strategy backtesting. When doing backtesting, we only pass amountSpecified(chosen from amount0/amount1 in event record) to swap because the actions from user's strategy might lead swap to fail(for example, it can't meet sqrtPriceLimitX96 condition if we pass it). That is why that from my perspective maybe swap inputs decided by try-error swap events is better than using input by parsing swap trxs directly. And of course, if you are doing events-replay work directly from the chain, try this fix and let me know if you have any problem.

Thanks a lot!

BTW, your effort and understanding on Uniswap v3 is highly appreciated. Have you considered participating in our project? We can make things better.

[jamescarter-le]

Hello both, many thanks for responding to my raised issue.

Yes I agree that the literal price a user gets is not too important from a backtesting point of view (apart from events like arbitrage and MEV).

The main issue was that I was not able to sync the full state of the Pool from the Event Logs on chain, I want to be able to do this to verify this code is working correctly before I allow automated transactions from this. (This is not a criticism, trust but verify 👍 ).

I think your approach of validating first using the Transaction call data through the full data set is a good one, and would serve to ensure that the code replicates exactly the UniswapV3Pool behavior, then we can confirm we can 'fast-forward' through this with the Event Logs, and then fork or snapshot as you say, the Pool state to make alterations to pool state in time.

I'm happy to contribute any bugs I can find, however TypeScript isn't my preferred environment (I'm a .NET person really, hence my scaffold code in this issue and not using your TS test setup) and so I was using uniswap-v3-simulator to validate my own simulation of the Pool, I get similar errors, and so thought to fix uniswap-v3-simulator with a team already experienced and use this to validate my dotnet version.

I was attempting to set up an EVM environment to debug the UniswapV3Pool directly to verify, but maybe pulling Tx calldata would be simpler as an initial goto. I've not done very much work with EVM, although I do have significant Blockchain experience from BTC.

[jamescarter-le]

Also, I was on the dev branch, as linked to on my previous 'help wanted' issue, can you recommend me the best branch?

[jamescarter-le]

I did manage to get the UniswapV3Pool deployed to test, and the 'bug' is also present there, so I think your suggestion of #53 is probably the best option unfortunately. When we pass the PriceLimit into the Pool, it will return a different value (in some circumstances) than if you do not, even though the PriceLimit is the same as that returned by Uniswap itself.

I've created an issue on v3 core for their feedback: Uniswap/v3-core#501

[kafeikui]

Also, I was on the dev branch, as linked to on my previous 'help wanted' issue, can you recommend me the best branch?

Just use main branch. We have merged dev into main.

[kafeikui]

I see, I'm not very good at .NET but I think @neptune-v is an expert ha-ha. We will pay attention to the issue on v3 core. Anything else about Uniswap v3 guess we can talk about it :D