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/migrate to injective v08 #6565

Merged

Conversation

aarmoa
Copy link
Contributor

@aarmoa aarmoa commented Sep 12, 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:

  • Adapted the Injective connector's configuration to be able tu use the SDK v 0.8.*
  • Added a fix to correctly handle the case when the connector is requested to create a market order without a price, or with price being NaN (the default case for market orders in most of the strategies).
  • Changed network configuration for Injective connector to support only the lb node for mainnet

Tests performed by the developer:
All tests running in green.
The connector can be configured when running the client for the mainnet network.

Tips for QA testing:

  • Connector can be correctly configured for mainnet network.
  • Connector can be used without issues in delegated account mode to create market orders. If the order book is not complete the strategy is required to provide a reference price, because the connector can't calculate one based on the orderbook.

abel added 3 commits September 12, 2023 10:32
…ctors to handle correclty order creation with price None or NaN (for market orders. Removed support for mainnet nodes other than the load balanced node.)
@rapcmia rapcmia self-requested a review September 13, 2023 02:02
Copy link
Contributor

@rapcmia rapcmia left a comment

Choose a reason for hiding this comment

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

LGTM

  • Test on local mbp on both osx64 and arm64
  • Ran account_delegate script on mainnet
    • Issue on changing node has been fixed
    • No errors observed and txn matched on the data used on script
  • connect injective_v2 and injective_v2_perpetual
    • Ran tests only for delegate_account
    • Setup does not require change of node
  • Setup simple PMM and PerpMM
    • Error on mainnet now is fixed
    • However not able to fully test due to QA test wallet
    • If insufficient balance, it is displayed as well on the logs
  • Build docker image and ran same steps, all ok

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!

@rapcmia
Copy link
Contributor

rapcmia commented Sep 15, 2023

Hi @aarmoa good day
When available, can you look on the branch conflicts? thank you

@cardosofede cardosofede merged commit d95109e into hummingbot:development Sep 15, 2023
2 checks passed
@aarmoa aarmoa deleted the feat/migrate_to_injective_v08 branch September 15, 2023 12:11
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.

None yet

3 participants