sidestream-tech / unified-auctions-ui

Unified MakerDAO auctions
https://unified-auctions.makerdao.com
GNU Affero General Public License v3.0
16 stars 13 forks source link

Extend Unified Auctions UI with 1inch callee #580

Closed LukSteib closed 1 year ago

LukSteib commented 1 year ago

Goal

Select 1inch callee via UI to participate in auctions

Context

Recently we have implemented a 1inch base exchange callee contract. (see https://github.com/makerdao/exchange-callees/pull/34/files) We now want to move on and integrate it to the UI. For that we might need to refactor the core.

Tasks

KirillDogadin-std commented 1 year ago

Modules:

  1. one-inch request handling: (analogous to the script in the PR)
    • sends the requests to the one_inch api
    • gets 1-inch swap parameters
  2. callee module:
    • repackages the 1-i api data for the smart contract function call (analogous to the script in the PR)
    • calls the contract function (callee)
      • getCalleeData
        1. verify callee name from market data
        2. get results of 1inch api call from market data
        3. repackage the data from the 1inch api if needed
        4. build function call arguments.
        5. call the contract function
        6. part of the functionality to implement the above process should be extracted into the corresponding helper file calleeFunctions/helpers/oneinch.ts
      • getMarketPrice
        1. call to the quote route
        2. get the estimated gas price, get the route of the trade, amounts of sold and received tokens.
        3. perform the necessary conversions.
        4. store the final calldata in the auction object
  3. Collaterals config
    • adjust COLLATERALS.ts: corresponding tokens' should include the market data for the 1inch callee. Make the name of the callee something like OneInchCallee and enable automaticRouter
valiafetisov commented 1 year ago

Pre-requirements:

Other points:

KirillDogadin-std commented 1 year ago

or did you had another approach in mind?

nope, as I was asked by @LukSteib to estimate the changes to the core, i left the deployment out of the post's scope.

by analogy of the ...

updated the comment

The name of the callee ...

updated the comment.

KirillDogadin-std commented 1 year ago

regarding gas price estimation:

please confim if the process is correct:

  1. Grab bytecode from out/UniswapV3SplitRouteCallee.sol/UniswapV3SplitCallee.json (corresponding json field)
  2. put the bytecode into the separate file
  3. use cast estimate --create $(cat bc.txt) "UniswapV3SplitCallee(address,address)" 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45 0x9759A6Ac90977b93B58547b4A71c78317f391A28 -- --rpc-url $RPC_URL command according to the test added here
  4. receive gwei price

currently i'm experiencing the following problem:

do you have the idea of what is done incorrectly?

valiafetisov commented 1 year ago

please confim if the process is correct:

I sadly don't know the process, but think it should be easy to figure out. You obviously don't have to use foundry for estimating, you can use any other tools (remix IDE, tenderly, hardhat, etc)

KirillDogadin-std commented 1 year ago

i've ran ethers.js script

import { ethers } from "ethers";
import abi from "./out/UniswapV3SplitRouteCallee.sol/UniswapV3SplitCallee.json";

const factory = ethers.ContractFactory.fromSolidity(abi)
const deploymentData = factory.interface.encodeDeploy(['0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45', '0x9759A6Ac90977b93B58547b4A71c78317f391A28'])
const provider = new ethers.providers.JsonRpcProvider(RPC_URL)
provider.estimateGas({ data: deploymentData }).then(gas => console.log(gas))

and got 53736

KirillDogadin-std commented 1 year ago

1inch integration thing arose:

valiafetisov commented 1 year ago

Ok, to be clear, the proposal is to replace pools with something simpler like:

{
    path: 'LINK -> ETH -> DAI', // this can be directly shown in the frontend
    calleeData: '0x...', // output of the `getCalleeData`
    exchangeFeeEth: new BigNumber(0.1),
    exchangeFeeDai: new BigNumber(100),
}

which for the oneinch migh look like

{
    path: 'Uniswap V3 -> Uniswap V2', // using response.protocols or whatever is returned from the response
    calleeData: '0x...',
    exchangeFeeEth: new BigNumber(0.1), // using response.tx.gasPrice
    exchangeFeeDai: new BigNumber(100), 
}

When the auction is executed, calleeData is being taken from auction.marketRecords[marketId]...calleeData and NOT generated again.

Do you see any problem with this proposal?

should i substitute it with smth like undefined or 3000?

If the question was about collateral config, then 1inch callee should have it's own type which doesn't require fee to be defined

KirillDogadin-std commented 1 year ago

If we're intending to use the public api, i would suggest implementing the funcitonality that prevents calling external data providers (oneinch api and multirouter) several times in a row. (e.g. only on button hit the update happens.)

{"error":"Rate limit exceeded. Contact o.kudinov@1inch.io for a dedicated enterprise endpoint"}
LukSteib commented 1 year ago

{"error":"Rate limit exceeded. Contact o.kudinov@1inch.io for a dedicated enterprise endpoint"}

If asking for enterprise API price quote would be good to tell what we are expecting: What'd be an acceptable rate limit for us? Any other requirements that we'd have?

KirillDogadin-std commented 1 year ago

reate limit

assuming we're at 100 users simultaniously, there're 20 auctions open, and we're updating every second: at least 2000 requests per second (depending on the implementation can be more)

other requirements

as of now, not aware of those.


alternative:

publicly accessible url limits the rate based on the IP address i would imagine. so having the response cached for e.g. 60 seconds would likely be enough ( https://help.1inch.io/en/articles/5998758-1inch-api-troubleshooting hints that the rate limit is scoped to the overall public. E.g. if the rate limit is 10 req/sec then it's either 1 person that can sends 10 req/sec or 2 ppl who each send 5 req/sec)

LukSteib commented 1 year ago

assuming we're at 100 users simultaniously, there're 20 auctions open, and we're updating every second: at least 2000 requests per second (depending on the implementation can be more)

Reached out to mail address mentioned in error message above asking for enterprise API price points.

valiafetisov commented 1 year ago

2000 requests per second

I guess you meant per minute? We don't need to send request every second, only every 30 seconds as it is right now done for all other exchange platforms.

But it would also depends on how the count the quota. If it's per origin, then we need 4000 requests per minute for 100 users and 20 auctions. If it's per ip address, then it's 40 requests per minute. In any case, we can just add a tiny proxy in front of it, to make it per ip address and probably never need enterprise plan

KirillDogadin-std commented 1 year ago

I guess you meant per minute? We don't need to send request every second, only every 30 seconds as it is right now done for all other exchange platforms.

as per https://github.com/sidestream-tech/unified-auctions-ui/blob/4cb58a4f18ed2f53c3cc0a754050c1d77348c508/frontend/store/auctions.ts#L21 and my experience logging things: it is exeuted every second

valiafetisov commented 1 year ago

https://github.com/sidestream-tech/unified-auctions-ui/blob/4cb58a4f18ed2f53c3cc0a754050c1d77348c508/frontend/store/auctions.ts#L21

This particular line ensures that refresh of the calculated auction values is done once per second. Enriching the same auction should trigger the cache implemented inside the core. This was pointed out multiply times over multiply PRs: literally no requests should be send to the blockchain (and now to external APIs) every second. Currently the cache suppose to be 10 seconds. But we can increase is to 29

https://github.com/sidestream-tech/unified-auctions-ui/blob/4cb58a4f18ed2f53c3cc0a754050c1d77348c508/core/src/calleeFunctions/index.ts#L21

and my experience logging things: it is exeuted every second

Please ensure via logging that no requests are send more often than every ~30 seconds. If it's not the case right now, please open a PR to fix that.

KirillDogadin-std commented 1 year ago

https://github.com/sidestream-tech/unified-auctions-ui/blob/4cb58a4f18ed2f53c3cc0a754050c1d77348c508/frontend/store/auctions.ts#L354

this is not cached and subcalls a bunch of functionality that queries the blockchain (and api)

If it's not the case right now, please open a PR to fix that.

https://github.com/sidestream-tech/unified-auctions-ui/pull/588

KirillDogadin-std commented 1 year ago

https://github.com/sidestream-tech/unified-auctions-ui/issues/580#issuecomment-1424184354

the best answer i found is still contained in the comment:

https://github.com/sidestream-tech/unified-auctions-ui/issues/580#issuecomment-1424141671

which is in my understanding "treating all the incoming requests to public api as coming from the single user"

LukSteib commented 1 year ago

Can you please re-iterate on the excat question(s) I should forward to the 1inch customer support? Not able to derive the missing puzzle piece from the linked comments above.

KirillDogadin-std commented 1 year ago

essentially 2 questions are outgoing:

Edit:

this is directed to the questions about public api

LukSteib commented 1 year ago

Based on feedback from 1inch customer support:

What is the request rate limit?

2 Requests per second

Is it limiting per origin or per IP?

It is limiting per IP

valiafetisov commented 1 year ago

2 Requests per second

Is this a paid or unpaid tier?

2 requests per second, should be fine for us. We can use a queue-like structure to never send requests in parallel. This will allow us to fetch ~50 auctions per 30 seconds (our current refresh delay).

It is limiting per IP

Great, so we don't need to introduce caching proxy

LukSteib commented 1 year ago

Is this a paid or unpaid tier?

This refers to the publicly available API (so unpaid tier)