-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Fix to Binance dates appearing incorrect in Roundtrip info #1846
Conversation
Was trying to pass a millisecconds Unix timestamp with the moment.unix() function, when we just need to pass it straight in. This was causing dates in the Rountrips table to appear like 50046-01-30
Fix issue #1812
exchanges/binance.js
Outdated
@@ -255,7 +255,10 @@ Trader.prototype.getOrder = function(order, callback) { | |||
|
|||
var price = parseFloat(data.price); | |||
var amount = parseFloat(data.executedQty); | |||
var date = moment.unix(data.time); | |||
|
|||
//Data.time is a 13 digit millisecon unix time stamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after //
exchanges/binance.js
Outdated
var date = moment.unix(data.time); | ||
|
||
//Data.time is a 13 digit millisecon unix time stamp. | ||
//https://momentjs.com/docs/#/parsing/unix-timestamp-milliseconds/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after //
Awesome stuff, thanks! Can you look at the comments by @greenbigfrog, also the DEMA fix (see #1812) is about to be merged (making your script redundant), can you remove yours? |
This reverts commit fab67a5.
@askmike No problem, all done. |
👍 Thanks :) |
@askmike Should there rly be revert commits for commits introduced inside the same PR be included afterwards? IMO the history should just be rewritten, resulting to a lot less commits per PR (still have completely separate stuff in separate commits though) |
@greenbigfrog normally when the change is small I do merge all of these myself in a single "squash merge commit", however I forgot to do so for this PR. I don't feel comfortable doing it now on the live develop branch as I now a lot of people are using that branch. I will definitely keep a better watch for future PRs. Other projects (like bitcoin) have the policy of always doing squash merge commits for every PR, I am not sure if Gekko is there yet as this does mean you lose a lot of git history with bigger PRs. |
when changed date type in binance.js, does not trade anymore and process dies with error: |
@Erasss What branch are you on and what is your most recent commit you have? |
No other errors, i used stable version, changed date to unix now working trading. |
Detail logs, and then child dead: |
Its problem from price precision, not date. Received again in stable version |
@Erasss Yeah this is unrelated to the issue here.. Binance has recently changed the min buy amounts. The updates for this are on the develop branch and aren't on stable yet. But if you want to stay on stable you can update these values yourself. Have a look in the /exchanges/binance.js to be Give that a go.. |
Yes, i changed allready Thanks |
Was trying to pass a millisecconds Unix timestamp with the moment.unix() function, when we just need to pass it straight in. This was causing dates in the Rountrips table to appear like 50046-01-30
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fixes random roundtrip dates on binance
What is the current behavior? (You can also link to an open issue here)
Rountrips table dates appear like 50046-01-30
What is the new behavior (if this is a feature change)?
The correct dates are shown
Other information:
Issue documented here Binance: Rountrip entry and exit dates are broken #1687