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

Add PriceMatch parameter to order editing #1440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Grepsy
Copy link

@Grepsy Grepsy commented Nov 4, 2024

This adds a PriceMatch parameter to the USD-futures order editing methods (both REST and WS). This is useful for "chasing" limit orders to prevent market order fees.

Submitting as a draft for feedback.

};
parameters.AddOptionalParameter("price", price?.ToString(CultureInfo.InvariantCulture));
Copy link
Owner

Choose a reason for hiding this comment

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

According to the documentation the price is required for USD futures

Copy link
Author

@Grepsy Grepsy Nov 6, 2024

Choose a reason for hiding this comment

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

I noticed that, it's a bit contradictory because the parameter itself is there and mentions:

only avaliable for LIMIT/STOP/TAKE_PROFIT order; can be set to OPPONENT/ OPPONENT_5/ OPPONENT_10/ OPPONENT_20: /QUEUE/ QUEUE_5/ QUEUE_10/ QUEUE_20; Can't be passed together with price

I'll do some testing to see whether it gets accepted.

Copy link
Author

Choose a reason for hiding this comment

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

I have tested it using the locally generated nuget package and the call does get accepted.

/// <summary>
/// PriceMatch
/// </summary>
public PriceMatch? PriceMatch { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

This is not the actual model which gets send, you'll need to copy it in the actual EditMultipleOrdersAsync methods

Copy link
Author

Choose a reason for hiding this comment

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

It's being added to the orderParameters collection from this model like this:

orderParameters.AddOptionalEnum("priceMatch", order.PriceMatch);

Is that what you mean or do I misunderstand?

Copy link
Owner

Choose a reason for hiding this comment

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

It is added in the Coin futures I see, but not in the USD futures EditMultipleOrdersAsync method right?

Copy link
Author

Choose a reason for hiding this comment

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

I think you mean the other way around :-) It's added on Usd but not Coin, I'll fix that!

Copy link
Author

Choose a reason for hiding this comment

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

I don't see any Edit* methods implemented on the BinanceRestClientCoinFuturesApiTrading. To be clear the changes I did are on UsdFuturesApi, both REST and Socket. Hope it makes sense.

@Grepsy Grepsy marked this pull request as ready for review November 6, 2024 23:08
@Grepsy
Copy link
Author

Grepsy commented Nov 6, 2024

Thank you for your feedback. I did some testing and the call does get accepted so the required in the docs only holds when priceMatch isn't supplied.

Please let me know if there's anything else I can do!

/// <summary>
/// PriceMatch
/// </summary>
public PriceMatch? PriceMatch { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

It is added in the Coin futures I see, but not in the USD futures EditMultipleOrdersAsync method right?

@Grepsy Grepsy force-pushed the feature/edit-order-pricematch branch from d5f7a52 to 5808ed6 Compare December 19, 2024 21:38
@Grepsy
Copy link
Author

Grepsy commented Dec 19, 2024

@JKorf Could you please have another look? I've checked the BinanceFuturesBatchEditOrder.cs changes, but these apply to USD not Coin futures as you mentioned. Thanks!

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.

2 participants