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 MultiAssetModeService for Future #487

Merged
merged 2 commits into from
May 20, 2023

Conversation

jeonghoyeo7
Copy link
Contributor

@jeonghoyeo7 jeonghoyeo7 commented May 16, 2023

I found the endpoint /fapi/v1/multiAssetsMargin is not supported yet to get or change the multi-asset mode of a user.
I added the related services accordingly in the following files in v2/futures.

  • client.go
  • position_service.go
  • position_service_test.go

@jeonghoyeo7
Copy link
Contributor Author

Hello @adshao .
Could you take a time to review this PR?
Thank you!

// ChangeMultiAssetModeService change user's multi-asset mode
type ChangeMultiAssetModeService struct {
c *Client
multiAssetsMargin string
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
multiAssetsMargin string
multiAssetsMargin bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I referred https://binance-docs.github.io/apidocs/futures/en/#change-multi-assets-mode-trade, where it is seen as
POST /fapi/v1/multiAssetsMargin (HMAC SHA256) has the parameter as below.
multiAssetsMargin | STRING | YES | "true": Multi-Assets Mode; "false": Single-Asset Mode

I follow the Binance api doc. When getting multi-asset mode, the response is bool type, but when we change by POST, its parameter multi-asset mode is string type.

Comment on lines 223 to 227
if multiAssetsMargin {
s.multiAssetsMargin = "true"
} else {
s.multiAssetsMargin = "false"
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if multiAssetsMargin {
s.multiAssetsMargin = "true"
} else {
s.multiAssetsMargin = "false"
}
s.multiAssetsMargin = multiAssetsMargin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above.

@adshao
Copy link
Owner

adshao commented May 17, 2023

Hello @adshao . Could you take a time to review this PR? Thank you!

Thank you for the work, please check my review comments.

@jeonghoyeo7
Copy link
Contributor Author

Thank you for your review and comments.

In summary, from https://binance-docs.github.io/apidocs/futures/en/#change-multi-assets-mode-trade, where it is seen as
POST /fapi/v1/multiAssetsMargin (HMAC SHA256) has the parameter as below.
multiAssetsMargin | STRING | YES | "true": Multi-Assets Mode; "false": Single-Asset Mode

Following the above link, it seems multiAssetsMargin, which will become s.multiAssetsMargin, should be a string.

@adshao
Copy link
Owner

adshao commented May 18, 2023

Thank you for your review and comments.

In summary, from https://binance-docs.github.io/apidocs/futures/en/#change-multi-assets-mode-trade, where it is seen as POST /fapi/v1/multiAssetsMargin (HMAC SHA256) has the parameter as below. multiAssetsMargin | STRING | YES | "true": Multi-Assets Mode; "false": Single-Asset Mode

Following the above link, it seems multiAssetsMargin, which will become s.multiAssetsMargin, should be a string.

setFormParams will automatically convert bool to string before submitting the form.

@jeonghoyeo7
Copy link
Contributor Author

jeonghoyeo7 commented May 18, 2023

Thanks for further explanation.

As I reviewed ChangePositionModeService and DualSide function, ChangePositionModeService has the same case as MultiAssetModeService in that they have the same type parameter of string.

Is setFormParams applied only to MultiAssetModeService or both cases? Then, does the following code need to be modified from

func (s *ChangePositionModeService) DualSide(dualSide bool) *ChangePositionModeService {
	if dualSide {
		s.dualSide = "true"
	} else {
		s.dualSide = "false"
	}
	return s
}

to

func (s *ChangePositionModeService) DualSide(dualSide bool) *ChangePositionModeService {
	s.dualSide = dualSide
	return s
}

? But, after reviewing

r.setFormParams(params{
		"dualSidePosition": s.dualSide,
	})
```,
the existing part related to ChangePositionModeService looks correct.

It seems because of the following part,

r.setFormParams(params{
"multiAssetsMargin": s.multiAssetsMargin,
})

`s.multiAssetsMargin` seems to be required to be string.

@adshao
Copy link
Owner

adshao commented May 20, 2023

Thanks for further explanation.

As I reviewed ChangePositionModeService and DualSide function, ChangePositionModeService has the same case as MultiAssetModeService in that they have the same type parameter of string.

Is setFormParams applied only to MultiAssetModeService or both cases? Then, does the following code need to be modified from

func (s *ChangePositionModeService) DualSide(dualSide bool) *ChangePositionModeService {
	if dualSide {
		s.dualSide = "true"
	} else {
		s.dualSide = "false"
	}
	return s
}

to

func (s *ChangePositionModeService) DualSide(dualSide bool) *ChangePositionModeService {
	s.dualSide = dualSide
	return s
}

? But, after reviewing

r.setFormParams(params{
		"dualSidePosition": s.dualSide,
	})
```,
the existing part related to ChangePositionModeService looks correct.

It seems because of the following part,

r.setFormParams(params{ "multiAssetsMargin": s.multiAssetsMargin, })

`s.multiAssetsMargin` seems to be required to be string.

I think DualSide was added before setFormParams supporting bool directly. You can just verify it in your unit tests.

@jeonghoyeo7
Copy link
Contributor Author

@adshao Thanks a lot for your comment.
I revised the multiAssetMode related as your suggestion.
And also, I further changed ChangePositionModeService for DualSide similarly.

type ChangePositionModeService struct {
	c        *Client
	dualSide bool
}

// Change user's position mode: true - Hedge Mode, false - One-way Mode
func (s *ChangePositionModeService) DualSide(dualSide bool) *ChangePositionModeService {
	s.dualSide = dualSide
	return s
}

I found setFormParams is also applied to this by

r.setFormParams(params{
	"dualSidePosition": s.dualSide,
})

I checked the related unit tests are all passed.

@adshao adshao merged commit eebc3b1 into adshao:master May 20, 2023
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.

None yet

2 participants