-
-
Notifications
You must be signed in to change notification settings - Fork 142
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/ Osmosis Chain/Connector #277
Conversation
…poolId->tokenId (so reusing tokenId as poolId for osmo) - should make og strat work
Please resolve conflicts as we just merged in another DEX connector |
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.
Please ma
@nkhrs As mentioned on Discord, the additions to the LP interfaces are fine. To wrap up this PR: please move the Osmosis chain tests to the /test-bronze folder, and address any unit test failures due to interface changes. After that, LGTM! |
… deleted. Currently getting an error back from RPC when trying to add CL liquidity
All done |
I was wrong about the invalid coins error - turns out it was angry about alphabetization, not ratio. Anyway, that's fixed. That should be all the issues you mentioned, but I did see that .find/undefined error once on my side as well: And it's definitely an issue with the API: It came back up pretty quickly, at least: When testing this, are you able to connect to a VPN in the EU? That might help, but I've also made it just ignore fees when we can't access that endpoint (fees will all appear as 0). |
Thanks for the update 🚀
|
@rapcmia Also - can you tunnel to Germany? |
I think it's fixed now - is it correct to assume that that's just the market price of the 2 coins? Otherwise there's no way to get a price for a specific pool in Osmosis; as far as I can tell, the concept doesn't exist. |
Hi @chasevoorhees thanks for the update,
|
@rapcmia Yikes - that one's just a typo. Sorry. Here's the updated file (also pushed to the repo above) |
@rapcmia This is definitely slippage related - and actually the default allowedSlippage in osmosis.yml is pretty low: allowedSlippage: 2% You may need to set this around 40% for that transaction to go through, however what this is actually used for is calculating the minimum amount of token you're willing to put into the pool... so it's not really slippage as used elsewhere/for trades, per se:
This is my join pool message with slippage=100% (hence tokenMinAmount0/1 == 0) Here's what I ended up putting into my pool:
I believe this implies a value imbalance in the pool (in my case, there was already too much of token0 so it won't accept any more - previously I was getting this: "slippage bound: insufficient amount of token 0 created. Actual: (0). Minimum estimated: (234720000000000000)") Would you suggest that I set the minAmount to 0 for all cases and disregard slippage as used elsewhere? Anyway, if you set your allowedSlippage='100%' then you should be able to get past this. I asked the Osmosis team about this one as well to confirm. |
I also read that you're having issues setting values through the client: |
Hi @chasevoorhees thanks for checking, I change the
|
|
Sorry this one is taking bit longer than expected; the good news is that I finally reconciled the tokenId == poolId || positionId issue. Basically, tokenId can be either poolId || positionId and I just figure out which on the backend once reduceLiquidity() is called. This means we'll support multiple CL positions for the same pool (GAMM positions are all strictly 1 per pool though). I'll update later tonight and will hopefully have finished re-testing everything by then. |
Thanks for the update @chasevoorhees,
|
Thanks for the update @chasevoorhees |
Been running it through debug and I'm somewhat confused on how the order tracking with the client works Here's what gateway_evm_amm_lp is doing:
I'm assuming that txStatus==1 means the transaction was confirmed. I'm copying this behavior, and thus we're ending our order tracking right after Osmosis accepts the position. So - when EVM positions run, every time we hit this: Consequently, the position is never picked up by status since it's removed right after it's confirmed... Anyway - still looking at this. Will try to get some more advice. |
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.
- Setup this PR with feat/Osmosis Chain Connector - add Osmosis to native tokens list hummingbot#6685
- Ran API tests using postman
- GET connectors, status and connectors ok
- POST balances, poll and tokens ok
- Wallet add, get and remove ok
- POST price, add and check position ok
- POST remove and collect fees ok
- Setup on amm_lp strategy:
- Connector available ok
- Ran tests on LOW and MEDIUM tier
- Modified gateway configs ok
- However there is ongoing issue on connector where continuously creating positions.
- The team decided to merge this connector and create a separate ticket Osmosis - Continuously creating position on amm-LP strategy #294 for a fix
- To be discussed for possible HIP or open community bounty
Hi @nkhrs please add the docs for the Osmosis chain/connector in our docs site - https://github.com/hummingbot/hummingbot-site |
Hi @david-hummingbot sure thing. Quick question, our current fork and branch (https://github.com/pecuniafinance/hummingbot-site/tree/doc/osmosis) contains a Connector Guide for Osmosis as well as the osmosis chain page, osmosis connector page, and updated cosmos chain page. Should I keep the guide in there or remove it and do a separate PR at a later date? |
If you already have a copy in your fork you can just submit a PR upstream to update our docs site ideally before the release this month as people will be looking to the docs for information on the connector, how to connect etc. |
Thanks for all the help @rapcmia Let me know if there are any issues with the backend/chain/connector (well, stuff I wrote) going forward. |
Hi @david-hummingbot PR is opened for the osmosis docs hummingbot/hummingbot-site#355 Cheers |
Before submitting this PR, please make sure:
[x ] Your code builds clean without any errors or warnings
[x ] You are using approved title ("feat/", "fix/", "docs/", "refactor/")
A description of the changes proposed in the pull request:
Adds Osmosis (with LP) support as a chain, plus some updates to Cosmos chain.
Cosmos amm.routes.ts models don't match up well with the rest due to inherent differences in their data. This has been handled by allowing certain endpoints to receive Cosmos-specific models switched by chain, eg. AddLiquidityRequest | CosmosAddLiquidityRequest.
Docs for above endpoints are in Swagger now.
Tests performed by the developer:
Jest tests added for all Osmosis functions. Very little patch use, instead a testnet wallet is instantiated and trades/liquidity actions are performed with it.
yarn test:cov is ~80% for Osmosis, improved for Cosmos.
Tips for QA testing:
THIS LINE MUST to be added to HBOT client native_tokens[]
"osmosis": "OSMO",
in hummingbot/core/utils/gateway_config_utils.py
PR has been submitted to HBOT main for that file as well.