-
-
Notifications
You must be signed in to change notification settings - Fork 140
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/carbondefi #256
feat/carbondefi #256
Conversation
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.
Requested changes:
-
Please move all tests to the
test-bronze
folder, so that they are not included in workflows -
I recommend that this PR should only add the AMM connector initially, that is within the bounds of the existing Gateway structure. Keep in mind that DEX connectors are only meant to be standard integrations that work with a specific strategy, like AMM-ARB. Afterwards it's merged in, I think you can add the custom functionality as another PR.
-
Regarding swapping ETH, you can modify the amm/trade and amm/price endpoints to remove the check for ETH for this connector.
-
I think the CLOB functionality, limit orders support, and associated endpoints could be part of this connector or as part of a different connector folder - it's up to how you want this connector to be used. Currently, Gateway assumes that these connectors are being added so that users can run standard strategies like AMM-ARB with them. However, if you want to add custom scripts that allow users to use other endpoints that this connector supports, that approach could work too.
Thank you for the review Michael. Changes:
|
Looks good @tiagofilipenunes. I think you should be able to create a Pull Request Proposal to have us review and merge this connector into the codebase. |
hi @tiagofilipenunes Please resolve branch conflicts and be informed that all PRs should use development branch and be pointed to |
@nikspz Thanks for pointing that out, just merged and resolved conflicts.
|
PR update:
|
As @rapcmia's tests are successful, I will merge this PR in so that other PRs can resolve any conflicts if needed. |
Hi @tiagofilipenunes, please add connector docs here - https://github.com/hummingbot/hummingbot-site |
Hi @david-hummingbot thanks for the reminder! Here is the PR to add the connector docs: |
Thanks @tiagofilipenunes, this will be included in the 1.25.0 release next week. |
Closes #255
A description of the changes proposed in the pull request:
In this pull request, a Carbon DeFi AMM connector is added. The following endpoints are available:
Tests performed by the developer:
Test for Carbonamm
Coverage:
Tips for QA testing and comments/questions: