-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Feat/pool id in amm arb #7018
Feat/pool id in amm arb #7018
Conversation
This reverts commit 83c927d.
…into development
…into development
…into development
…into development
…into development
…into development
…into development
@vic-en Should it be optional? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below. I think users should be able to enter either trading pair or poolID, and the poolID is used if it is present.
"pool_id": ConfigVar( | ||
key="pool_id", | ||
prompt="Specify poolId to interract with on the DEX connector >>> ", | ||
prompt_on_new=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to prompt_on_new=False
I think users should not need to enter a poolID. However, if they do enter a poolID, it should be used instead of the trading pair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fengtality @nikspz This is done
thanks @vic-en @nikspz please test with Balancer and Uniswap on Arbitrum: Expected behavior:
If these cases pass, this PR should be ready to merge |
@fengtality Uniswap on Gateway wouldn't acknowledge the poolId. Only Balancer will ack it. Core files for all other connectors will need to modified to make use of poolId when it's supplied. Otherwise, they'll always default to using their smart routers. |
That being said, I can update Uniswap to acknowledge the poolId when it's present in my Balancer pr on Gateway this weekend. |
@fengtality Let me know if you want me to deprecate the useRouter on Uniswap. It's almost a duplicate feature |
@nikspz @fengtality |
thanks for adding I'll review the |
Test performed:
|
@nikspz Seems only sells failed. I'll get back soon. |
@nikspz Balancer pool ids are the longer addresses. That is why they worked. |
@nikspz This is ready to test again. Update gateway with the latest branch. |
Before submitting this PR, please make sure:
A description of the changes proposed in the pull request:
Adds pool_id to amm_arb strategy.
To be tested with hummingbot/gateway#280
Tests performed by the developer:
Tips for QA testing: