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/add_injective_v2_paper_trade_support #6684

Merged

Conversation

aarmoa
Copy link
Contributor

@aarmoa aarmoa commented Nov 27, 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:
This PR adds Injective V2 spot connector to the list of paper trade connectors.
Includes a refactoring to make the client configuration for paper trade connectors the only official list of paper trade connectors.

Tests performed by the developer:
All unit tests passing in green

Tips for QA testing:

  • Run a pure market making strategy using the injective_v2_paper_trade connector.
  • Check that you can create an Avellaneda strategy.
  • Check that you can create a Cross Exchange Market Making strategy.

PRP: https://snapshot.org/#/hbot-prp.eth/proposal/0xd74ce4c37dbc73a9379956c6f535117569cc95d09ff183cf52868b9ab1a568e9

abel added 2 commits November 28, 2023 10:56
…GES and use the values from the Pydantic configuration
…to feat/add_injective_v2_paper_trade_support
Copy link
Contributor

@cardosofede cardosofede left a comment

Choose a reason for hiding this comment

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

LGTM!

@cardosofede
Copy link
Contributor

@aarmoa @rapcmia the poll already met the quorum, but I think that based on the nature of the PR we can merge it without the final approval time. What do you think @fengtality ?

@nikspz
Copy link
Contributor

nikspz commented Nov 29, 2023

@aarmoa Could you please review this issue?
it happened on the start of the strategy using injective_v2_paper_trade

Steps:
Start the client
Create pureMM strategy using injective_v2_paper_trade
start the strategy

Actual:
Review Unhandled error in background task showing

logs_injpaper2.log
6684.zip

2023-11-29 14:51:47,595 - 2763684 - hummingbot.core.utils.async_utils - ERROR - Unhandled error in background task: 
Traceback (most recent call last):
  File "/home/nikita/injpaper6684/hummingbot/core/utils/async_utils.py", line 9, in safe_wrapper
    return await c
  File "/home/nikita/injpaper6684/hummingbot/connector/exchange/injective_v2/injective_v2_api_order_book_data_source.py", line 40, in listen_for_subscriptions
    await self._data_source.start(market_ids=market_ids)
  File "/home/nikita/injpaper6684/hummingbot/connector/exchange/injective_v2/data_sources/injective_data_source.py", line 222, in start
    await self.initialize_trading_account()
  File "/home/nikita/injpaper6684/hummingbot/connector/exchange/injective_v2/data_sources/injective_read_only_data_source.py", line 211, in initialize_trading_account
    raise NotImplementedError
NotImplementedError

image

@rapcmia rapcmia requested review from nikspz and removed request for nikspz and rapcmia December 5, 2023 10:43
@aarmoa
Copy link
Contributor Author

aarmoa commented Dec 7, 2023

Sorry @nikspz, I did not receive a notification with your comment. I will take a look at the issue.

@aarmoa
Copy link
Contributor Author

aarmoa commented Dec 7, 2023

@nikspz please try again. I made some changes for paper trade to work also when there is no Injective V2 connector configured in the system

Copy link
Contributor

@nikspz nikspz left a comment

Choose a reason for hiding this comment

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

Test performed:

  • Cloned and installed feature branch
  • Manually built docker image successfully
  • Created/started pureMM using injective_v2_paperTrade successfully
  • Created/started pureMM using binance successfully
  • Created/started XEMM strategy using injective_v2_paperTrade and binance_paperTrade successfully
  • Created/started avellaneda_market_making strategy using injective_v2_paperTrade

@nikspz nikspz merged commit e63368c into hummingbot:development Dec 8, 2023
2 checks passed
@aarmoa aarmoa deleted the feat/add_injective_v2_paper_trade_support branch December 9, 2023 03:11
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.

None yet

4 participants