patch: Improve autorouter #561

KirillDogadin-std · 2022-12-07T09:33:25Z

Closes #558

Checklist:

issue number linked above after pound (#)
- replace "Closes " with "Contributes to" or other if this PR does not close the issue
issue checkboxes are all addressed
manually checked my feature / not applicable
wrote tests / not applicable
attached screenshots / not applicable

core/src/calleeFunctions/helpers/uniswapAutoRouter.ts

KirillDogadin-std · 2022-12-07T09:39:54Z

core/src/calleeFunctions/UniswapV3Callee.ts

+    collateralAmount: BigNumber,
+    preloadedPools?: Pool[]
 ): Promise<BigNumber> {
-    // convert collateral into DAI
-    const { route, fees } = await getRouteAndGasQuote(network, collateral.symbol, collateralAmount, marketId);
-    if (!route) {
-        throw new Error(`No route found for ${collateral.symbol} to DAI`);
+    if (!preloadedPools) {
+        throw new Error(`pools required to get market price for callee type "${marketId}"`);


this intends to address #531 (comment)

Yes, this is possible solution, but it doesn't completely follow the proposal. Maybe I should ask like that:

How do we get pools for the getMarketPrice in case of the auto router?

We first need to call fetchAutoRouteInformation in the parent of the getMarketPrice

Then we need to pass pools here, then to the convertCollateralToDaiUsingPool and then based on the supplied pools it will return us the price

But why do we pass pools to convertCollateralToDaiUsingPool if we already know it's the price (since it's returned by the fetchAutoRouteInformation together with pools)? Or what to do with the cases when we simply want to convert 0.001 ETH fee to DAI and don't have pools? Ie how do you handle this now?

The suggestion then is to always return price with pools together. In other words, change getMarketPrice output type to include pools. And replace/remove "enrichment with pools" with "enrichment with market data"

adjusted code to either use the total price + pools from AlphaRouter (if autorouting worked out) or fallback to the older way (if hardcoded route).

valiafetisov · 2022-12-08T19:45:55Z

core/src/calleeFunctions/UniswapV3Callee.ts

+        return { price: daiAmount.dividedBy(collateralAmount), pools: generatedPools };
+    }
+    if (totalPrice && pools) {
+        return { price: totalPrice.dividedBy(collateralAmount), pools: pools };


The only remaining problem I see now is that totalPrice here is not the adjusted price that takes exchange fees into account (so we can properly compare exchange prices between each other), but the normal one

i observe that the autorouter returns 2 amounts:

/** * The quote for the swap. * For EXACT_IN swaps this will be an amount of token out. * For EXACT_OUT this will be an amount of token in. */ quote: CurrencyAmount; /** * The quote adjusted for the estimated gas used by the swap. * This is computed by estimating the amount of gas used by the swap, converting * this estimate to be in terms of the quote token, and subtracting that from the quote. * i.e. quoteGasAdjusted = quote - estimatedGasUsedQuoteToken */ quoteGasAdjusted: CurrencyAmount;

My understanding of the above does not combine well with your claim.

The documentation claims that quote is the amount of "token out". If the value does not account for exchange fees, then the amount out is different from quote and hence the documentation is not valid.

Did you perhaps mean the gas fees? should i use quoteGasAdjusted value instead of quote?

valiafetisov · 2022-12-08T19:49:19Z

core/src/calleeFunctions/UniswapV3Callee.ts

-    const pools = await routeToPool(network, route, collateral.symbol, fees);
-    const daiAmount = await convertCollateralToDaiUsingPool(
+): Promise<{ price: BigNumber; pools: Pool[] }> {
+    const { route, fees, totalPrice, pools } = await getRouteAndGasQuote(


Nit: I would remove getRouteAndGasQuote whatsoever and move its logic here, inside getMarketPrice. Because it's just adds a very strange layer of abstraction which doesn't add any value, but complicates understanding of what's going on

valiafetisov · 2022-12-08T19:50:14Z

core/src/calleeFunctions/index.ts

-        const { route, fees } = await fetchAutoRouteInformation(network, collateral.symbol, amount.toFixed());
-        if (!route) {
-            throw new Error('No automatic route can be found');
+        const { pools } = await fetchAutoRouteInformation(network, collateral.symbol, amount.toFixed());


Where do we still use getPools if we the proposal is to never use them separately from the price?

Why do we ever send amount as a string?

Where do we still use getPools if we the proposal is to never use them separately from the price?

getCalleeData for UniswapV3 has 2 use cases:

autorouter

the prices and pools are extracted at the same time. No need to perform other actions (getPools is not called)

hardcoded

the route is provided, prices are not extracted with pools (because alpharouter is not called) and we have to do the old fashioned extraction from the hardcoded route. But we also have to unify the format to use pools primarily. So the getPools is called for the sake of converting route to pool.

I will refactor the code a bit so that getPools cannot be called for callee configs where autorouter is enabled.

removed unused arg (amount)

valiafetisov · 2022-12-08T19:53:18Z

core/src/types.ts

@@ -116,7 +116,7 @@ declare interface MarketDataBase extends Partial<ExchangeFees> {
 export declare interface Pool {
    addresses: string[];
    fee: number;
-    routes: string[];
+    routes: (string | undefined)[];


In which case it can be an array of undefined? 😓

uniswap autorouter does not promise to return the symbol of the token. It's optional there.

will throw if the autorouter does not return symbol in pool

LukSteib

Hard for me to review properly

don't have a test case where the auto router uses a route with an unknown token
cannot properly test regeneration of pools

However,

was able to participate in goerli auction via auto router
UI is not broken but experience is quite similar to current state of prod

Hence approving this PR

LukSteib

Live testing this as we currently have auctions on mainnet. Enabling the auto-router on the RENBTC-A auctions results in UI crash (see below)

console error:

auctions.ts?b334:336 Uncaught TypeError: Cannot read properties of undefined (reading 'forEach')
    at Store.updateAuctionsPrices (auctions.ts?b334:336:1)
    at Array.wrappedActionHandler (vuex.esm.js?2f62:851:1)
    at Store.dispatch (vuex.esm.js?2f62:516:1)
    at Store.boundDispatch [as dispatch] (vuex.esm.js?2f62:406:1)
    at local.dispatch (vuex.esm.js?2f62:779:1)
    at eval (auctions.ts?b334:238:1)

Screenshot UI:

KirillDogadin-std · 2022-12-13T15:59:25Z

fixed the error, the auction seems to be already gone from the mainnet. I used fork on hardhat instead.

CI tells that it's not mergable. Idk how so. Any thoughts @valiafetisov ?

valiafetisov · 2022-12-13T16:20:50Z

CI tells that it's not mergable. Idk how so. Any thoughts?

Executing commands on the fresh clone worked for me

user@user ~ % cd /tmp      
user@user /tmp % git clone https://github.com/sidestream-tech/unified-auctions-ui
Cloning into 'unified-auctions-ui'...
remote: Enumerating objects: 8097, done.
remote: Counting objects: 100% (495/495), done.
remote: Compressing objects: 100% (291/291), done.
remote: Total 8097 (delta 313), reused 320 (delta 201), pack-reused 7602
Receiving objects: 100% (8097/8097), 9.99 MiB | 11.54 MiB/s, done.
Resolving deltas: 100% (5913/5913), done.
user@user /tmp % cd unified-auctions-ui             
user@user unified-auctions-ui % git fetch origin refs/pull/561/head
From https://github.com/sidestream-tech/unified-auctions-ui
 * branch            refs/pull/561/head -> FETCH_HEAD
user@user unified-auctions-ui % git merge 4521483a2ba76b1d00d0414ba3b64f6df9cfbc4d
Updating 89db7a2..4521483
Fast-forward
 .../src/calleeFunctions/CurveLpTokenUniv3Callee.ts | 16 ++++--
 core/src/calleeFunctions/UniswapV2CalleeDai.ts     | 14 +++--
 .../calleeFunctions/UniswapV2LpTokenCalleeDai.ts   |  4 +-
 core/src/calleeFunctions/UniswapV3Callee.ts        | 45 ++++++++++------
 core/src/calleeFunctions/WstETHCurveUniv3Callee.ts | 10 ++--
 .../calleeFunctions/helpers/uniswapAutoRouter.ts   | 14 ++++-
 core/src/calleeFunctions/helpers/uniswapV3.ts      | 27 +---------
 core/src/calleeFunctions/index.ts                  | 61 +++++++++-------------
 core/src/calleeFunctions/rETHCurveUniv3Callee.ts   | 16 ++++--
 core/src/types.ts                                  |  7 ++-
 10 files changed, 117 insertions(+), 97 deletions(-)

So the state of the branch doesn't seem to be a problem. ~~@DeusAvalon?~~ – seems to be github outage

LukSteib · 2022-12-14T14:00:01Z

As discussed orally, will document the other error experienced during live test yesterday:

network: mainnet
related auction: https://etherscan.io/tx/0xbbe894d1fcaa1e1e917cb121b7222f0aeaeacbc28f2101afb2402e1c50db18a7
- taken at block height: 16176353
Error experienced when trying to execute swap tx with unsiwap v3 callee being selected
Error message: Transaction error: "execution reverted: Too little received (cannot estimate gas; transaction may fail or may require manual gas limit)"

KirillDogadin-std · 2022-12-14T16:05:21Z

could you reproduce this on fork?

e.g. on block 16176345

i fail to see the same error.

LukSteib · 2022-12-15T07:58:19Z

could you reproduce this on fork?

As discussed verbally, currently there is no chance for me to start simulation with fork at specific block height. Hence created #568 as intermediate issue.

KirillDogadin-std · 2023-01-31T09:49:17Z

As discussed verbally, currently there is no chance for me to start simulation with fork at specific block height. Hence created #568 as intermediate issue.

did #232 ever get completely resolved? might it be the case that the calculation is just still screwed and in some cases the profit is just really not sufficient, but calculated incorrectly?

valiafetisov · 2023-01-31T10:56:28Z

might it be the case that the calculation is just still screwed and in some cases the profit is just really not sufficient, but calculated incorrectly?

Yes, might be the case that profit amount is calculated incorrectly. But this is always about the precise amount and not about it being negative – we did not have internal or external complains about failed transactions (except for one issue which was long time fixed)

KirillDogadin-std · 2023-02-02T13:15:44Z

Yes, might be the case that profit amount is calculated incorrectly. But this is always about the precise amount and not about it being negative – we did not have internal or external complains about failed transactions (except for one issue which was long time fixed)

i have just tested this on main branch. Same result. Therefore i would say that the core issue here is not related to this pr and should be resolved in the different issue's scope.

LukSteib

Tests conducted:

on goerli: able restart auction, enable auto router, execute swap tx with auto router selected ✔️ (ETH-A auction)
via simulation on localhost: able to simulate auction, enable auto router, execute tx with auto router selected ✔️ (ETH-A auction)

Via simulation: For other collateral types like RENBTC-A or LINK-A enabling the auto router via UI resulted in an endless (killed process after 5min) loading state without getting actual prices (no console error thrown).

KirillDogadin-std · 2023-02-07T15:15:46Z

Via simulation: For other collateral types like RENBTC-A or LINK-A enabling the auto router via UI resulted in an endless (killed process after 5min) loading state without getting actual prices (no console error thrown).

this is due to alpha router not being able to load in the data and failing.

currently the ui does not notify that this happened:

we can scope this in the other issue and develop some smart way of showing this
i can hack-patch it here and disable loading if the loading failed

LukSteib · 2023-02-07T15:30:03Z

we can scope this in the other issue and develop some smart way of showing this

Fine for me to scope it out. So for me this results in two follow up issues:

investigate problem with profit calculation
better error handling for alpha router

KirillDogadin-std added 2 commits December 7, 2022 09:57

feat: adjust the pool source to be from the alpharouter return

5b84bf9

adjust uniswap v3 price calculation logic

96fda6e

KirillDogadin-std commented Dec 7, 2022

View reviewed changes

core/src/calleeFunctions/helpers/uniswapAutoRouter.ts Outdated Show resolved Hide resolved

KirillDogadin-std commented Dec 7, 2022

View reviewed changes

KirillDogadin-std added 4 commits December 7, 2022 13:29

revert the changes with market price and pools

548ad21

feat: extract pools along with price

b1c6a70

feat: use preferably totalPrice

f88b1ad

fix: v3 and v3 auto compatability

e4422f9

KirillDogadin-std marked this pull request as ready for review December 7, 2022 14:01

valiafetisov reviewed Dec 8, 2022

View reviewed changes

KirillDogadin-std added 4 commits December 9, 2022 09:39

refactor: adjust getPools function to disallow autorouters

45fec35

refactor: throw if autorouter does not return symbol

163a6bc

reafactor: merge funtions

1aa5d02

fix: use gas adjusted price

7443d7b

LukSteib approved these changes Dec 9, 2022

View reviewed changes

Merge branch 'main' into improve_autorouter

6baad54

LukSteib requested changes Dec 13, 2022

View reviewed changes

fix: dont loop over undefined

39de14d

Merge branch 'main' into improve_autorouter

4521483

valiafetisov mentioned this pull request Jan 30, 2023

Extend Unified Auctions UI with 1inch callee #580

Closed

3 tasks

Merge branch 'main' into improve_autorouter

f7c3550

valiafetisov approved these changes Feb 7, 2023

View reviewed changes

LukSteib requested changes Feb 7, 2023

View reviewed changes

LukSteib approved these changes Feb 7, 2023

View reviewed changes

valiafetisov merged commit 4cb58a4 into main Feb 7, 2023

valiafetisov deleted the improve_autorouter branch February 7, 2023 15:36

This was referenced Feb 7, 2023

Improve auto router feature #586

Open

Investigate profit calculation #232

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patch: Improve autorouter #561

patch: Improve autorouter #561

KirillDogadin-std commented Dec 7, 2022 •

edited by valiafetisov

Loading

KirillDogadin-std Dec 7, 2022

valiafetisov Dec 7, 2022

KirillDogadin-std Dec 7, 2022

valiafetisov Dec 8, 2022

KirillDogadin-std Dec 9, 2022

valiafetisov Dec 8, 2022

valiafetisov Dec 8, 2022

KirillDogadin-std Dec 9, 2022

KirillDogadin-std Dec 9, 2022

valiafetisov Dec 8, 2022

KirillDogadin-std Dec 9, 2022 •

edited

Loading

LukSteib left a comment

LukSteib left a comment

KirillDogadin-std commented Dec 13, 2022 •

edited

Loading

valiafetisov commented Dec 13, 2022 •

edited

Loading

LukSteib commented Dec 14, 2022 •

edited

Loading

KirillDogadin-std commented Dec 14, 2022

LukSteib commented Dec 15, 2022

KirillDogadin-std commented Jan 31, 2023

valiafetisov commented Jan 31, 2023

KirillDogadin-std commented Feb 2, 2023

LukSteib left a comment

KirillDogadin-std commented Feb 7, 2023

LukSteib commented Feb 7, 2023

patch: Improve autorouter #561

patch: Improve autorouter #561

Conversation

KirillDogadin-std commented Dec 7, 2022 • edited by valiafetisov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillDogadin-std Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

LukSteib left a comment

Choose a reason for hiding this comment

LukSteib left a comment

Choose a reason for hiding this comment

KirillDogadin-std commented Dec 13, 2022 • edited Loading

valiafetisov commented Dec 13, 2022 • edited Loading

LukSteib commented Dec 14, 2022 • edited Loading

KirillDogadin-std commented Dec 14, 2022

LukSteib commented Dec 15, 2022

KirillDogadin-std commented Jan 31, 2023

valiafetisov commented Jan 31, 2023

KirillDogadin-std commented Feb 2, 2023

LukSteib left a comment

Choose a reason for hiding this comment

KirillDogadin-std commented Feb 7, 2023

LukSteib commented Feb 7, 2023

KirillDogadin-std commented Dec 7, 2022 •

edited by valiafetisov

Loading

KirillDogadin-std Dec 9, 2022 •

edited

Loading

KirillDogadin-std commented Dec 13, 2022 •

edited

Loading

valiafetisov commented Dec 13, 2022 •

edited

Loading

LukSteib commented Dec 14, 2022 •

edited

Loading