Skip to content

Conversation

@carlosmiei
Copy link
Contributor

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds two new order update methods to support Binance's order cancel/replace functionality: updateOrder for spot trading and futuresUpdateOrder for futures trading. These methods enable users to modify existing orders atomically, addressing the feature request in issue #582.

Key Changes:

  • Added updateOrder method that calls the /api/v3/order/cancelReplace endpoint
  • Added futuresUpdateOrder method that calls the /fapi/v1/order PUT endpoint with placeholder logic for conditional orders

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +604 to +607
// for now it is not supported
// const payloadCopy = { ...payload }
// delete payloadCopy.conditional
// return privCall('/fapi/v1/algoOrder', payloadCopy, 'PUT')
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

Commented-out code should be removed. If this functionality is planned for future implementation, consider creating a TODO comment or GitHub issue instead of leaving commented code in the codebase.

Copilot uses AI. Check for mistakes.

// Order endpoints
order: payload => order(privCall, payload, '/api/v3/order'),
updateOrder: payload => privCall('/api/v3/order/cancelReplace', payload, 'POST'),
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The updateOrder endpoint lacks test coverage. Other order endpoints in test/orders.js have corresponding tests (e.g., orderTest, getOrder, cancelOrder). Consider adding tests for updateOrder to verify parameter handling, error cases, and successful order updates using the /api/v3/order/cancelReplace endpoint.

Copilot uses AI. Check for mistakes.
Comment on lines +602 to +610
futuresUpdateOrder: payload => {
if (payload && 'conditional' in payload) {
// for now it is not supported
// const payloadCopy = { ...payload }
// delete payloadCopy.conditional
// return privCall('/fapi/v1/algoOrder', payloadCopy, 'PUT')
}
return privCall('/fapi/v1/order', payload, 'PUT')
},
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The futuresUpdateOrder endpoint lacks test coverage. Other futures order endpoints in test/futures.js have corresponding tests (e.g., futuresOrder, futuresGetOrder, futuresCancelOrder). Consider adding tests for futuresUpdateOrder to verify parameter handling, error cases, and successful order updates using the /fapi/v1/order PUT endpoint.

Copilot uses AI. Check for mistakes.
Comment on lines +602 to +608
futuresUpdateOrder: payload => {
if (payload && 'conditional' in payload) {
// for now it is not supported
// const payloadCopy = { ...payload }
// delete payloadCopy.conditional
// return privCall('/fapi/v1/algoOrder', payloadCopy, 'PUT')
}
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The conditional check at line 603 does not perform any action when 'conditional' is present in the payload. The if block is empty (only contains commented code), so the function will continue to line 609 regardless. Either implement the conditional order update logic or remove this placeholder code.

Copilot uses AI. Check for mistakes.
Comment on lines +602 to +610
futuresUpdateOrder: payload => {
if (payload && 'conditional' in payload) {
// for now it is not supported
// const payloadCopy = { ...payload }
// delete payloadCopy.conditional
// return privCall('/fapi/v1/algoOrder', payloadCopy, 'PUT')
}
return privCall('/fapi/v1/order', payload, 'PUT')
},
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The futuresUpdateOrder implementation is inconsistent with futuresGetOrder and futuresCancelOrder patterns. Both of those methods handle conditional orders by checking the conditional flag and properly routing to the algo endpoint, and they also create a payloadCopy to remove the 'conditional' property before making the API call. Consider implementing similar logic here for consistency.

Copilot uses AI. Check for mistakes.

// Order endpoints
order: payload => order(privCall, payload, '/api/v3/order'),
updateOrder: payload => privCall('/api/v3/order/cancelReplace', payload, 'POST'),
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

Consider adding parameter validation or using a helper function similar to the 'order' helper used for the order endpoint. The order endpoint validates required parameters like 'symbol' and 'side' and handles automatic newClientOrderId generation. The updateOrder endpoint likely has similar requirements that should be validated.

Copilot uses AI. Check for mistakes.
@carlosmiei carlosmiei self-assigned this Dec 17, 2025
@carlosmiei carlosmiei merged commit 74eca0a into ccxt:master Dec 17, 2025
1 check passed
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.

1 participant