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/injective low leve api components #6811

Merged

Conversation

aarmoa
Copy link
Contributor

@aarmoa aarmoa commented Jan 26, 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:

  • Added a new configuration parameter in the connectors to allow the user to choose if the gas to be configured in transactions is going to be determined using simulation or if it will be calculated using the Python SDK estimator. The new configuration is backwards compatible and by default the option will be simulation (to keep the same behavior the connector has prior to the change). But using the estimator improves performance, because the simulation is no longer required, removing the wait time required for the simulation to happen.
  • Improved the logic in the connector that keeps track of the last message received from the chain or the indexer, to improve disconnection detections.
  • The PR also includes some internal changes to stop using some methods in the SDK now marked as deprecated.

Tests performed by the developer:
All unit tests passing in green.
Spot and perpetual connectors were tested also by running bots connected to testnet, both using delegated accounts and vaults.

Tips for QA testing:

  • Check an instance of the connector created before the change is still usable without issues in the new version
  • Validate the new configuration parameter (the gas fee calculator)

This PR is associated to the docs update in hummingbot/hummingbot-site#338

abel added 17 commits January 8, 2024 15:25
… orders in successful TXs for MsgBatchUpdateOrders
…to feat/injective_low_leve_api_components
…ake fee estimation configurable. The user can choose to calculate gas fee using simulation or use the Python SDK message based gas fee estimation
…to feat/injective_low_leve_api_components
…to feat/injective_low_leve_api_components
…to feat/injective_low_leve_api_components
…to feat/injective_low_leve_api_components
…to feat/injective_low_leve_api_components
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

@nikspz code review ready, please run the test that @aarmoa requested and ask any questions that you have around the fee calculation.
All the information should be provided here: hummingbot/hummingbot-site#338

@rapcmia
Copy link
Contributor

rapcmia commented Jan 31, 2024

PR update:

  • Review new feature
  • Ran test on commit e9f9b756ece3d79..
  • Setup wallets successfully for both spot and perpetuals
    • Confirmed new parameters added
  • Setup simple perp-mm strategy and observe behavior using default setup simulated_transaction_fee_calculator
  • Found bug on connector which is not related to the PR

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:

  • commit e9f9b75..

  • connect injective_v2 successfully for both spot and perpetuals

  • Confirmed new parameters added

  • Injective_v2 (spot)

    • testnet on
      • simulated_tx_fee_calculator: ok
      • message_based_tx_fee_calculator: ok
    • mainnet
      • simulated_tx_fee_calculator:
        • setup pureMM strategy and did trades successfully
        • review history: ok, fees matched with the Helix app exchange
      • message_based_tx_fee_calculator:
        • setup pureMM strategy and did trades successfully
        • ok, fees matched with the Helix app exchange
  • Injective_perpetaul_v2 on mainnet

    • simulated_tx_fee_calculator: ok, fees matched using PerpetualMM strategy with the Helix app exchange
    • message_based_tx_fee_calculator: ok, fees matched using PerpetualMM strategy with the Helix app exchange

Note: issue that Ralph reported #6824 reproduced on mainnet spot and perpetual. This part could be improved by Injective.

As Abel reported, issue related to the public node we use and not related to this PR
However, was able to trade using V1 strategies and delegate_account mode
simulated - error while listening to tx stream
image

@rapcmia rapcmia merged commit cecc0dd into hummingbot:development Feb 2, 2024
2 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.

None yet

4 participants