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 sub transfer history API #461

Closed
wants to merge 2 commits into from
Closed

Conversation

ngocthanh1389
Copy link

What is the problem?
Need to add new API to get transfer history

https://binance-docs.github.io/apidocs/spot/en/#transfer-to-master-for-sub-account

Copy link
Owner

@adshao adshao left a comment

Choose a reason for hiding this comment

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

Please add unit test as well, thanks!

@adshao
Copy link
Owner

adshao commented Mar 3, 2023

chatgpt

@adshao
Copy link
Owner

adshao commented Mar 3, 2023

Code Review: This pull request adds two new services to the Binance API client: SimpleEarnService and SubTransferHistoryService.

The SimpleEarnService service allows users to query their lending account history for subscription, redemption, and interest.

The SubTransferHistoryService service allows sub-account owners to query their sub-account asset transfer history.

Both services seem to be useful additions that provide more functionality to users of the Binance API client, so the changes seem good.

@adshao
Copy link
Owner

adshao commented Mar 9, 2023

chatgpt: please tell me if there is any improvement for the code:

${file:v2/simple_earn_service.go}

@adshao
Copy link
Owner

adshao commented Mar 9, 2023

AI Code Review:

The code seems okay but there is one possible improvement that can be made. The Size method is setting the size parameter to *s.current instead of *s.size. This could be a typo and should be corrected to avoid errors in the future.

The correct implementation should be:

func (s *SimpleEarnService) Size(size int64) *SimpleEarnService {
    s.size = &size
    return s
}

Another suggestion is to add error handling in case the endpoint is not set correctly in the switch statement. This would ensure that the code handles unexpected cases and returns a meaningful error message.

r.setParam("current", *s.current)
}
if s.size != nil {
r.setParam("size", *s.current)
Copy link
Owner

@adshao adshao Mar 9, 2023

Choose a reason for hiding this comment

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

Do you intend to use *s.size?

@adshao
Copy link
Owner

adshao commented Mar 9, 2023

chatgpt: please tell me if there is any improvement in following code:

${file:v2/subaccount_service.go}

@adshao
Copy link
Owner

adshao commented Mar 9, 2023

AI Code Review:

I cannot determine if there are any bugs in this code as it is only a code snippet and does not include any information about the variables or functions it interacts with.

@adshao
Copy link
Owner

adshao commented Mar 9, 2023

AI Code Review:

The code looks good, but here are a few minor suggestions:

  • Add comments to exported structs and methods.
  • Use camelCase for exported method names, i.e TransferType should be TransferType.
  • Use a pointer receiver for the Do method of SubTransferHistoryService to avoid copying the struct unnecessarily:
func (s *SubTransferHistoryService) Do(ctx context.Context) ([]*SubTransfer, error) {

Other than that, the code seems to follow Go's idiomatic style and is well-structured.

@Haiss2 Haiss2 deleted the sync branch October 13, 2023 08:52
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