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

Update Binance currency pairs #7693

Merged

Conversation

starteleport
Copy link
Contributor

@starteleport starteleport commented Jan 12, 2024

Description

This adds missing Binance symbols to the symbol database.

Related Issue

Closes #7692

Motivation and Context

Some existing Binance trading pairs were absent in the symbol database.

Requires Documentation Change

No

How Has This Been Tested?

This is the metadata change only, so I suppose there is no need to run tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (non-breaking change which improves implementation)
  • Performance (non-breaking change which improves performance. Please add associated performance test and results)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Non-functional change (xml comments/documentation/etc)

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • Added tests for new stablecoin, fixed existing.
  • All existing tests are passing.
  • My branch follows the naming convention bug-<issue#>-<description> or feature-<issue#>-<description>

@AlexCatarino
Copy link
Member

We also have a request for GBP-quoted crypto pairs. It's out of the scope of the issue that led to this pull request, but we could add a commit to address it too.

@starteleport
Copy link
Contributor Author

Actually yes, it's not a complete list of the pairs that are absent. If it's okay to go out of scope here, I could add a broader range of crypto pairs (i.e. all that are absent at the moment)

@Martin-Molinero
Copy link
Member

Martin-Molinero commented Jan 15, 2024

Hey @starteleport ! Thank you for the contribution,
Running the binance toolbox updater will generate the new complete SPDB automatically based on their api see https://github.com/QuantConnect/Lean.Brokerages.Binance/blob/master/QuantConnect.BinanceBrokerage.ToolBox/Program.cs#L70. Could you give it a run?

@starteleport starteleport force-pushed the bug-7692-add-fdusd-spot-pairs branch from 7af69c1 to 34073f8 Compare January 21, 2024 19:09
@starteleport
Copy link
Contributor Author

@Martin-Molinero done!

@starteleport starteleport changed the title Add Binance FDUSD spot pairs to symbol database Update Binance currency pairs Jan 21, 2024
Copy link
Collaborator

@jhonabreul jhonabreul left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Looks good. Just one comment: why the changes in the Kraken symbols?

@Martin-Molinero
Copy link
Member

Thank you @starteleport! Almost there, please revert the kraken SPDB changes like JA noted 👍. There seems to be a few unit tests failing in the PR, possibly related to that change

@starteleport starteleport force-pushed the bug-7692-add-fdusd-spot-pairs branch from 34073f8 to c8625c5 Compare January 23, 2024 17:31
@starteleport starteleport force-pushed the bug-7692-add-fdusd-spot-pairs branch from c8625c5 to de9200a Compare January 23, 2024 17:36
@starteleport
Copy link
Contributor Author

Ah, sorry for Kraken bits, they were supposed to be rolled back.

@starteleport
Copy link
Contributor Author

Also, I'm looking into the failing tests now

Copy link
Member

@Martin-Molinero Martin-Molinero left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

Copy link
Collaborator

@jhonabreul jhonabreul left a comment

Choose a reason for hiding this comment

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

Thank you!

@starteleport starteleport force-pushed the bug-7692-add-fdusd-spot-pairs branch from ab143c3 to e113564 Compare January 23, 2024 20:08
@starteleport
Copy link
Contributor Author

@Martin-Molinero, could you review again, please? I've fixed a typo in a symbol name.

@starteleport
Copy link
Contributor Author

That's kind of strange. I have OptionChainProviderTests.LiveOptionChainProviderReturnsFutureOptionData failing locally and in my GH fork.

I'm not sure about the reasons, because this seems unrelated. Any ideas? If not, I'll look into that a bit later.

@starteleport
Copy link
Contributor Author

It looks like the API used during the test is failing:

20240123 20:28:02.303 ERROR:: <GetFutureOptionContractList>d__15.MoveNext(): Failed to retrieve futures options chain from CME, returning empty result (5 / 5) System.Net.Http.HttpRequestException: Response status code does not indicate success: 403 (Forbidden).

Given that there had been quite a few test runs of this test recently, and the throttling inside GetFutureOptionContractList, I suspect that we have hit some rate limiting of the called API.

@Martin-Molinero Martin-Molinero merged commit 17902e2 into QuantConnect:master Jan 23, 2024
4 of 5 checks passed
@Martin-Molinero
Copy link
Member

It looks like the API used during the test is failing:

20240123 20:28:02.303 ERROR:: <GetFutureOptionContractList>d__15.MoveNext(): Failed to retrieve futures options chain from CME, returning empty result (5 / 5) System.Net.Http.HttpRequestException: Response status code does not indicate success: 403 (Forbidden).

Given that there had been quite a few test runs of this test recently, and the throttling inside GetFutureOptionContractList, I suspect that we have hit some rate limiting of the called API.

Hey @starteleport! Thank you 🚢
Yes, that unit test has started failing on and off for github CI, unrelated 👍

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.

Add FDUSD-Quoted Crypto Pairs
4 participants