Feat/pancakeswap - remaining CLMM schema changes #509
Conversation
|
@vic-en note merge conflicts |
|
@fengtality @nikspz This connector is ready. I implemented all functionalities as is in the Uniswap connector as of now. All endpoints are tested and working. |
|
hi @vic-en could you please add pancakeswap connector to Client also 
|
|
@nikspz it has been updated |
|
@nikspz No changes to the client should be needed after v2.8, since the list of connectors and chains are fetched from Gateway at initialization. Here's what I see in Hummingbot when running Gateway with this PR: |
|
Failed to approve token using Steps:
Actual: logs_hummingbot.log 
|
logs_hummingbot.log 
|
|
@nikspz The PancakeSwap/CLMM approval issue is due to fact that there are 2 smart contracts for Pancakeswap and Uniswap CLMM - one used for swaps and other used for liquidity operations The same exists for Uniswap: To handle this, I added handling for 
You should only need to use See hummingbot/hummingbot#7772 and #508 branches |
|
@nikspz Also, most users will likely use Router for swaps, so I suggest concentrating your tests on opening and closing LP positions for the Pancakeswap CLMM and AMM connectors |
|
@vic-en Some minor issues found during testing. Since you are working on the Uniswap bugs PR, please include these fixes in that one. 1 - GET Quote Position for both Uniswap CLMM and Pancakeswap CLMM don't show the proper default values in Swagger, because they are using the schemas directly instead of overriding them. Adding connector-specifc schemas like the AMM routes should fix this. 
is.
2- The response to |
|
When fetching an open position quote, the base and quote amounts don't match what's shown in PancakeSwap. Here, the same upper and lower price bounds results in response that says quote amount should be 0. 
|
|
@fengtality Noted. |
|
hi @vic-en Could you please check clmm connector and adding liquidity price?
logs_hummingbot.log |
|
@fengtality the issue with clmm is that the baseToken and quoteToken are specified in the swagger but not used at all. Note that fixing this may break existing implementation on the client if the client doesn't send the baseToken and quoteToken when required. |
|
@fengtality I've isolated changes I plan to apply to other clmm endpoints(including Uniswap's) to fix the issue with the amounts in 97c2380 Pls take a look at changes in that commit and let me know if you want me to proceed. |
|
@vic-en I'll check the client again, but the price issue that Nikita is referring to should be unrelated to adding |
|
@fengtality It is the cause of inverted prices and flipped/incorrect amount. As it is right now, there wouldn't be an issue using a pool like https://pancakeswap.finance/info/v3/pairs/0xbe141893e4c6ad9272e8c04bab7e6a10604501a5 because the order of token is ETH-USDT which is what the client would typically use. |
That shouldn't be the issue, because the connector fetches the pool and determines if the baseToken provided matches the pool's token0: It's possible that we need to invert the amounts as well as the price. |
|
@fengtality Sources of My recent fix to the quotePositionRoute endpoint on Pancakswap in this pr is proof that order of supplied token is different from the pool's for WBNB-USDT |
|
@vic-en The functions you are referring to shouldn't be inconsistent, since the poolInfo in the first step is used to fetch the poolInfo where the I agree that this code is confusing. I had AI do most of the Uniswap connector, which is why I appreciate the help in cleaning it up along with the Uniswap fixes. What I'm saying is that adding quote and base token symbols to the add-liquidity routes in the CLMM schema shouldn't be necessary, since the symbols are used only to fetch the right pool address. Once we have the pool, we just need to track if the user's base token is token0 or token1 and carry that logic throughout the connector. I think Raydium has the correct behavior: |
|
Could you check why add liquidity gives wrong amount @vic-en?
|
|
@nikspz The tx showed that the add liquidity was successful for https://bscscan.com/tx/0x85ae5d5d0ae64a8e10c8aa1a821c8d451a3f17eaaa7b692dbbcc70c6342df342#eventlog Also note that you should ideally execute open-position before adding-liquidity |
|
|
@nikspz I discussed Pancakeswap with @vic-en, and we reached agreement that the CLMM and AMM schemas should be revised to address the issues raised above. See #513 for more details. Since this is an existing issue with other connectors like Raydium and Uniswap and a larger change is needed, I think we should merge in Pancakeswap for v2.9 release. Victor will apply the fix to Pancakeswap and Uniswap for the next release, and I'll do the same for the other connectors. |
|
@vic-en I created #513 for the remaining changes we discussed, but I had a chance to discuss with @cardosofede today and he brought up some good points re: design. We might be able to keep the endpoint design simpler if there are some user interface changes in the client. I'll do some testing and propose an updated design for 513 for next release. Let's keep this PR open till then. |
fengtality
changed the title
|
@fengtality noted. |
|
@nikspz Am I missing something? I think the schema is already present. Let me know. Thanks. |
@vic-en see my comment above |
|
@fengtality Got it! |
3 participants
Before submitting this PR, please make sure:
A description of the changes proposed in the pull request:
Add Pancakeswap connector - [https://github.com/hummingbot/hummingbot/issues/7654]
Tests performed by the developer:
Tips for QA testing: