Skip to content

Conversation

@AlexVanchov
Copy link

No description provided.

@dmzoneill
Copy link
Collaborator

hi, thanks for the work.
Could you kindly rebase; resolving the merge conflicts?

thanks

@dmzoneill dmzoneill mentioned this pull request Mar 27, 2021
@AlexVanchov
Copy link
Author

AlexVanchov commented Mar 27, 2021

I am still editing and adding futures for other functions. I will check out merge conflicts

* @return array with error message or the order details
*/
public function buy(string $symbol, $quantity, $price, string $type = "LIMIT", array $flags = [])
public function buy(string $symbol, $quantity, $price, string $type = "LIMIT", array $flags = [], $futures = false)
Copy link

@YetiCGN YetiCGN Mar 28, 2021

Choose a reason for hiding this comment

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

I think it would be better to use a new class FAPI or FuturesAPI than to do this with switches. Otherwise, if Margin API is introduced, do you want to be switching Margin and Futures on and off?

public function buy(string $symbol, $quantity, $price, string $type = "LIMIT", array $flags = [], $futures = false)
{
return $this->order("BUY", $symbol, $quantity, $price, $type, $flags);
return $this->order("BUY", $symbol, $quantity, $price, $type, $flags, false, $futures);
Copy link

Choose a reason for hiding this comment

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

Or at least use the $flags array to set 'fapi' => true and send it to the third parameter of httpRequest() like it's done with wapi and sapi already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm more for a separate class or at least separate functions like fBuy and/or fHttpRequest or whatever would fit best. Having the wapi and sapi is according to me already more then enough in httpRequest... (personal view!)

Copy link

Choose a reason for hiding this comment

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

Yeah, SAPI could be a separate class as well to make it cleaner. WAPI is deprecated as of today: https://www.binance.com/en/support/announcement/f45dde7da58b473aa885349946bed269

@AlexVanchov
Copy link
Author

I added FAPI class where all functions are the same but they are linked to futures endpoints. There is change in userData stream where you can add function which can be runner on connection to the WebSocket. You can review it now

@dmzoneill
Copy link
Collaborator

Can you please rebase?

@AlexVanchov
Copy link
Author

Can you please rebase?

I haven't used it before. I don't know if that should happen.

Command: git rebase
Result: Current branch master is up to date.

@YetiCGN
Copy link

YetiCGN commented Mar 29, 2021

Can you please rebase?

I haven't used it before. I don't know if that should happen.

Command: git rebase
Result: Current branch master is up to date.

You need to rebase onto jaggedsoft:master. You're likely doing the default where it's your fork's master.

@AlexVanchov
Copy link
Author

I rebased. Anything else to do?

@llomgui
Copy link

llomgui commented Apr 1, 2021

Hello @AlexVanchov,

Did you implement a getPositions function? I could not find it in changes.

@dmzoneill
Copy link
Collaborator

@AlexVanchov

did you push the changes to the merge request.. still tell me here there are maege conflicts

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

@dmzoneill
Copy link
Collaborator

@AlexVanchov

I would love to see this merge. can you please rebase, squash and push the commits?

@AlexVanchov
Copy link
Author

It still have some bugs with futures for some functions. I will pull request again when it's ready

@dmzoneill
Copy link
Collaborator

any update here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants