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/pancakeswap v3 price calculation. #251

Merged
merged 19 commits into from
Jan 7, 2024

Conversation

0xcodercrane
Copy link
Contributor

@0xcodercrane 0xcodercrane commented Dec 15, 2023

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:

  • Add an interface for v3 smart router
  • Add the calculation method for the v3 smart router
  • Add the execution method for the v3 smart router

Tests performed by the developer:

Tips for QA testing:

@0xcodercrane 0xcodercrane changed the title feat: pancakeswap v3 price calculation. feat/pancakeswap v3 price calculation. Dec 15, 2023
@fengtality
Copy link
Contributor

@vic-en can you take a look at this PR since you worked on PancakeSwap upgrade?

@vic-en
Copy link
Collaborator

vic-en commented Dec 16, 2023

@fengtality
These changes are not related to pancake v3 LP.
They're changes to the existing swap.
Nevertheless, I'll look before today ends.

Copy link
Collaborator

@vic-en vic-en left a comment

Choose a reason for hiding this comment

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

@0xcodercrane
Just one comment for you, you can remove the router abi file as I see it's no longer used.

@fengtality
As mentioned earlier, this changes pertain to the swap funtionalities.
As it is, it integrates the latest smart router which allow swaps to automatically use v3 pools when available.
That being said, there's no funtionality to strictly use v3 pool only as available in uniswap code. So I guess further decision is up to you.

@0xcodercrane
Copy link
Contributor Author

@0xcodercrane Just one comment for you, you can remove the router abi file as I see it's no longer used.

@fengtality As mentioned earlier, this changes pertain to the swap funtionalities. As it is, it integrates the latest smart router which allow swaps to automatically use v3 pools when available. That being said, there's no funtionality to strictly use v3 pool only as available in uniswap code. So I guess further decision is up to you.

@vic-en routerAbi is required field of Uniswapish.
and for the functionality which allow swaps to use only v3 pools, i pushed the changes about allowed pool types. i believe we can use that function.
45052d9

@cardosofede
Copy link
Contributor

@vic-en @0xcodercrane @fengtality what about this pr, what else is missing to merge it?

@jellebuth
Copy link

We have been testing the implementation and noticed a 10x increase in RPC calls for price fetching. The free ANKR endpoint does not support price fetching for PCS V3 as there is a limit of 300/s and it is exceeded.

Please take a careful look into this before merging.

image

yarn.lock Outdated Show resolved Hide resolved
@0xcodercrane
Copy link
Contributor Author

Could you please approve ci/cd run @fengtality ?

@0xcodercrane
Copy link
Contributor Author

adding last fixes of unit test.

@cardosofede
Copy link
Contributor

approved workflow run @0xcodercrane

@0xcodercrane
Copy link
Contributor Author

0xcodercrane commented Jan 5, 2024

approved workflow run @0xcodercrane

thank you. how can I see which tests are failing from ci/cd. Would you help me on that? @cardosofede

@0xcodercrane
Copy link
Contributor Author

hey admins! anyone please approve the workflow run?

@fengtality
Copy link
Contributor

@0xcodercrane Thanks - looks like tests are passing now. I'll review and address @vic-en's comment above shortly.

Copy link
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.

Looks good - thanks for the addition of the subgraph connection - I can definitely see why this improves latency for price fetching.

@jellebuth Thanks for testing this PR. Does the change from Ankr to LlamaRPC as default provider fix your issues?

@0xcodercrane Requested changes:

Review the fields in pancakeswap.yml in the templates folder. If any are not used anymore after this change, please remove them from this file. This will let users know what options they have to modify how the connector works.

@vic-en I don't think we need to add an option for users to select only V2 or V3 pools, unless they request it afterwards.

@fengtality
Copy link
Contributor

Actually, I will approve merge this PR in now, so that other outstanding PRs can update their dependencies. @0xcodercrane please clean up the Pancakeswap default configs template at your convenience.

@fengtality fengtality merged commit f7b0981 into hummingbot:development Jan 7, 2024
3 checks passed
@0xcodercrane 0xcodercrane deleted the fix/pancakeswap-v3 branch January 9, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants