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

v2.10.4: add some missing simple earn endpoints #419

Merged
merged 5 commits into from
Apr 3, 2024

Conversation

SebastianBoehler
Copy link
Contributor

Summary

Added some simple earn endpoints as I saw #386
I am confused on how to submit params for get request, proceeded with query params but there is some inconsistency across methods. sometimes as body, sometimes as url query. According to docs for get request it must be query string

Additional Information

Anyone feel free to contribute more, I've just added about half of the simple earn endpoints, those are important to me.

Tried to not include breaking changes and kept naming of existing methods, tho types were outdated, not accurate on some of them

Copy link
Owner

@tiagosiebler tiagosiebler left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! For GET requests, simply pass any params as an object (in the second param of the getPrivate(url, params) call) - the networking abstraction will automatically handle serialisation etc while building the request. Here's an example from the master branch:
https://github.com/tiagosiebler/binance/blob/master/src/main-client.ts#L259-L261

src/main-client.ts Outdated Show resolved Hide resolved
@SebastianBoehler
Copy link
Contributor Author

By merging/approving #420 first in case my commit was the only thing preventing it from merging we could have both changes in the version bump from this PR

**/

getFlexibleSavingProducts(
params: SimpleEarnProductListParams,
Copy link
Owner

Choose a reason for hiding this comment

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

If all properties on this object are optional, then "params" should be optional too. Same for any other calls with fully optional params.

Suggested change
params: SimpleEarnProductListParams,
params?: SimpleEarnProductListParams,

The SDK will automatically see that this is undefined and will make the request without any params (and it looks cleaner in code).

Comment on lines 86 to 89
export interface SimpleEarnLockedProductPositionParams
extends SimpleEarnFlexibleProductPositionParams {
positionId?: string;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Since it's only 4 extra params (not 10-50), I would prefer a flatter view, even if it means a little more repetition:

Suggested change
export interface SimpleEarnLockedProductPositionParams
extends SimpleEarnFlexibleProductPositionParams {
positionId?: string;
}
export interface SimpleEarnLockedProductPositionParams {
asset?: string;
productId?: string;
current?: number;
size?: number;
positionId?: string;
}

What might start with one simple extends can quickly become a mess that's hard to follow around (for some).

Copy link
Owner

Choose a reason for hiding this comment

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

This one's optional too, as well as SimpleEarnFlexibleProductPositionParams, so just make sure to make the param optional in the function call that has this interface

Copy link
Owner

@tiagosiebler tiagosiebler left a comment

Choose a reason for hiding this comment

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

Some changes. Pls double check which params should be optional

Comment on lines 86 to 89
export interface SimpleEarnLockedProductPositionParams
extends SimpleEarnFlexibleProductPositionParams {
positionId?: string;
}
Copy link
Owner

Choose a reason for hiding this comment

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

This one's optional too, as well as SimpleEarnFlexibleProductPositionParams, so just make sure to make the param optional in the function call that has this interface

Comment on lines 1215 to 1220
getSimpleEarnAccount(params: {
recvWindow?: number;
timestamp: number;
}): Promise<SimpleEarnAccountResponse> {
return this.getPrivate(`/sapi/v1/simple-earn/account`, params);
}
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
getSimpleEarnAccount(params: {
recvWindow?: number;
timestamp: number;
}): Promise<SimpleEarnAccountResponse> {
return this.getPrivate(`/sapi/v1/simple-earn/account`, params);
}
getSimpleEarnAccount(): Promise<SimpleEarnAccountResponse> {
return this.getPrivate(`/sapi/v1/simple-earn/account`);
}

@SebastianBoehler
Copy link
Contributor Author

All done :)

Copy link
Owner

@tiagosiebler tiagosiebler left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the updates!

@tiagosiebler tiagosiebler merged commit 05dd466 into tiagosiebler:master Apr 3, 2024
3 checks passed
@tiagosiebler
Copy link
Owner

Released :)

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