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/polkadex v2 #6558

Merged
merged 6 commits into from Sep 7, 2023
Merged

Conversation

petioptrv
Copy link
Contributor

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 introduces an update to the Polkadex connector to use the V2 API.

Tests performed by the developer:

  • Unit tests.

Tips for QA testing:

Note that every now and then, the periodic order status update may fail because it happened immediately after submitting an order. Since Polkadex is a decentralized exchange, it takes a couple of seconds for the order to make it onto the chain and to become available for order status updates.

[ch-43708]

@rapcmia
Copy link
Contributor

rapcmia commented Aug 31, 2023

PR update:

  • Test latest commit 061d77b6ec42a7
  • Request CA QA team to share account for mainnet and successfully import ✅
  • Polkadex available on connect command
  • Successfully connected using mnemonic passphrase with balance displayed correct ✅
  • Setup simple PMM on DOT/USDT
    • Observe order fetch status failed which is expected for the connector
    • Orders are created and cancelled successfully, no stuck order observed
    • Set tight spreads with longer refresh time but unable to have a opportunity for trade with running for 4hrs
  • Build docker image and ran tests, 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.

the changes are okey but just check that and see if you want to implement it in another PR or do the changes on this one.
Btw, I think that we should also trigger the event of the order book updates what do you think?

@@ -37,6 +37,8 @@ class MarketEvent(Enum):

class OrderBookEvent(int, Enum):
TradeEvent = 901
OrderBookDataSourceUpdateEvent = 904
PublicTradeEvent = 905
Copy link
Contributor

Choose a reason for hiding this comment

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

@petioptrv hey pet, the TradeEvent is actually the public trade event check that the order book is triggering it when a new trade arrives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here was to differentiate the "data-source" event from the event that the clients of the exchange connector want to consume, but I agree that, in this case, it's redundant. I'll make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, @rapcmia, the only part of the functionality that needs to be re-tested is that public trade events are being received by the bot

@cardosofede cardosofede merged commit e483036 into hummingbot:development Sep 7, 2023
2 checks passed
@petioptrv petioptrv deleted the feat/polkadexV2 branch September 7, 2023 12:33
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

4 participants