(BTC Markets): Wrapper SubmitOrder parameter order fix & IsOrderPlaced condition correction #394

xtda · 2019-12-10T23:53:48Z

PR Description

Corrects parameter order for BTCMarkets SubmitOrder() wrapper to NewOrder() Method
(OrderType & OrderSide were incorrect)

Also fixes the condition check on IsOrderPlaced in order manager Submit() Method that will cause it to return order cannot be placed error if IsOrderPlaced is true

Type of change

Bug fix (non-breaking change which fixes an issue)

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

go test ./... -race
golangci-lint run

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Travis with my changes
Any dependent changes have been merged and published in downstream modules

…r IsOrderPlaced

codecov · 2019-12-11T00:03:05Z

Codecov Report

Merging #394 into master will decrease coverage by 0.03%.
The diff coverage is 15.66%.

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage    41.4%   41.36%   -0.04%     
==========================================
  Files         163      163              
  Lines       38341    38366      +25     
==========================================
- Hits        15874    15870       -4     
- Misses      21352    21372      +20     
- Partials     1115     1124       +9

Impacted Files	Coverage Δ
exchanges/btcmarkets/btcmarkets.go	`23.76% <ø> (ø)`	⬆️
engine/timekeeper.go	`0% <0%> (ø)`	⬆️
exchanges/btcmarkets/btcmarkets_wrapper.go	`25.05% <0%> (-0.98%)`	⬇️
engine/orders.go	`0% <0%> (ø)`	⬆️
common/common.go	`89.22% <100%> (+0.61%)`	⬆️
ntpclient/ntpclient.go	`37.5% <33.33%> (-40.28%)`	⬇️
database/models/sqlite3/boil_types.go	`66.66% <0%> (-33.34%)`	⬇️
database/models/postgres/psql_upsert.go	`91.17% <0%> (-8.83%)`	⬇️
dispatch/dispatch.go	`86.15% <0%> (-2.57%)`	⬇️
database/models/postgres/audit_event.go	`51.84% <0%> (-1.31%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 473092a...f821351. Read the comment docs.

thrasher-

Very nice find! tACK

thrasher- · 2019-12-11T01:44:15Z

Just realised that there's another bug: https://github.com/thrasher-corp/gocryptotrader/blob/master/exchanges/btcmarkets/btcmarkets_wrapper.go#L547

This will return all orders (inactive/cancelled etc) so we can specify "open" as the last param to retrieve only open orders

xtda · 2019-12-11T03:51:15Z

Just realised that there's another bug: https://github.com/thrasher-corp/gocryptotrader/blob/master/exchanges/btcmarkets/btcmarkets_wrapper.go#L547

This will return all orders (inactive/cancelled etc) so we can specify "open" as the last param to retrieve only open orders

Addressed this and some other issues with GetActiveOrders()

At quick glance GetOrderHistory() also needs to be reworked will push through another update soon and get you to rereview it

BTC markets batch end points have limits (20 for cancel 50 for query) adds new method SplitStringSliceByLimit in common to split a slice by limit and return slice of slice

MadCozBadd

:D

thrasher-

Thanks for fixing these issues! Just have 2 basic nits

common/common.go

thrasher- · 2019-12-12T04:40:41Z

exchanges/btcmarkets/btcmarkets_wrapper.go

 			tempResp.OrderSide = order.Bid
 			if tempData[y].Side == ask {
 				tempResp.OrderSide = order.Ask
 			}
 			tempResp.OrderDate = tempData[y].CreationTime
+
+			switch tempData[y].Type {
+			case "Limit":


Can use consts:

gocryptotrader/exchanges/btcmarkets/btcmarkets.go

Line 64 in 86c04d9

limit = "Limit"

…found but default to system

thrasher-

Thanks for making those changes! I just noticed another 2 but they're related to the ntpclient

thrasher- · 2019-12-12T21:32:46Z

ntpclient/ntpclient.go

@@ -38,7 +37,10 @@ func NTPClient(pool []string) (time.Time, error) {

 		defer con.Close()

-		con.SetDeadline(time.Now().Add(5 * time.Second))
+		err = con.SetDeadline(time.Now().Add(5 * time.Second))


Please change to

if err := con.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { log.Warnf(log.TimeMgr, "Unable to SetDeadline. Error: %s\n", err) con.Close() continue }

The other thing I noticed with this is we are deferring con.Close() but on each for loop iteration, we are reassigning con. The defer will only execute upon func exit, so it's possible for lingering cons

thrasher- · 2019-12-12T21:33:50Z

ntpclient/ntpclient.go

@@ -55,5 +57,6 @@ func NTPClient(pool []string) (time.Time, error) {

 		return time.Unix(int64(secs), nanos), nil
 	}
-	return time.Unix(0, 0), errors.New("no valid time servers")
+	log.Warnln(log.Global, "No valid ntp server found returning current system time")


Please use log.TimeMgr here, and just a basic grammar fix:

No valid NTP servers found, using current system time

thrasher- · 2019-12-12T23:06:23Z

ntpclient/ntpclient.go

@@ -30,30 +29,36 @@ type ntppacket struct {
 // NTPClient create's a new NTPClient and returns local based on ntp servers provided timestamp
 func NTPClient(pool []string) (time.Time, error) {


Since we don't use err, we could do a check at the top to check if len(pool) == 0 and complain if it's empty

I will remove the error return value
a pool should never be empty because of https://github.com/thrasher-corp/gocryptotrader/blob/master/config/config.go#L1250

thrasher- · 2019-12-12T23:08:32Z

exchanges/btcmarkets/btcmarkets_wrapper.go

 	if len(req.Currencies) == 0 {
-		allPairs := b.GetEnabledPairs(asset.Spot)
+		allPairs := b.GetAvailablePairs(asset.Spot)


We should keep this as GetEnabledPairs, so that we're only interested in the ones the user has enabled (otherwise they can set it in req.Currencies). Our order manager will use enabled pairs in a future update.

…if no server can be reached

thrasher-

Thanks for making those requested changes!

shazbert

Epic changes - Few nits

shazbert · 2019-12-12T23:07:53Z

common/common_test.go

+	slice50 := initStringSlice(t, 50)
+	out := SplitStringSliceByLimit(slice50, 20)
+	if len(out) != 3 {
+		t.Errorf("test failed expected len() to be 3 instead received: %v", len(out))


I think we got rid of test failed strings. Might do that as well.

shazbert · 2019-12-12T23:25:43Z

common/common_test.go

@@ -499,3 +500,30 @@ func TestChangePermission(t *testing.T) {
 		}
 	}
 }
+
+func initStringSlice(t *testing.T, size int) (out []string) {
+	t.Helper()


There's no error messages here so this can be rm'd

Helper() is called here to cause this function to be ignored when printing out warnings/errors/line information

It actually shifts the error to the calling function line if it has a t.Error or t.Fatal, it doesn't add anything to the output.

for example:

func initStringSlice(t *testing.T, size int) (out []string) { t.Helper() for x := 0; x < size; x++ { out = append(out, "gct-"+strconv.Itoa(x)) } t.Error("Cats") return }

Will indicate error at ln 514 instead of ln 509

shazbert · 2019-12-13T00:30:20Z

ntpclient/ntpclient_test.go

-	_, err = NTPClient(firstInvalid)
-	if err != nil {
-		t.Errorf("failed to get time %v", err)
+	if reflect.TypeOf(v) != reflect.TypeOf(time.Time{}) {


This will always be a time.Time so can RM this check. Or could force a time.Now().UTC() check?

Honestly this was just a lazy way to maintain test coverage to at least know its returning a time.Now()

I can do a time.Now.UTC() check but i would have to also parse the time into a format that doen't include anything above milliseconds or return value of NTPClient and time.Now.UTC() will be out and cause a failure

Another one is just comparing the year/day/month is the same, that's a good enough check

I kept the type check because you can never have to much test coverage right :D?
Also added a check for IsZero() and using 2006-01-02T15:04:05 format

shazbert

Thanks for making those changes!

MadCozBadd

Reapproved!

thrasher-

reACK!

…d condition correction (thrasher-corp#394) * corrected param order Side -> Type, also corrected condition check for IsOrderPlaced * send open status for GetActiveOrders * GetActiveOrder() changes to include OrderID and status matching * BTC Markets batch order limit fixes & SplitStringSliceByLimit method BTC markets batch end points have limits (20 for cancel 50 for query) adds new method SplitStringSliceByLimit in common to split a slice by limit and return slice of slice * rm line :D * Added test for SplitStringSliceByLimit and moved to const * ntp client reworked to not return error if no valid time servers are found but default to system * clean up * new line added * use TimeMgr sublogger and wording correction on output * Moved to DialTimeout() & Removed SetDeadline call * removed line * added setdeadline fix * goimport file * removed unused error from NTPClient as we now default to system time if no server can be reached * Added checks for number overflows * converted to uint as you should not be passing a negative number in * Increased test cases for NTPClient * Removed Helper call as no longer outputting any data from function * removed unused param

corrected param order Side -> Type, also corrected condition check fo…

199d89b

…r IsOrderPlaced

xtda added the bug label Dec 10, 2019

xtda requested review from thrasher-, shazbert and MadCozBadd December 10, 2019 23:53

xtda self-assigned this Dec 10, 2019

thrasher- approved these changes Dec 11, 2019

View reviewed changes

send open status for GetActiveOrders

145f329

xtda added 3 commits December 11, 2019 15:02

GetActiveOrder() changes to include OrderID and status matching

76622bd

BTC Markets batch order limit fixes & SplitStringSliceByLimit method

5dcaa3f

BTC markets batch end points have limits (20 for cancel 50 for query) adds new method SplitStringSliceByLimit in common to split a slice by limit and return slice of slice

rm line :D

86c04d9

xtda requested a review from thrasher- December 11, 2019 22:38

MadCozBadd approved these changes Dec 11, 2019

View reviewed changes

thrasher- requested changes Dec 12, 2019

View reviewed changes

xtda added 4 commits December 13, 2019 08:01

Added test for SplitStringSliceByLimit and moved to const

63e15d5

ntp client reworked to not return error if no valid time servers are …

0845199

…found but default to system

clean up

114830f

new line added

36241d3

thrasher- requested changes Dec 12, 2019

View reviewed changes

xtda added 5 commits December 13, 2019 08:37

use TimeMgr sublogger and wording correction on output

eeadb7d

Moved to DialTimeout() & Removed SetDeadline call

343b460

removed line

91149c4

added setdeadline fix

c498e40

goimport file

f959bea

thrasher- reviewed Dec 12, 2019

View reviewed changes

removed unused error from NTPClient as we now default to system time …

bb1c601

…if no server can be reached

thrasher- approved these changes Dec 13, 2019

View reviewed changes

shazbert requested changes Dec 13, 2019

View reviewed changes

xtda added 3 commits December 13, 2019 11:50

Added checks for number overflows

0b431ac

converted to uint as you should not be passing a negative number in

1520360

Increased test cases for NTPClient

e5e4909

shazbert approved these changes Dec 13, 2019

View reviewed changes

xtda added 2 commits December 13, 2019 15:14

Removed Helper call as no longer outputting any data from function

cdadfe6

removed unused param

f821351

MadCozBadd approved these changes Dec 16, 2019

View reviewed changes

thrasher- approved these changes Dec 16, 2019

View reviewed changes

thrasher- merged commit a727bee into thrasher-corp:master Dec 16, 2019

xtda deleted the btcmarketsorderplacedfix branch February 9, 2020 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(BTC Markets): Wrapper SubmitOrder parameter order fix & IsOrderPlaced condition correction #394

(BTC Markets): Wrapper SubmitOrder parameter order fix & IsOrderPlaced condition correction #394

xtda commented Dec 10, 2019

codecov bot commented Dec 11, 2019 •

edited

thrasher- left a comment

thrasher- commented Dec 11, 2019

xtda commented Dec 11, 2019

MadCozBadd left a comment

thrasher- left a comment

thrasher- Dec 12, 2019

xtda Dec 12, 2019

thrasher- left a comment

thrasher- Dec 12, 2019

thrasher- Dec 12, 2019

thrasher- Dec 12, 2019

xtda Dec 12, 2019

thrasher- Dec 12, 2019

thrasher- left a comment

shazbert left a comment

shazbert Dec 12, 2019

shazbert Dec 12, 2019

xtda Dec 13, 2019

shazbert Dec 13, 2019

shazbert Dec 13, 2019

shazbert Dec 13, 2019

shazbert Dec 13, 2019

xtda Dec 13, 2019

thrasher- Dec 13, 2019

xtda Dec 13, 2019

shazbert left a comment

MadCozBadd left a comment

thrasher- left a comment

		@@ -30,30 +29,36 @@ type ntppacket struct {
		// NTPClient create's a new NTPClient and returns local based on ntp servers provided timestamp
		func NTPClient(pool []string) (time.Time, error) {

(BTC Markets): Wrapper SubmitOrder parameter order fix & IsOrderPlaced condition correction #394

(BTC Markets): Wrapper SubmitOrder parameter order fix & IsOrderPlaced condition correction #394

Conversation

xtda commented Dec 10, 2019

PR Description

Type of change

How has this been tested

Checklist

codecov bot commented Dec 11, 2019 • edited

Codecov Report

thrasher- left a comment

Choose a reason for hiding this comment

thrasher- commented Dec 11, 2019

xtda commented Dec 11, 2019

MadCozBadd left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

MadCozBadd left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2019 •

edited