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

Extend multi-callee with auto router #527

Closed
5 of 11 tasks
Tracked by #522 ...
valiafetisov opened this issue Nov 7, 2022 · 13 comments · Fixed by #531
Closed
5 of 11 tasks
Tracked by #522 ...

Extend multi-callee with auto router #527

valiafetisov opened this issue Nov 7, 2022 · 13 comments · Fixed by #531

Comments

@valiafetisov
Copy link
Contributor

valiafetisov commented Nov 7, 2022

Goal

Automatic router is used for tokens exchangeable via UniswapV3Callee as a separate exchange config.

Context

After

We're finally ready to add full support of the uniswap automatic router. The idea is to add them as a new separate exchange config, eg:

  exchanges: {
+     'Uniswap V3 auto router': {
+         callee: 'UniswapV3Callee',
+         automaticRouter: true,
+     },
      'Uniswap V3': {
          callee: 'UniswapV3Callee',
          route: ['ETH'],
      },
      'Uniswap V2': {
          callee: 'UniswapV2CalleeDai',
          route: ['ETH'],
      },
  },

And then get quote and the route in the getMarketPrice (or some higher level function). Then, route will become part of the marketData object inside marketDataRecords['Uniswap V3 auto router'] during the auction enrichment and will be picked up from there by the getCalleeData during auction execution.

Note: we currently store routes as just a list of intermediate collateralSymbols through which the exchange should be made eg:

const route = [ 'ETH' ]

While the callees accept array of token addresses + pools, eg:

const path = [
  '0x7Fc66500c84A76Ad7e9c93437bFc5Ac33E2DDaE9', // AAVE address
  3000, // AAVE+WETH uniswap pool
  '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2', // WETH address
  3000, // WETH+DAI uniswap pool
  '0x6B175474E89094C44Da98b954EedeAC495271d0F' // DAI address
]

While we might want to store this data in this format:

const pools = [
  {
    names: ['AAVE', 'WETH'],
    addresses: ['0x7Fc66500c84A76Ad7e9c93437bFc5Ac33E2DDaE9', '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'],
    fee: 3000,
  },
  {
    names: ['WETH', 'DAI'],
    addresses: ['0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2', '0x6B175474E89094C44Da98b954EedeAC495271d0F'],
    fee: 3000,
  }
]

Therefore, we should:

  • Keep exchange[marketId].route config as now
  • Transform route format into pools format during enrichment and store it under marketDataRecords[marketId].pools instead of route
  • We should expect that intermediate tokens might be tokens that are not part of the collateral config
    • So that we need to store their address in the marketDataRecords[marketId].pools
    • So that we might need to fetch the symbol name from the chain (if it's not provided with the route)

Assets

  • getUniswapAutoRoute
    export const getUniswapAutoRoute = async function (
    network: string,
    collateralSymbol: string,
    inputAmount: string | number = 1,
    walletAddress?: string
    ) {
    const collateralConfig = getCollateralConfigBySymbol(collateralSymbol);
    const provider = await getProvider(network);
    const router = new AlphaRouter({ chainId: 1, provider });
    const inputToken = await getUniswapTokenBySymbol(network, collateralConfig.symbol);
    const outputToken = await getUniswapTokenBySymbol(network, 'DAI');
    const inputAmountInteger = new BigNumber(inputAmount).shiftedBy(collateralConfig.decimals).toFixed(0);
    const inputAmountWithCurrency = CurrencyAmount.fromRawAmount(inputToken, inputAmountInteger);
    // get auto route
    const route = await router.route(
    inputAmountWithCurrency,
    outputToken,
    TradeType.EXACT_INPUT,
    {
    recipient: walletAddress || '0x000000000000000000000000000000000000dEaD', // use given address or "dead" address as fallback
    slippageTolerance: new Percent(10, 100),
    deadline: Math.floor(Date.now() / 1000 + 1800),
    },
    {
    maxSplits: 0,
    }
    );
    if (!route) {
    throw new Error(`Could not get auto route for collateral "${collateralSymbol}".`);
    }
    return route;
    };

Implementation proposal

  • Refactor route->pools:
    • Generate a new pools value during enrichment instead of route
    • Use pools on the frontend
    • Use pools value during execution (ie create encodePools instead of using encodeRoute)
    • Check that old routes still working
    • (optional) Merge enrichAuctionWithMarketDataRecords and enrichMarketDataRecordsWithValues into one function, but without a loop getMarketDataById which returns both fetched values, calculated values and pools
  • Add new automaticRouter markets to the collateral config
  • Handle new automaticRouter type when the callee is UniswapV3Callee via calling getUniswapAutoRoute
    • Transform output of the getUniswapAutoRoute into the pools format
    • Include estimated exchange fees into marketUnitPrice via using route.quoteGasAdjusted
    • Do not reapply hardcoded fees when automaticRouter is present
@KirillDogadin-std
Copy link
Contributor

is this necessary to specify in the config?:

automaticRouter: true

i fail to see the use case for the false scenario. What should happen then? or should the Uniswap V3 auto router exchange be present for every collateral config with various flags (automatic router is set to true or false but always present)?

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Nov 8, 2022

Generate a new pools value during enrichment instead of route

are you referring to the route variable here that is generated in the getCalleeData function family?

i'm failing to pinpoint where exactly routes' generation takes place in enrichAuctionWithMarketDataRecords process

@valiafetisov
Copy link
Contributor Author

i fail to see the use case for the false scenario. What should happen then? or should the Uniswap V3 auto router exchange be present for every collateral config with various flags (automatic router is set to true or false but always present)?

What's your proposal? I think the types should enforce that either route: [] is present or automaticRouter in order for the configuration to be explicit (vs forgetting to set route: [] for the automatic router to kick in)

are you referring to the route variable here that is generated in the getCalleeData function family?

No. Currently the exchanges[marketId].route is passed directly into auction.marketDataRecords[marketId].route, and then used in the frontend to display routing. Instead, the proposal is to replace it with pools so it can be both generated from the static route: [ 'ETH' ] from the config and be provided by the auto router.

i'm failing to pinpoint where exactly routes' generation takes place in enrichAuctionWithMarketDataRecords process

Currently there is no generation, route: [ 'ETH' ] is passed directly and used inside getCalleeData. Instead, in order to make it compatible with the automatic router (which output can't be compressed into the same format), I suggest to convert route into pools which will be compatible with both callee format (where we need token addresses) and frontend format (where we need symbols)

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Nov 9, 2022

What's your proposal? I think the types should enforce that either route: [] is present or automaticRouter in order for the configuration to be explicit (vs forgetting to set route: [] for the automatic router to kick in)

the part after the question mark makes sense. The question I raised is rather to discover some additional app-logic/story that i am not aware of, not in order to adjust the datastructure.

No. Currently the exchanges[marketId].route is passed directly into auction.marketDataRecords[marketId].route, and then used in the frontend to display routing. Instead, the proposal is to replace it with pools so it can be both generated from the static route: [ 'ETH' ] from the config and be provided by the auto router.

👍🏻

Currently there is no generation, route: [ 'ETH' ] is passed directly and used inside getCalleeData. Instead, in order to make it compatible with the automatic router (which output can't be compressed into the same format), I suggest to convert route into pools which will be compatible with both callee format (where we need token addresses) and frontend format (where we need symbols)

👍🏻


one more q:

  • i checked-out main and while going through the code i've noticed that my LSP marks some places with ts errors.
  • i checked with npm run build and i also get errors there
error

src/calleeFunctions/helpers/uniswapV2.ts:23:9 - error TS2322: Type 'RegularCalleeConfig | UniswapAutoRouterConfig' is not assignable to type 'RegularCalleeConfig'.
  Property 'route' is missing in type 'UniswapAutoRouterConfig' but required in type 'RegularCalleeConfig'.

23         return marketData;
           ~~~~~~~~~~~~~~~~~~

  src/types.ts:88:5
    88     route: string[];
           ~~~~~
    'route' is declared here.

src/calleeFunctions/helpers/uniswapV3.ts:47:74 - error TS2339: Property 'route' does not exist on type 'RegularCalleeConfig | UniswapAutoRouterConfig'.
  Property 'route' does not exist on type 'UniswapAutoRouterConfig'.

47     const route = encodeRoute(network, [collateral.symbol, ...marketData.route]);
                                                                            ~~~~~

src/calleeFunctions/UniswapV3Callee.ts:19:89 - error TS2339: Property 'route' does not exist on type 'RegularCalleeConfig | UniswapAutoRouterConfig'.
  Property 'route' does not exist on type 'UniswapAutoRouterConfig'.

19     const uniswapV3route = await encodeRoute(network, [collateral.symbol, ...marketData.route]);
                                                                                           ~~~~~

src/calleeFunctions/index.ts:58:9 - error TS2322: Type '{ marketUnitPrice: BigNumber; errorMessage: any; callee: "UniswapV2CalleeDai" | "WstETHCurveUniv3Callee" | "CurveLpTokenUniv3Callee" | "UniswapV3Callee" | "rETHCurveUniv3Callee"; route: string[]; } | { ...; } | { ...; }' is not assignable to type 'MarketData'.
  Type '{ marketUnitPrice: BigNumber; errorMessage: any; callee: "UniswapV3Callee"; automaticRouter: boolean; }' is not assignable to type 'MarketData'.
    Type '{ marketUnitPrice: BigNumber; errorMessage: any; callee: "UniswapV3Callee"; automaticRouter: boolean; }' is missing the following properties from type 'MarketDataUniswapV2LpToken': token0, token1

 58         return {
            ~~~~~~~~
 59             ...calleeConfig,
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
 61             errorMessage,
    ~~~~~~~~~~~~~~~~~~~~~~~~~
 62         };
    ~~~~~~~~~~

src/calleeFunctions/index.ts:64:5 - error TS2322: Type '{ marketUnitPrice: BigNumber; callee: "UniswapV2CalleeDai" | "WstETHCurveUniv3Callee" | "CurveLpTokenUniv3Callee" | "UniswapV3Callee" | "rETHCurveUniv3Callee"; route: string[]; } | { ...; } | { ...; }' is not assignable to type 'MarketData'.
  Type '{ marketUnitPrice: BigNumber; callee: "UniswapV3Callee"; automaticRouter: boolean; }' is not assignable to type 'MarketData'.
    Type '{ marketUnitPrice: BigNumber; callee: "UniswapV3Callee"; automaticRouter: boolean; }' is missing the following properties from type 'MarketDataUniswapV2LpToken': token0, token1

64     return {
       ~~~~~~~~
65         ...calleeConfig,
   ~~~~~~~~~~~~~~~~~~~~~~~~
66         marketUnitPrice: marketUnitPrice ? marketUnitPrice : new BigNumber(NaN),
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
67     };
   ~~~~~~

intentionally merged like this? does scope of this pr include fixing those? or is my setup somehow deviant?

@aomafarag
Copy link
Contributor

one more q:

* i checked-out `main` and while going through the code i've noticed that my LSP marks some places with ts errors.

* i checked with `npm run build` and i also get errors there

error

intentionally merged like this? does scope of this pr include fixing those? or is my setup somehow deviant?

I checked out main and ran npm run build in core/ and didn't get any errors. Can you show me how you implemented the UniswapAutoRouterConfig type mentioned in the error?

@KirillDogadin-std
Copy link
Contributor

ok, sorry for the hussle for no reason:

my shell aliases went out of control (typo) and git-stash-pop got executed unintentionally.

@KirillDogadin-std
Copy link
Contributor

@valiafetisov,

just to be clear, the change introduced via this issue should not alter the logic of UniswapV2LpTokenCalleeConfig processing, right? That is, the enrichment process should be smart enough to distinguish presence of the route in the config, map it into pools and then use pools later on - is that right?

should routes of the non-automatic callees be mapped into pools as well?

@valiafetisov
Copy link
Contributor Author

just to be clear, the change introduced via this issue should not alter the logic of UniswapV2LpTokenCalleeConfig processing, right?

Hard to say, but they are related in a way. UniswapV2LpTokenCalleeConfig underneath is actually a pair of two tokens that is exchanged separately, so when we calculate price of the UniswapV2LpTokenCalleeConfig token we need get quote for each token. And the price determination of each token will be changed in this PR. To summarise: technically UniswapV2LpTokenCalleeConfig is dependent on the newly introduced logic, but I don't know if it will also need a refactoring because of the proposed changes or not.

Maybe in order to avoid broader changes, instead of replacing route with pools, we should keep route and add pools (but without names in it)?

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Nov 9, 2022

Ok, works.

Next question:

after looking at how to approach this cleanly i've encountered the problem with typing and i am trying to avoid the mess.

  • previously the contents of the callee config were only relevant during the processing step in getCalleeData. during enrichment of marketData this structure / type was not dealt with.
  • now according to the tech proposal in the issue description we need to address the structure of the callee config on the higher level. This implies, to my understanding, that we have to check for presence of the previously discussed boolean flag.
  • sadly, typescript will raise errors when i'm accessing this property because it is not present in any other callee config.
  • question - which path should i take?:
    • come up with some very very smart way of defining types and accessing them (dealing with their structure) in appropriate context?
    • define a common field for each of the callee configs. E.g. type, and compare it via if type === 'someAutoRouter' to invoke necessary actions on the high level?

@valiafetisov
Copy link
Contributor Author

Not sure what exactly the problem is, but, we're doing similar thing (like accessing automaticRouter) in other places. Here you can find how we access route key which only exist on CurveLpTokenUniv3Callee

const marketData = collateral.exchanges[marketId];
if (marketData?.callee !== 'CurveLpTokenUniv3Callee') {
throw new Error(`Can not encode route for the "${collateral.ilk}"`);
}
const route = await encodeRoute(network, marketData.route);

So to my understanding all you need to do is:

  1. Define new RoutableCalleeConfig type just like RegularCalleeConfig but with UniswapV3Callee only
  2. Check that calleeConfig.callee is UniswapV3Callee (note, you might need to do const marketData = collateral.exchanges[marketId]; trick from above – first creating a const due to ts bug discovered in the previous PR)
  3. Access calleeConfig.autoRouter

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Nov 9, 2022

Ok, such option is clear but isn't it somehow undesired due to us limiting the expected set of callees? That is, if we would like to add another callee that also supports auto routing, then we would have to not only adjust the collateral config (add the callee to relevant collaterals), but also adjust code so that the if conditions allow the code to execute?

@valiafetisov
Copy link
Contributor Author

but also adjust code so that the if conditions allow the code to execute?

If you don't need config to enforce callee name, then you can create RoutableCalleeConfig as

export declare interface RoutableCalleeConfig {
    callee: RegularCalleeConfig['callee']
    autoRouter: boolean;
}

And it will still enforce the same thing, but without checking the callee name? You can also use in keyword to check if a key is present to make it typesafe (ie if ('autoRouter' in calleeConfig) instead of if (calleeConfig.autoRouter))

@KirillDogadin-std
Copy link
Contributor

Since I don't find myself being sufficiently familiar with the uniswap exchanges i would like to do an intermidiate iteration with the code in order to determine wether the direction that the solution moves into is correct from the perspective of the product-logic. Please see #531

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 a pull request may close this issue.

3 participants