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: make the conditional order factory case insensitive #225

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Dec 2, 2024

We are observing some issues handling some TWAP orders in the logs.

image

According to the log, this message should only be shown for legacy polling, meaning that the order was not recognised as a known one, and instead of using the SDK polling for Conditional Orders, its using the generic polling of the Watch Tower.

The check if the order is a known one is based on the ConditionalOrderFactory who instanciates conditional orders based on their handler.

I can see in the error the handler is TWAP, but it was not instantiated using the SDK which I believe it means the handler was not recognised.

This PR makes the ConditionalOrderFactory to not be case sensitive, because otherwise it depends on the casing of the handler if its recognised or not. With this, the legacy polling should not be used for TWAP.

After this PR

After this PR we need to generate a new SDK version and make use of it in Watch Tower

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 78.071% (-0.2%) from 78.23%
when pulling d8e9e4a on make-conditional-order-factory-case-insensitive
into 5ad85a1 on main.

@anxolin anxolin requested a review from shoom3301 December 2, 2024 21:06
@anxolin anxolin merged commit 698faf1 into main Dec 3, 2024
7 of 9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants