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.

Sign up for GitHub

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

Jump to bottom

Add inverse support #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add inverse support #6

wants to merge 3 commits into from

Conversation

arodus
Copy link
Contributor

@arodus arodus commented Oct 31, 2023 •
edited
Loading

Description

Add missing support for inverse

I'm currently investigating if there are changes required in the brokerage model due to a slightly different order handling. But this should only affect backtests.

Related Issue

Part of QuantConnect/Lean#5898
Depends on #5

How Has This Been Tested?

  • New tests
  • Backtesting using inverse as a datasource
  • Running algorithm on testnet
  • Running algo on live using inverse as datasource

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.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My branch follows the naming convention bug-<issue#>-<description> or feature-<issue#>-<description>

@arodus arodus force-pushed the feature-inverse-futures branch from 0bfd8f3 to 27b3f59 Compare November 2, 2023 10:53
@arodus arodus changed the title Add inverse futures support Add inverse support Nov 3, 2023
@arodus arodus force-pushed the feature-inverse-futures branch from 27b3f59 to 2c3e33c Compare November 6, 2023 13:48
@Martin-Molinero Martin-Molinero mentioned this pull request Nov 17, 2023
Closed
jhonabreul
jhonabreul requested changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@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.

Nice! Thanks for the contribution 🚀 Leaving minor comments.

QuantConnect.BybitBrokerage/Models/Enums/PositionMode.cs Outdated Show resolved Hide resolved
QuantConnect.BybitBrokerage/Api/BybitPositionApiEndpoint.cs
/// <param name="category">The product category</param>
/// <param name="symbol">The symbol for which the mode should be changed</param>
/// <param name="mode">The mode which should be set</param>
public void SwitchPositionMode(BybitProductCategory category, Symbol symbol, PositionMode mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

How/where is this going to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used but the question is if it should. Hedge mode is currently not supported and for inverse the default mode is hege which can only be changed to normal mode by using the API.
So should this be done automatically or is the user responsible for that, if so is there a way to make this function available to the user in any way. It would definitely need to be documented.

QuantConnect.BybitBrokerage.Tests/BybitInverseFuturesBrokerageHistoryProviderTests.cs Outdated Show resolved Hide resolved
QuantConnect.BybitBrokerage.Tests/BybitInverseFuturesBrokerageTests.cs Outdated Show resolved Hide resolved
@jhonabreul
Copy link
Contributor

Another comment, trading inverse contracts with the Unified Trading Account will use the Inverse Derivatives account, so balance and margin will be used from the Inverse Derivatives account.

  • Perhaps we need to modify which account we want to fetch the initial balance from? https://bybit-exchange.github.io/docs/v5/account/wallet-balance
    • "Account type: Unified account: UNIFIED (trade spot/linear/options), CONTRACT(trade inverse)"
  • How will the we know which account to used?
    • Do we need a different brokerage class for this case for inverse contracts trading only?
    • If so, would we need a new brokerage factory as well?

@arodus
Copy link
Contributor Author

arodus commented Nov 24, 2023

@jhonabreul
Will double check the different wallet-balance endpoints.

@arodus arodus force-pushed the feature-inverse-futures branch from b54823a to 85cf70f Compare November 24, 2023 19:47
@arodus
Copy link
Contributor Author

arodus commented Nov 25, 2023

@jhonabreul

How will the we know which account to used?
Do we need a different brokerage class for this case for inverse contracts trading only?
If so, would we need a new brokerage factory as well?

Yes it seems that's the case. Will add a new brokerage factory and adjust the Brokerage again so it can be inherited from.

@arodus arodus force-pushed the feature-inverse-futures branch from 85cf70f to e6bcfe7 Compare December 2, 2023 19:58
@arodus arodus force-pushed the feature-inverse-futures branch from e6bcfe7 to 65cd35f Compare December 2, 2023 19:59
jhonabreul
jhonabreul approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@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.

New changes look good, thank you!
Approved on my side. We can wait for @Martin-Molinero to give it a look too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhonabreul jhonabreul jhonabreul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arodus @jhonabreul