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.

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/ balancer connector #280

Merged
merged 27 commits into from
Jun 2, 2024
Merged

Conversation

vic-en
Copy link
Collaborator

@vic-en vic-en commented Feb 13, 2024

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:
Balancer connector.

Tests performed by the developer:

  • Curl tests
  • Unit test

Tips for QA testing:

  • Curl test
  • End-to-end testing with Hummingbot client.

@vic-en vic-en changed the title add balancer connector with unit tests feat/ balancer connector with unit tests Feb 13, 2024
@vic-en vic-en changed the title feat/ balancer connector with unit tests feat/ balancer connector Feb 13, 2024
Copy link

@nikspz
Copy link
Contributor

nikspz commented Feb 14, 2024

@vic-en Could you please fix failing tests/ add coverage?
https://github.com/hummingbot/gateway/actions/runs/7889551246/job/21530292736?pr=280

@nikspz nikspz self-requested a review February 15, 2024 08:54
@nikspz
Copy link
Contributor

nikspz commented Feb 15, 2024

  • Latest development branch + feat/ balancer connector #280
    • Test performed:
      • gateway generate certs and yarn start = ok
        • ethereum arbitrum
          • gateway connect balancer = ok
          • checked balance successfully
          • approved WETH and USDC using Client interface
          • create/started amm_arb using balancer ethereum arbitrum
            • checked status - got warning: Markets are offline for WETH-USDC
              • Got not realistic PNL for balancer_ethereum_arbitrum to paper_trade
              • amm/price failed during checking status
            • curl
              • amm/price: ok
              • Reviewed delay before response is about ~10-15 sec
        • ethereum mainnet
          • gateway connect balancer = ok
          • checked balance successfully
          • created amm_arb using balancer ethereum mainnet
        • avalanche_avalanche
          • gateway connect balancer = ok
          • checked balance successfully
          • created amm_arb using balancer avalanche_avalanche
        • polygon_mainnet
          • gateway connect balancer = ok
          • checked balance successfully
          • created amm_arb using balancer polygon_mainnet

image

Was able to get 1 trade on balancer arbitrum
balancer
amm/price failed during checking status
image

Reviewed delay before response is about ~10-15 sec
balancer amm price

@nikspz
Copy link
Contributor

nikspz commented Feb 15, 2024

@vic-en Could you please fix failing tests/ add coverage for us to be able to check and add it to this release?
https://github.com/hummingbot/gateway/actions/runs/7889551246/job/21530292736?pr=280

@vic-en
Copy link
Collaborator Author

vic-en commented Feb 15, 2024

@nikspz I'm re-running the tests. I don't think the faillure is due to my change.

@vic-en
Copy link
Collaborator Author

vic-en commented Feb 15, 2024

FYI @fengtality @nikspz
I believe the workflow file was updated at some point.
I see only ethereum goerli node url is now patched -

sed -i 's/https:\/\/rpc.ankr.com\/eth_goerli/http:\/\/127.0.0.1:8545\//g' ./conf/ethereum.yml

That implies several unit tests will be making call to real node endpoints as defined the the configuration templates.

Aside that, I see several unit tests referencing mainnet. Those are not ideal, especially when no patch in workflow file.

image

@vic-en
Copy link
Collaborator Author

vic-en commented Feb 15, 2024

@nikspz You can now see the workflow is passing now and all I did was re-run the workflow.

The hit and miss will always happen due to the numerous unpatched node urls.

@nikspz nikspz mentioned this pull request Feb 15, 2024
@nikspz
Copy link
Contributor

nikspz commented Mar 5, 2024

We should try with router disabled but we need a the config for that
image
we have this thing for Uniswap
image

@vic-en Did you have any chance to check this feature for balancer? Could you please provide some update?

@fengtality
Copy link
Sponsor Contributor

@vic-en I think the hit-and-miss nature of the router means that this connector will need to support the useRouter: false flag to be usable by the community. I think everyone does the same thing with the Uniswap connector.

@vic-en
Copy link
Collaborator Author

vic-en commented Mar 6, 2024

@fengtality Balancer pools can have several tokens. At thesame time a pair of specific tokens can be availale on several pools.

Unlike uniswap, there will be a challenge of which pool to use as there can be multiple pools with same pair of tokens.

CC @nikspz

i.e Pools filtered for BAL and WETH
image

This was referenced Apr 6, 2024
@nikspz
Copy link
Contributor

nikspz commented Apr 11, 2024

hi @vic-en please fix branch conflicts, update with latest development and continue with this PR

@vic-en
Copy link
Collaborator Author

vic-en commented Apr 11, 2024

@nikspz Will do and complete this weekend.

@vic-en
Copy link
Collaborator Author

vic-en commented Apr 16, 2024

@nikspz This is ready.

@nikspz
Copy link
Contributor

nikspz commented Apr 17, 2024

@vic-en Could you please check failing tests / add coverage?

@vic-en
Copy link
Collaborator Author

vic-en commented Apr 17, 2024

@nikspz Again, the failling test isn't due to my changes. I mentioned this some time ago - #280 (comment)
Also, another pr has exact failed test as mine - https://github.com/hummingbot/gateway/actions/runs/8714449599/job/23912168538?pr=308

Nevertheless, I can look at that when time permits.

@fengtality
Copy link
Sponsor Contributor

I think this PR fixes the failing tests: #311

@nikspz
Copy link
Contributor

nikspz commented Apr 19, 2024

  • Test performed:
    • commit 65f4d94
    • Cloned and installed balancer gw280 + latest development Client
    • gateway generate certs and review gateway ONLINE
    • connected balancer connector for
      • ethereum mainnet: ok
      • ethereum arbitrum: ok
      • avalanche: ok
      • polygon: ok
    • chain/status: ok
    • wallet/add: ok
    • amm/price:
      • ethereum mainnet: ok
        • able to get the price
        • some of requests returns Error: Price query failed: No pool found, however checking again after short time was successfull
      • ethereum arbitrum: ok
      • polygon: ok
      • avalanche: "Error: 503 unsupported chain or connector

@vic-en Could you check avalanche? Failed to amm/price for balancer_avalanche_avalanche

image

image

image
image

@vic-en
Copy link
Collaborator Author

vic-en commented Apr 22, 2024

@nikspz This is updated. Avalanche now works.

Also, to test with poolId just add the extra poolId in the query like below
image
Note that you can get poolId from url on that Balancer page
image

cc @fengtality

@nikspz
Copy link
Contributor

nikspz commented Apr 22, 2024

We should try with router disabled but we need a the config for that image we have this thing for Uniswap image

@vic-en
Could poolId be available to use / set / change from Client interface?

I tried to check amm/price works for both with and without poolId

@nikspz This is updated. Avalanche now works.

Confirmed, works now.

  • avalanche avalanche
    • gateway connect balancer avalanche = ok
    • checked balance successfully
    • checked curl amm/price successfully
    • created amm_arb using balancer avalanche successfully

@nikspz
Copy link
Contributor

nikspz commented Apr 22, 2024

@vic-en

avalanche_avalanche - trade was successful ✔
https://snowtrace.io/tx/0x53483b9fc56f1cf6d7d9c535295372ca04220743ad2f393b2685e97673248688?chainId=43114

amm/trade failing for ethereum_arbitrum ❌
Upon checking reason was - balancer ethereum_arbitrum failed to approve through Client ❌ (Though it returned approved before)
image

logs_gateway_app.log.2024-04-22.log
image
https://arbiscan.io/tx/0xd5e5a6c588793f019b58d946e3352092c6d6c906ae9b0fd3eb81f3e664b32835
image

liquidity/add ❌
"TypeError: uniswapish.addPosition is not a function\n
image
image
same error for liquidity/add for balancer_avalanche

@vic-en
Copy link
Collaborator Author

vic-en commented Apr 22, 2024

@nikspz The LP endpoints for uniswap like contracts cannot work with Balancer. Balancer is different.
The poolId I added is to make swaps to interract with only the pool whose Id is supplied.

Note:

  • I'll look at the issue with arbitrum approval.
  • I'll also create a pr on the client to make amm arb configure a poolId
    Eta is on/before this weekend.

@nikspz
Copy link
Contributor

nikspz commented May 1, 2024

Note:

  • I'll look at the issue with arbitrum approval.
  • I'll also create a pr on the client to make amm arb configure a poolId
    Eta is on/before this weekend.

Hi @vic-en Do you have any updates on this one?

@vic-en
Copy link
Collaborator Author

vic-en commented May 6, 2024

@nikspz I'll push the update today.

@vic-en
Copy link
Collaborator Author

vic-en commented May 8, 2024

@nikspz This is ready for testing.
Regarding the failed swaps on arbitrum, they failed due to slippage. Increase the slippage in the configuration file.

@vic-en vic-en requested a review from fengtality May 20, 2024 19:24
Copy link
Sponsor Contributor

@fengtality fengtality left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved - Note small typo in logging statement @vic-en

);
if (info.swaps.length === 0) {
throw new UniswapishPriceError(
`No pool found for ${quoteToken.address} to ${baseToken.address}.`
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be No pool found for ${baseToken.address} to ${quoteToken.address}.

@fengtality
Copy link
Sponsor Contributor

I will merge this PR into development and address the typo there. The failing tests are due to the Uniswap router used in the tests requiring network access and can be ignored for now.

@fengtality fengtality merged commit 87a3d66 into hummingbot:development Jun 2, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants