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

Conceptualise support of several callees per collateral #374

Closed
2 tasks done
Tracked by #459 ...
LukSteib opened this issue Jul 25, 2022 · 13 comments
Closed
2 tasks done
Tracked by #459 ...

Conceptualise support of several callees per collateral #374

LukSteib opened this issue Jul 25, 2022 · 13 comments
Assignees

Comments

@LukSteib
Copy link
Contributor

LukSteib commented Jul 25, 2022

Goal

Create a concept on how we will be able to support several callees per collateral (in the UI but also with the bot)

Context

Right now we support one callee per collateral type. However we envision a future state where there are several callees that can be used to execute a swap transaction for an active auction. Multi callee support is useful beacause a) it might allow to provide better market prices b) it adds redundancy (e.g. problem if there is only one callee based on 1inch and then 1inch is down)

Tasks

  • Come up with a technical concept to allow several callees for a collateral type
  • Come up with a concept how this can be reflected in the UI

Assets

@valiafetisov
Copy link
Contributor

valiafetisov commented Oct 5, 2022

Initial mock

MakerDAO collateral@2x (1)

The only problem I have with this minimal proposal is that it doesn't reflect that different exchange paths/callees will have different transaction fees. And the algorithm to suggest the best callee would rather need to compare total prices minus transaction fee, not unit prices or profit numbers directly. But it's also a question how well we can estimate transaction fees for arbitrary paths/callees – I think some places (eg automatic router, 1inch callee, even provide this info, but some doesn't)

@LukSteib
Copy link
Contributor Author

LukSteib commented Oct 5, 2022

Feedback on the mock itself:

  • I like this minimal proposal without artificially blowing up the UI
  • What happens to the naming of the collapsible "Best Market Price" if user selects another exchange path (which is no longer the best price)

The only problem I have with this minimal proposal is that it doesn't reflect that different exchange paths/callees will have different transaction fees. And the algorithm to suggest the best callee would rather need to compare total prices minus transaction fee, not unit prices or profit numbers directly.

I agree that the algorithm to determine best callee should rely on total prices minus tx fees. (Btw: Doesn't total prices minus transaction fees yield net profit?)
What about tackling the feature step by step.

V1:

  • UI as shown in your proposal above
  • Base the callee selection on market unit price
  • Better understand how well we are able to estimate tx fees (as we need to display them in the table either way)
  • Have the general setup that allows for multi callee usage

V2:

  • Base the callee selection on total prices minus tx fees
  • Adjust the UI to better reflect this approach

@valiafetisov
Copy link
Contributor

What happens to the naming of the collapsible "Best Market Price"

Good point, let's name it to simply Market Unit Price

Doesn't total prices minus transaction fees yield net profit?

No, as we learned the formula to compute actual profit is more complicated than that.

What about tackling the feature step by step

The problem is that automatic router already takes [exchange] fees into account (otherwise it will provide unrealistic routes with 10+ swaps). If we wouldn't account for the fees in all other callees we would also compare some callees with and some callees without fees. I think the solution is to separate current fee into two parts:

  • exchange fees (will be included into the exchange price, aka Market Unit Price)
  • auction participation fees (will be shown as now)

For the old callees hardcode the exchange fee if we can't get it generically.

MakerDAO collateral@2x (2)

Next step is to outline the technical solution including this fee separation.

@LukSteib
Copy link
Contributor Author

LukSteib commented Oct 5, 2022

Good point, let's name it to simply Market Unit Price

Good one

No, as we learned the formula to compute actual profit is more complicated than that.

True...

I think the solution is to separate current fee into two parts

I like the idea of separating the fees. Thought about the same already.

exchange fees (will be included into the exchange price, aka Market Unit Price)

Just to be clear. This type of fees we wouldn't explicitly show in the UI but include in the figure for market unit prices of the different callees?

@valiafetisov
Copy link
Contributor

This type of fees we wouldn't explicitly show in the UI but include in the figure for market unit prices of the different callees?

Yes, eg Market Unit Price = 297.23 DAl per ETH is the net exchange price, same as getting a total quote for 20.00 ETH, then removing the exchange fees from result and then dividing it by 20 to get unit price.

@LukSteib
Copy link
Contributor Author

LukSteib commented Oct 5, 2022

then removing the exchange fees from result

But wouldn't that lead to the situation of us neglecting the exchange fees in the comparison? In other words: A callee with very high fees could yield a lower (hence apparently more lucrative) net exchange price. However for the user a callee with higher net exchange price but lower fees would be more favourable...
Hope that you can follow my concern.

@valiafetisov
Copy link
Contributor

But wouldn't that lead to the situation of us neglecting the exchange fees in the comparison?

Sorry, I meant adding fees to the total quote. This will result in higher prices for callees that consume more gas. The only problem is to somehow fine-tune those hardcoded gas prices for the old callees.

@valiafetisov
Copy link
Contributor

valiafetisov commented Oct 11, 2022

Technical proposal

Refactor core

  1. Update collateral config exchange key to exchanges
    from
    exchange: {
        callee: 'UniswapV3Callee',
        route: ['ETH'],
    },
    to
    exchanges: {
        "Uniswap v3": {
            callee: 'UniswapV3Callee',
            route: ['ETH'],
        },
        "Uniswap v2": {
            callee: 'UniswapV2CalleeDai',
            route: ['ETH'],
        },
        // "Uniswap v3 automatic route": {
        //    callee: 'UniswapV3Callee',
        //    route: undefined,
        // },
    },
    So basically prepending it with unique id, which will also be displayed on the frontend as title
  2. Create function getMarketData that use exchanges configs and returns
    {
        "Uniswap v3": {
             unitPrice: BigNumber,
             route: string[], // passed from the config in this PR, generated in case of automatic router in another PR
        },
        //...
    }
    Notes:
    - In case it's impossible to fetch specific market, the object should still be there, but unitPrice should be set to new BigNumber(NaN)
    - The main complexity here is that returned unit prices should all include exchange fees in order to be comparable between each other, see discussion above. For this, existing bid transaction fee logic should be reworked: swapTransactionFee should be split into variable part (that depends on the callee) and on constant part (basically the should be much more close to the bidTransactionFee); the variable part will then be hidden in the unitPrice. Technically that means, that while computing unit price (eg here) we should first remove exchange fees (can be hardcoded per callee per route length)
  3. Create getBestMarketData function that sort output of the getMarketData by unitPrice and return the lowest (together with it's id)
    {
         marketId: "Uniswap v3"
         unitPrice: BigNumber,
    }
  4. Modify _getMarketPrice to use getBestMarketData and just output unitPrice
  5. During collateral auction enrichment
    • Keep marketUnitPrice as is (but put the best market price there)
    • Add result of the getMarketData under new marketData key along existing marketUnitPrice
    • Add suggestedMarketId key to the auction type along existing marketUnitPrice
  6. Extend bidWithCallee to accept marketId required argument
    • Generate callee data based on the marketId
  7. Modify store to send suggestedMarketId to the bidWithCallee
  8. Modify bot to send suggestedMarketId to the bidWithCallee

Modify UI elements

  1. Modify Auction type to contain new keys from point 5 above, eg:
    marketUnitPrice: BigNumber
    suggestedMarketId: 'Uniswap v3'
    marketData: {
            "Uniswap v3": {
                 unitPrice: BigNumber,
                 route: string[],
            },
            "Uniswap v2": {
                 unitPrice: BigNumber,
                 route: string[],
            },
        }
    },
  2. Modify fake function to create desired type (note that unitPrice can also be new BigNumber(NaN))
  3. Create MarketPriceSelection component as shown in the mock
    • Use animation as in the BasePanel
    • Expect BigNumber.isNaN() to be true for some markets (so they should display unknown and not be selectable)
    • Use synced prop marketId to emit and receive currently selected marketId
    • Expect that suggestedMarketId might change after the component render –> meaning that
      • in case no selection was made by the user, selected market might change by itself
      • in case a selection was made by the user, it should stay fixed even if suggestedMarketId changes
  4. Send marketId (suggestedMarketId if none selected) to the bidWithCallee action

Final thoughts

We might want to a little think a little bit ahead and consider partial swapping, in order to not completely redo this logic (although it will significantly affect the flow anyway). I think since we split markets on different objects it already prepares the ground on estimating the best size of the part to be taken (based on the available liquidity or multiple "probes")

@LukSteib
Copy link
Contributor Author

LukSteib commented Oct 12, 2022

Thanks for the detailed proposal, really like it!

I think since we split markets on different objects it already prepares the ground on estimating the best size of the part to be taken

Can you elaborate on that point in the light of future partial liquidation? Is it that we are covering ground to decide in the future what's the best amount to swap on a certain market (aka via a certain callee)?


Moving forward do you have a proposal on how to break the proposal down into issues. I expect that not each of the high level steps needs to be an issue on its own.
On the core side I would bundle the following points into one issue:

  • 1, 2
  • 3 - 8

On the UI side I would bundle the following points into one issue:

  • 1, 2
  • 3, 4

@KirillDogadin-std
Copy link
Contributor

We might want to a little think a little bit ahead and consider partial liquidation,

what exactly stands behind the word "liquidation" here? The process of converting the underwater vault into the auction or purchasing just a part of the available collateral from the auction?

@valiafetisov
Copy link
Contributor

what exactly stands behind the word "liquidation" here?

Sorry confuse two terms, I meant partial swapping. Clipper contract allow users to bid on the collateral auctions "partially" based on the user desire (and some contract-based limitations). Currently we only allow this in Bidding with DAI flow, but in the future planning to support in the Directly swap into profit flow as well. See more explanation below

Can you elaborate on that point in the light of future partial liquidation? Is it that we are covering ground to decide in the future what's the best amount to swap on a certain market (aka via a certain callee)?

I don't want to plan both features at the same time, keeping the scope limited, but as they kind of interconnected, we have to account for the future. From my understanding, proposed changes doesn't contradict future partial swapping logic. I'm just raising it here, so you can think about potential edge cases as well. The main complexity of the partial swapping is that market price might be significantly different depending on the amount we're trying to swap, different for each callee. Eg if you try to swap 2000 ETH your unit price is X DAI, if you try to swap 10 ETH with the same callee, your unit price is X/2 DAI. I imagine we will end up with something like bestProportion for each callee + rerun getMarketData if the user enters proportion by hand.

On the core side I would bundle the following points into one issue:

Since it's described as a refactoring, I think it makes sense to do all core changes at once, then merge. For the same reason, ideally, the UI would need core functionality already in place to be merged (but can be started beforehand and also completed in one PR). Splitting it would create more additional work (eg creating temporary copies of types and fake functions instead of modifying existing ones, then cleaning it up). So my suggestion is to create two issues 🙂

@KirillDogadin-std
Copy link
Contributor

ok, proposal seems good

@LukSteib
Copy link
Contributor Author

I imagine we will end up with something like bestProportion for each callee + rerun getMarketData if the user enters proportion by hand.

Makes sense. And agree that it's valuable to have these considerations as back thoughts already.

So my suggestion is to create two issues

Fine for me. Will proceed and create these based on your detailed outline.

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

No branches or pull requests

3 participants