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.

Sign up for GitHub

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

Jump to bottom

Interact directly with the pool in Uniswap connector #112

Closed
fengtality opened this issue May 24, 2023 · 16 comments
Closed

Interact directly with the pool in Uniswap connector #112

fengtality opened this issue May 24, 2023 · 16 comments
Assignees
@VPashkov
Labels
bounty enhancement New feature or request uniswap connector

Comments

@fengtality
Copy link
Contributor

Description

  • Users have reported that the Uniswap connector is slow compared to other connectors as well as other bots, resulting in missed arbitrage opportunities
  • This may be due to the fact that it uses the smart order router to estimate and execute trades, rather than getting a quote and executing a trade
  • Add the ability to use these direct methods rather than the router to the Uniswap connector as an option
  • Afterwards, estimateSellTrade, estimateBuyTrade, and executeTrade should be able to use the quote/trade methodology described in the Uniswap docs, but also the existing router method
  • The following variables to the Uniswap connector config should be added:
    • useRouter (bool): if True, estimation and execution functions use router; if false, use quote and trade functions of SDK;
    • feeTier (string): Matches one of the Uniswap fee tier strings. If useRouter is False, then user has to specify feeTier to find the right pool
  • Add these new variables to the Uniswap default config template. By default, useRouter should be True, and feeTier is "MEDIUM"

Bounty

  • Sponsor: Hummingbot Foundation
  • Bounty Type: Enhancement / Open Source
  • Bounty Amount: 100,000 HBOT (P1)
  • Developer Payment: 100,000 HBOT (100%)
  • Status: Open
@fengtality fengtality added enhancement New feature or request uniswap connector bounty labels May 24, 2023
@fengtality fengtality mentioned this issue May 24, 2023
Closed
@fengtality fengtality moved this to Open in Bounties Board May 24, 2023
@zedquach
Copy link

What is the expected timeline for this ticket?

@fengtality
Copy link
Contributor Author

What is the expected timeline for this ticket?

Do you mean when it'll be delivered? That depends on if/when a community dev picks up this bounty.

@VPashkov
Copy link
Contributor

@fengtality I would like to take a shot at this. I have a few questions, though.

  1. How useRouter and feeTier will be configured? Should I simply provide public getters/setters, add parameters to constructor/getInstance methods, read it from config, or something else?
  2. I will need to store Quoter and Pool factory contract addresses somewhere. Should I extend UniswapConfig? If yes, I will need also to update its schema, templates, etc., right?
  3. At first glance executeTrade, should just work after updating estimateSellTrade and estimateBuyTrade. Do you have something specific in mind that needs to be done with it or do I simply need to make sure it works with the quote/trade methodology?

@zedquach
Copy link

zedquach commented Jun 2, 2023

I'll take this

@fengtality
Copy link
Contributor Author

fengtality commented Jun 2, 2023 •
edited
Loading

@VPashkov Thanks - see below for answers to your questions. If you would like to be assigned this issue, you have first dibs. @zedquach we will let you know if it's open in a few days.

  • I recommend this approach, which should be simplest - if it's not clear, I can provide more guidance.
  • useRouter and feeTier are 2 additional params in UniswapConfig. You can store the Quoter and Pool factory contract addresses here too.
  • In the future, we should provide more flexibility on a request-by-request basis, but this approach should limit scope of this task and not necessitate changes to Hummingbot client
  • Update the Uniswap schema and templates accordingly. You can make all the new params optional.
  • In Uniswap, read these values. Modify estimateSellTrade and estimateBuyTrade so that it gets a quote from the pool directly following the guide if useRouter is False.
  • Object returned should remain the same { trade: route.trade, expectedAmount };
  • Return an error message if useRouter is false and feeTier is not specified or invalid
  • I believe no changes should be needed to executeTrade nor the controllers and validators
  • Add unit tests to check that this new logic works

@VPashkov
Copy link
Contributor

VPashkov commented Jun 2, 2023

@fengtality Yes, I would like to be assigned. I think I got all the answers I need for now. Thanks.

@fengtality fengtality moved this from Open to Assigned in Bounties Board Jun 3, 2023
@VPashkov
Copy link
Contributor

VPashkov commented Jun 6, 2023

A quick status update.
I have a working implementation. It works significantly faster.
Some naive performance testing shows 250-350ms for price requests(/amm/price) for new implementation versus 3.5-7 seconds for the old one. For trade requests(/amm/trade) it's 2-2.5 seconds versus 7-9 seconds.
I'm working on unit tests right now.

@fengtality
Copy link
Contributor Author

fengtality commented Jun 6, 2023 via email

@nikspz
Copy link
Contributor

nikspz commented Jun 14, 2023

hi @VPashkov Could you please add your updates here and submit PR/draft?

@VPashkov
Copy link
Contributor

Hi @nikspz.
Sorry for the delay. I'm struggling with writing tests for the executeTrade method. A lot of mocking is required, but I think it needs to be done to make sure I'm not breaking existing code. I think I will need a couple of more days to finish.

@fengtality
Copy link
Contributor Author

Hey @VPashkov if you're having issues mocking data for tests, I suggest submitting the PR as-is.

We are aware of the challenges testing Gateway functions that require network calls and don't want to make devs spend time unnecessarily

@VPashkov
Copy link
Contributor

Got it. I'm sending PR without tests for executeTrade then.

@VPashkov VPashkov mentioned this issue Jun 22, 2023
Merged
2 tasks
@nikspz nikspz moved this from Assigned to Submitted in Bounties Board Jun 22, 2023
@toxicehc
Copy link

Hi @VPashkov - Would this also work on polygon uniswap?

@fengtality fengtality moved this from Submitted to Merged in Bounties Board Jul 2, 2023
@toxicehc
Copy link

toxicehc commented Jul 4, 2023 via email

@VPashkov
Copy link
Contributor

VPashkov commented Jul 4, 2023

Hey @toxicehc, sorry didn't see your message. I'll check DM on Discord. Thanks.

@github-project-automation github-project-automation bot moved this from Merged to Assigned in Bounties Board Jul 5, 2023
@fengtality fengtality moved this from Assigned to Merged in Bounties Board Jul 5, 2023
@nikspz
Copy link
Contributor

nikspz commented Jul 10, 2023

Thanks for your participation, the bounty has been sent to @VPashkov . The fix has been deployed on development branch and next hummingbot version 1.18.0

@nikspz nikspz moved this from Merged to Paid in Bounties Board Jul 10, 2023
@VPashkov VPashkov mentioned this issue Jul 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@VPashkov VPashkov

Labels
bounty enhancement New feature or request uniswap connector
Projects
Bounties Board
Status: Paid
Milestone
No milestone
Development

No branches or pull requests
5 participants
@VPashkov @fengtality @toxicehc @zedquach @nikspz