Skip to content
This repository was archived by the owner on Oct 30, 2023. It is now read-only.

Conversation

@eugenpodaru
Copy link

What does this PR do?

  • Create separate classes for MarginNewOrder and MarginNewOrderResponse
  • Add SideEffectType to MarginNewOrder
  • Refactor some code.

Where should the reviewer start?

  • src/main/java/com/binance/api/client/domain/account/SideEffectType.java
  • src/main/java/com/binance/api/client/domain/account/MarginNewOrder.java
  • src/main/java/com/binance/api/client/domain/account/MarginNewOrderResponse.java
  • src/main/java/com/binance/api/client/impl/BinanceApiService.java
  • src/main/java/com/binance/api/client/impl/BinanceApiMarginRestClientImpl.java
  • src/main/java/com/binance/api/client/impl/BinanceApiAsyncMarginRestClientImpl.java

How should this be manually tested?

  • Placing a new margin order BinanceApiMarginRestClient.newOrder

Any background context you want to provide?

The reason I went with separate classes for MarginNewOrder and MarginNewOrderResponse is because adding the new properties to NewOrder and NewOrderResponse would have made they usage confusing. Besides sideEffectType, the MarginNewOrder should also have the isIsolated property, which is currently not implemented. The same holds for MarginNewOrderResponse. I did not inherit MarginNewOrder from NewOrder and MarginNewOrderResponse from NewOrderResponse since it's not clear that this is the relationship between those. I could have extracted the common properties to a base class, but did not really see the benefit.

See the documentation change log for when the change was done:

image

Copy link
Member

@joaopsilva joaopsilva left a comment

Choose a reason for hiding this comment

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

LGTM.

@joaopsilva joaopsilva merged commit 81a9be7 into binance-exchange:master Nov 16, 2020
@joaopsilva
Copy link
Member

Thanks!

@eugenpodaru eugenpodaru deleted the feature/add-side-effect-to-margin-order branch November 16, 2020 22:20
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.

2 participants