exchanges/order: Added TimeInForce type and values #1382

samuael · 2023-10-29T18:59:45Z

ሰላም፣ ወንድሞች

PR Description

This PR adds a type TimeInForce(TIF) to represent common Time In Force values. It affects some exchanges and packages that were using the ImmediateOrCancel and FillOrKill values. Units that were affected by the change are tested. Additionally, new unit tests are added for newly added functions.

Exchanges that are directly affected by the change are Binance, Bitrex, BTCMarket, CoinbasePro, Huobi, Kraken, Poloniex

Packages affected: Orders, Engine, cmd/exchange_wrapper_standards

Fixes # (issue)

Type of change

Please delete options that are not relevant and add an x in [] as the item is complete.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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
Test X

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented on 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 Github Actions/AppVeyor with my changes
Any dependent changes have been merged and published in downstream modules

shazbert

Really cool update. Thanks big boss.

exchanges/order/orders.go

exchanges/order/order_types.go

exchanges/bittrex/bittrex_types.go

… into update_tif

shazbert

Thanks for the changes. 🦸

exchanges/order/order_types.go

exchanges/order/orders.go

exchanges/order/order_test.go

samuael · 2023-11-15T11:59:11Z

Sorry for the delayed update on this PR.
I think I have added all the required updates.

… into update_tif

shazbert

Thanks for the update, not much left over.

exchanges/order/order_types.go

exchanges/order/orders.go

exchanges/order/order_types.go

… into update_tif

shazbert

Thanks for the changes. Things are looking good. Just need to test all endpoints associated with these changes.

… into update_tif

samuael · 2023-12-06T18:46:39Z

The endpoints I can run are only those that are public. The change I made, TimeInForce is mostly used by the authenticated, order-related endpoints.

I am open to hear any other alternative of running the authenticated tests you may suggest

… into update_tif

gloriousCode

Thanks for updating things!

gloriousCode · 2025-01-24T00:19:24Z

exchanges/order/order_types.go

+	ImmediateOrCancel    bool
+	FillOrKill           bool


Any reason to keep fields ImmediateOrCancel and FillOrKill with your new TimeInForce?

and post only.

gloriousCode · 2025-01-24T00:20:53Z

exchanges/gateio/gateio_test.go

@@ -3437,7 +3437,7 @@ func TestGetTimeInForce(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "gtc", ret)

-	ret, err = getTimeInForce(&order.Submit{Type: order.Market, FillOrKill: true})
+	ret, err = getTimeInForce(&order.Submit{Type: order.Market, TimeInForce: order.FOK})


This is having issue I think
https://github.com/thrasher-corp/gocryptotrader/actions/runs/12864603088/job/35863457166?pr=1382#step:10:430

… into update_tif # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

… into update_tif

shazbert

Nice direction and clean. Just some nits and suggestions. Thanks heaps!

shazbert · 2025-02-20T22:40:46Z

exchanges/order/order_types.go

-	ImmediateOrCancel bool
-	FillOrKill        bool
+	// TimeInForce holds time in force values
+	TimeInForce TimeInForce

 	PostOnly bool


Post only can be merged with TimeInForce across all order types as it conflicts with FOK, IOC and it implies limit order type

Post-only can be a value for either the Order Type or the Time In Force type. Some exchanges have a dedicated PostOnly field, and in such cases, we need to check whether either the Time In Force or the Order Type is set to Post-only. I left this as it is to avoid making that decision.

Post-only can be used with GTT, GTC, or GTD, and having it as a parameter here is restrictive. Some exchanges have TimeInForce and PostOnly as separate parameters.

Suggestion: You can either flag TimeInForce values order.GTT | order.POSTONLY or you can merge the entire TimeInForce values and flag everything together into order.Type - order.Limit | order.GTT | order.PostOnly and with both options update to parser functions to allow for delimited string values and methods for extraction. I will have a play around today and see if there are any caveats.

Merging them altogether into order type is a bit of a mission. And can be circled around in a future update. In your other comment:

FOK and IOC has an order type replica.. So, I am not renaming these values.
However, I have renamed the rest

suggestion: Seems like a time in force policy as well as PostOnly and that should be removed from order types and moved to TIF. Could have a method Is(order.FillOrKill) bool and Is(order.InstanceOrCancelled) bool Just so its an easy catch all expression and self obvious.

There are a bunch of trigger conditionals in order type StopLimit which could be improved as it should just be Stop | Limit as well as SpecialMode/exchange specific e.g. TWAP or MarketMakerProtection (which has post only variant which can be removed and handled) but that is out of scope.

So just keeping it time in force is a nice reviewable action but I am open to other perspectives and direction. If we proceed to merge them altogether we add in edge cases where they cannot be combined but if we refactor to keep them separate like

type Order struct { Type OrderType Condition ConditionType TimeInForce TimeInForce SpecialMode SpecialMode }

might be impractical.

Sorry for delayed reply to this comments & Pardon me if I don't fully understand what you are saying here.

I have a point here: Three exchanges Okx, CoinbasePro, Deribit, and Huobi use PostOnly as an independent Order Type too. How do we handle these?

As it has to be directly mapped in the wrapper file between order.Type and String representation.

The same applies to FillOrKill and ImmediateOrCancel

Thanks Boss 👑. You would apply the same logic their end but from a submission perspective from the IBotInterface/wrapper funcs, an order would apply order.Limit as type and for the TimeInForce value as Post Only or FillOrKIll or Immediate Or Cancel we can extract the string that we need from that. That way we standardise the layer on our side and we segregate primary areas of concern. I will have a play today and see if that can be done while you are offline so I can give a concrete example.

shazbert · 2025-02-20T22:43:00Z

exchanges/order/order_types.go

+	POC                                 // POC represents PendingOrCancel
+	PostOnlyGTC                         // PostOnlyGCT represents PostOnlyGoodTilCancelled


POC and PostOnlyGTC can merged to PostOnly and all Abbreviations can be changed from GTC -> GoodTillCancel and comments removed etc.

FOK and IOC has an order type replica.. So, I am not renaming these values.
However, I have renamed the rest

exchanges/kucoin/kucoin_wrapper.go

exchanges/order/order_test.go

exchanges/order/orders.go

shazbert · 2025-02-20T23:28:30Z

exchanges/order/orders.go

@@ -1234,6 +1254,37 @@ func StringToOrderStatus(status string) (Status, error) {
 	}
 }

+// StringToTimeInForce converts time in force string value to TimeInForce instance.
+func StringToTimeInForce(timeInForce string) (TimeInForce, error) {


suggestion: This is a good easy catch all but it adds bloat and reduced performance there should be an umarshal json method or function to a type exchange side. So suggestion would be to remove this completely. This is just a suggestion doesn't need to be done.

Let's keep it here and remove it for when we need the UnmarshalJSON method rather than this one

exchanges/poloniex/poloniex_wrapper.go

… into update_tif

shazbert · 2025-02-28T01:17:05Z

Please review and merge/cherry-pick changes from this commit @samuael boss this is regarding our conversation here

samuael · 2025-02-28T21:11:24Z

Please review and merge/cherry-pick changes from this commit @samuael boss this is regarding our conversation here

I have done so and applied the changes. I will try to do further updates based on your recent approach.
Thank you @shazbert boss!

shazbert

Thanks for the changes, Heres some more nits for current state.

shazbert · 2025-03-03T00:18:41Z

exchanges/binance/binance_wrapper.go

 		var requestParamsOrderType RequestParamsOrderType
 		switch s.Type {
 		case order.Market:
 			timeInForce = ""
 			requestParamsOrderType = BinanceRequestParamsOrderMarket
 		case order.Limit:
-			if s.ImmediateOrCancel {
-				timeInForce = BinanceRequestParamsTimeIOC
+			if s.TimeInForce == order.ImmediateOrCancel {


Can use IS method

shazbert · 2025-03-03T00:20:24Z

exchanges/binance/binance_test.go

@@ -1420,7 +1420,7 @@ func TestNewOrderTest(t *testing.T) {
 		TradeType:   BinanceRequestParamsOrderLimit,
 		Price:       0.0025,
 		Quantity:    100000,
-		TimeInForce: BinanceRequestParamsTimeGTC,


Suggestion: Binance request params can be removed?

shazbert · 2025-03-03T00:22:17Z

exchanges/btcmarkets/btcmarkets.go

-	}
-	if s.FillOrKill {
-		return fillOrKill
+	switch s.TimeInForce {


Can use method Is

shazbert · 2025-03-03T00:23:08Z

exchanges/coinbasepro/coinbasepro_wrapper.go

-		if s.ImmediateOrCancel {
-			timeInForce = CoinbaseRequestParamsTimeIOC
+		timeInForce := order.GoodTillCancel.String()
+		if s.TimeInForce == order.ImmediateOrCancel {


Use method Is

shazbert · 2025-03-03T00:24:43Z

exchanges/deribit/deribit_wrapper.go

-	timeInForce := ""
-	if s.ImmediateOrCancel {
+	var timeInForce string
+	switch s.TimeInForce {


Use method Is as we are flagging and can be order.GoodTillDay | order.PostOnly for example.

shazbert · 2025-03-03T00:43:47Z

exchanges/order/order_types.go

+type TimeInForce uint16
+
+// Is checks to see if the enum contains the flag
+func (t TimeInForce) Is(in TimeInForce) bool {


PLease have a test for this and remove IsIOC method as this is more generic for that application.

shazbert · 2025-03-03T00:44:07Z

exchanges/order/order_types.go

+	FillOrKill        // FOK represents FillOrKill
+	ImmediateOrCancel // IOC represents ImmediateOrCancel
+	PostOnly          // PostOnlyGCT represents PostOnlyGoodTilCancelled


remove commentary

shazbert · 2025-03-03T00:44:36Z

exchanges/order/orders.go

+}
+
+// IsIOC determines whether the TimeInForce value is set to IOC (Immediate or Cancel).
+func (t TimeInForce) IsIOC() bool {


shazbert · 2025-03-03T00:45:56Z

exchanges/order/orders.go

@@ -740,6 +718,33 @@ func (t Type) String() string {
 	}
 }

+// String implements the stringer interface.
+func (t TimeInForce) String() string {


Because this can be flagged with Post only and GTT, GTD, GTC, it needs to be able to be a delimitated string and umarshal multiple options. Please add a test.

shazbert · 2025-03-03T00:46:14Z

exchanges/poloniex/poloniex_wrapper.go

@@ -549,7 +549,7 @@ func (p *Poloniex) ModifyOrder(ctx context.Context, action *order.Modify) (*orde
 		action.Price,
 		action.Amount,
 		action.PostOnly,
-		action.ImmediateOrCancel)
+		action.TimeInForce.IsIOC())


Use Is method

… into update_tif

samuael added 3 commits October 29, 2023 21:28

Added TimeInForce type and updated related files

5412e91

Linter issue fix and minor coinbasepro type update

0bbd6fb

Bitrex consts update

b786e75

gloriousCode requested a review from a team October 29, 2023 21:13

gloriousCode assigned samuael Oct 29, 2023

gloriousCode added the review me This pull request is ready for review label Oct 29, 2023

shazbert requested changes Oct 29, 2023

View reviewed changes

exchanges/order/orders.go Show resolved Hide resolved

exchanges/order/orders.go Outdated Show resolved Hide resolved

exchanges/order/order_types.go Outdated Show resolved Hide resolved

exchanges/bittrex/bittrex_types.go Outdated Show resolved Hide resolved

samuael added 2 commits October 30, 2023 16:28

added unit test and minor changes in bittrex

afa9695

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

cb2619b

… into update_tif

shazbert requested changes Nov 10, 2023

View reviewed changes

gloriousCode added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Nov 13, 2023

samuael added 4 commits November 15, 2023 15:08

Unit tests update

da4b3cb

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

e41ab86

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

c9b88ab

… into update_tif

Fix minor linter issues

e11beef

shazbert requested changes Nov 26, 2023

View reviewed changes

exchanges/order/order_types.go Outdated Show resolved Hide resolved

exchanges/order/orders.go Outdated Show resolved Hide resolved

exchanges/order/order_types.go Outdated Show resolved Hide resolved

samuael added 3 commits November 27, 2023 21:55

Update TestStringToTimeInForce unit test

9add848

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

aecc942

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

3a7ac71

… into update_tif

shazbert added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Dec 5, 2023

shazbert reviewed Dec 5, 2023

View reviewed changes

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

44f1cbb

… into update_tif

samuael added 4 commits December 22, 2023 15:48

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

964ed95

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

05b4921

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

2179454

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

3043702

… into update_tif

samuael added 2 commits January 15, 2025 14:59

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

c651a21

… into update_tif

Update order tests

f63547d

github-actions bot removed the stale label Jan 16, 2025

samuael added 2 commits January 16, 2025 17:01

Complete updating the order unit tests

28cb5f7

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

f708de2

… into update_tif

gloriousCode requested changes Jan 24, 2025

View reviewed changes

samuael added 8 commits January 31, 2025 11:24

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

9fe9504

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

6828343

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

41de4e8

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

84445fc

… into update_tif

update kucoin and deribit wrapper to match the time in force change

745977b

fix time-in-force related test errors

b05fe7f

linter issue fix

2085156

shazbert requested changes Feb 20, 2025

View reviewed changes

samuael added 2 commits February 24, 2025 23:46

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

de494ac

… into update_tif

time in force constants, functions and unit tests update

889c7cc

shift tif policies to TimeInForce

83b6521

shazbert requested changes Mar 3, 2025

View reviewed changes

samuael added 9 commits March 4, 2025 01:27

Update time-in-force, related functions, and unit tests

9bef751

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

18ed556

… into update_tif

fix linter issue and time-in-force processing

0fbaad8

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

892ff5e

… into update_tif

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

88584b8

… into update_tif

added a good till crossing tif value

0cf66a2

order type fix and fix related tim-in-force entries

1679be7

update time-in-force unmarshaling and unit test

3f752f2

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

8733ed4

… into update_tif

		POC // POC represents PendingOrCancel
		PostOnlyGTC // PostOnlyGCT represents PostOnlyGoodTilCancelled

exchanges/order: Added TimeInForce type and values #1382

Are you sure you want to change the base?

exchanges/order: Added TimeInForce type and values #1382

Conversation

samuael commented Oct 29, 2023

PR Description

Type of change

How has this been tested

Checklist

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

samuael commented Nov 15, 2023

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

samuael commented Dec 6, 2023

gloriousCode 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

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

samuael Feb 27, 2025 • edited Loading

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 commented Feb 28, 2025

samuael commented Feb 28, 2025

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

samuael Feb 27, 2025 •

edited

Loading