feat(ProtoRev): Supporting Two Pool Routes

[davidterpay]

What is the purpose of the change

This PR updates the route building logic of ProtoRev to be able to construct two pool arbitrage routes. This is particularly useful when there are multiple pools of that consist of the same assets and are imbalanced. As before, routes will only be constructed using the base denoms.

Testing and Verifying

• Added unit test that test the construction of two pool routes under various conditions
• Updated existing test cases that should also now create two pool routes

Documentation and Release Note

This PR is a part of the open issue for ProtoRev updates for V17 (#5858)

[NotJeremyLiu]

Can we add in the comment what test these pools are for

[NotJeremyLiu]

lgtm after supporting both directions

[NotJeremyLiu]

lgtm

[stackman27]

just have a question to understand this logic more before approving

[stackman27]

nit: or SwapInDenom, SwapOutDenom

Suggested change

	swapIn, swapOut string,
	tokenInDenom, tokenOutDenom string,

[stackman27]

i'm trying to understand this: if SwapDenom == tokenOutDenom & huge swap occours in highest liquidity pool, isn't base denom being cheaper so you would create a route to balance it?

[davidterpay]

just to make sure i understand the scenario:

if BaseDenom == tokenOutDenom, base denom becomes more expensive as the token supply of baseDenom in that pool decreases. i.e. if x == tokenInDenom and y == tokenOutDenom, then x + deltaX / y - deltaY > x / y. in this case, tokenOut is y which means it is more expensive and hence we can get more of X on the current pool than the other pool. We should then find the highest liquidity pool with the same assets and route through that. if the swap occurred on the highest liquidity pool, then we cannot make a route.

[czarcas7ic]

@NotJeremyLiu @davidterpay getting the same protorev e2e failure after two runs. Any way you can take a look into this?

[davidterpay]

@NotJeremyLiu @davidterpay getting the same protorev e2e failure after two runs. Any way you can take a look into this?

It looks like its failing a CL test swap. I'll see if I can replicate the same issue and resolve it locally. Will ping once I find the issue (likely tmw morning EST).

[ValarDragon]

devbot merge base

[davidterpay]

We think the issue may be arising because of the two pool routing as the swap in the CL test does create some arbitrage that the module is confirmed to capture. This might be messing with the calculations done in the test case.

There are a few approaches for this:

1. We can disable protorev for the test case (but im unsure if this will affect the other protorev test since it is executed in parallel).
2. We can determine if the calculations for sqrt amount can be updated in accordance with any liquidity changes that protorev introduces.
3. We can switch out the denoms used in the test case for other denoms so that we do not have to disable protorev.

[davidterpay]

osmosis/tests/e2e/e2e_test.go

Lines 330 to 332 in 92e2979

	// Change the parameter to disable protorev
	err = chainABNode.ParamChangeProposal("protorev", string(protorevtypes.ParamStoreKeyEnableModule), []byte("false"), chainAB)
	s.Require().NoError(err)

Added this for the time being.

[czarcas7ic]

Do you think we can instead remove the route from protorev in the protorev test itself? It doesn't seem scalable if we have another test that relies on calculating the output of the pools to check if protorev is disabled. Also, since everything is concurrent, there could be a scenario in which

1. Protorev test starts
2. CL test starts as well
3. CL test diables protorev
4. Protorev test fails due to being disabled via the CL test

[davidterpay]

@czarcas7ic set the max pool points such that it will always pass on the ProtoRev test and will never execute a backrun in the CL test. Should always pass now even if the tests execute in parallel.