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 spot offline vaults #6512

Merged

Conversation

aarmoa
Copy link
Contributor

@aarmoa aarmoa commented Aug 4, 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 expands the capabilities of the Injective V2 spot connector, adding logic to support trading through off-line vaults.

The PR also includes a refactoring to the connectors's configuration in order to allow connectors to use full Pydantic configurations (meaning that the connectors now can have configuration models with N submodels, each of them supporting the use of secret parameters)
The new version is backwards compatible and the connectors will still be using the old configuration style model, unless they explicitly declare the use the new module (with this PR only Injective V2 spot connector uses the new configuration model).

There is also an improvement in the logic that checks the orders are included in a transaction. Now the connector updates the order hashes in case they were incorrectly calculated locally in the bot, to reduce the chances of producing lost orders.

Tests performed by the developer:
Added unit tests for the new logic.
All tests passing in green.

Tips for QA testing:
Validate that old connectors can still be configured as usual.
Validate that the private key for Injective V2 connectors is stored in the config file encrypted

This PR is a continuation of the work done in #6493. No need for a PRP.

abel added 17 commits July 21, 2023 12:31
… sent by the libraries to the stderr braking the bot interface
…the full subaccount id to create orders with delegated accounts
… use the full subaccount id for delegated accouns, but the subaccount index only for vaults
@rapcmia
Copy link
Contributor

rapcmia commented Aug 7, 2023

PR update:

  • Test latest commit 904786af41
  • Check old connectors {GOLD, SILVER} due to changes on configuration
    • Connect exchanges API and tested invalid keys ok
    • Check connector .yml files and api/secret are encrypted
  • Setup injective_v2:
    • Connector now selects between mainnet, testnet and custom
    • Connector status now only shows injective_v2 only,
      • How would I know if which network connected?
        • We think the only way to know is by checking the balance
          image
          • On the screenshot we setup testnet and from the balance it is noticeable huge amounts
      • How would I change from different network ( testnet -> mainnet/custom )?
        • User must run the command again like normal CEX setup and select the network
      • Validate that the private key for Injective V2 connectors is stored in the config file encrypted?
        • Yes, checking injective_v2.yml pkey is encrypted when setting up testnet
    • Connector now support two types of account configuration {delegate_account, vault_account}
      • vault_account not yet tested pending
    • custom_network not yet tested pending

Ran tests on mainnet

image

  • When selecting network, there is a added value of node=’lb’
  • When selecting mainnet_network getting error of Can't instantiate abstract class InjectiveNetworkMode with abstract methods network, use_secure_connection (see screenshot attached above)

Ran tests on testnet

image

  • On the prompt of type of account config, there are added characters also
  • Only on testnet was able to move from the setup and there are two types of account type {delegate_account, vault_account}
    image
    • Able to create and cancel orders ✅
    • Trade data between client history and CSV matched on testnet trade history

@rapcmia
Copy link
Contributor

rapcmia commented Aug 8, 2023

  • Test latest commit a2a33874afb1bb
  • Mainnet is now fixed and able to setup delegate_account
  • However there are still UI issuewhen selecting network and account configuration
    image
  • For vault_account config, unfortunately this is not available on our resource (seems referring to smart contract_base vault for managing assets) so QA can't further look into 🙇🏼

@aarmoa
Copy link
Contributor Author

aarmoa commented Aug 8, 2023

Hello @rapcmia

When selecting mainnet_network getting error of Can't instantiate abstract class InjectiveNetworkMode with abstract methods network, use_secure_connection (see screenshot attached above)

Good catch, I have pushed already a fix for this.

When selecting network, there is a added value of node=’lb’
On the prompt of type of account config, there are added characters also

The connector has a default configuration that is used when querying for the available trading pairs. And those characters you see are the way Hummingbot displays the Pydantic configuration instances (the default way for Pydantic). If you want to change that it will be a task for the foundation. Changing the way Pydantic is used in Hummingbot is outside the scope of this PR (@cardosofede)

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

  • Connector does not require gateway connection
  • Check {binance, kucoin, ascendex and gateio} spot and perpetual conenctors due to changes on connector configration usign full Pydantic config
  • Setup Injective_v2
    • Connector status now only display injective_v2
    • On setup, user can select between:
      • Network to connect {mainnet_network, testnet_network, custom_nework}
        • custom_network no tested ❌
      • Supports two account configuration {delegate_account, vault_account}
        • vault_account not tested ❌
    • Successfully connected wallets for mainnet and testnet
    • Ran balance command, ok
    • Ran tests on PMM for both networks
  • Fixed issues related to mainnet
  • Build docker image and did the same tests

There are minor UI issues related to Pydantic config as adviced by developer. The foundation team will have a look on this concern. Other than that PR is good to be approve under bronze standards. Thank you

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.

LGMT!

@cardosofede cardosofede merged commit 4b02c52 into hummingbot:development Aug 10, 2023
2 checks passed
@aarmoa aarmoa deleted the feat/injective_spot_offline_vaults branch August 10, 2023 12:12
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

3 participants