FEATURE: recover active orders with open orders periodically #1328

kbearXD · 2023-09-28T08:16:14Z

No description provided.

bbgokarma-bot · 2023-09-28T08:16:17Z

This pull request may get 322 BBG.

To receive BBG token, please left your polygon address as an issue comment in this pull request with the following format, e.g.,

polygon:0xAb5801a7D398351b8bE11C439e05C5B3259aeC9B

Once this pull request is merged, your BBG token will be sent to your wallet.

codecov · 2023-09-28T08:26:41Z

Codecov Report

Merging #1328 (ccb7308) into main (fb110a1) will increase coverage by 0.11%.
Report is 3 commits behind head on main.
The diff coverage is 41.60%.

@@            Coverage Diff             @@
##             main    #1328      +/-   ##
==========================================
+ Coverage   20.68%   20.79%   +0.11%     
==========================================
  Files         560      561       +1     
  Lines       40014    40105      +91     
==========================================
+ Hits         8276     8339      +63     
- Misses      31138    31160      +22     
- Partials      600      606       +6

Files	Coverage Δ
pkg/strategy/grid2/metrics.go	`87.93% <100.00%> (+0.89%)`	⬆️
pkg/strategy/grid2/strategy.go	`33.40% <0.00%> (+0.74%)`	⬆️
pkg/strategy/grid2/active_order_recover.go	`39.21% <39.21%> (ø)`

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

bbgokarma-bot · 2023-10-02T03:56:29Z

Re-estimated karma: this pull request may get 317 BBG

c9s · 2023-10-04T07:26:24Z

pkg/strategy/grid2/active_order_recover.go

+		}
+	}
+
+	ticker := time.NewTicker(util.MillisecondsJitter(40*time.Minute, 30*60*1000))


how did you decide the interval? does it mean 40minutes + 0~30minute jitter?

Yes, because I think we don't need to check it very frequently, so I wanna choose interval around 1 hour. To avoid DDOS, I use 0-30 min jitter.

gx578007 · 2023-10-04T08:13:00Z

pkg/strategy/grid2/active_order_recover.go

+	for _, activeOrder := range activeOrders {
+		if _, exist := openOrdersMap[activeOrder.OrderID]; !exist {
+			s.logger.Infof("find active order (%d) not in open orders, updating...", activeOrder.OrderID)
+			delete(openOrdersMap, activeOrder.OrderID)


OrderID looks not existing in the map? So the delete is redundant?

change to delete when exist

c9s · 2023-10-06T07:32:02Z

pkg/strategy/grid2/strategy.go

+
+					s.recoverActiveOrders(ctx, session)
+				})
+			*/


remove unused code?

c9s · 2023-10-06T07:32:20Z

pkg/strategy/grid2/strategy.go

-
-				s.recoverActiveOrders(ctx, session)
-			})
+			go s.recoverActiveOrdersWithOpenOrdersPeriodically(ctx)


every "OnAuth" event will launch a new goroutine for this?

you might need to check if recovered := atomic.LoadInt32(&s.recovered) then we start the goroutine for this?

or maybe you can move this to OnStart (which only trigger one time)

Yes, I should move it to OnStart

bbgokarma-bot · 2023-10-06T10:05:16Z

Re-estimated karma: this pull request may get 395 BBG

c9s · 2023-10-09T08:45:57Z

pkg/strategy/grid2/active_order_recover.go

+	}
+}
+
+func (s *Strategy) queryOpenOrdersThenRecoverActiveOrders(ctx context.Context) {


how about just syncOpenOrders

c9s · 2023-10-09T08:47:34Z

pkg/strategy/grid2/active_order_recover.go

+	// update active orders not in open orders
+	for _, activeOrder := range activeOrders {
+		if _, exist := openOrdersMap[activeOrder.OrderID]; !exist {
+			s.logger.Infof("find active order (%d) not in open orders, updating...", activeOrder.OrderID)


use #%d format for the orderID

c9s · 2023-10-09T08:49:30Z

pkg/strategy/grid2/active_order_recover.go

+			})
+
+			if err != nil {
+				s.logger.WithError(err).Errorf("[ActiveOrderRecover] unable to query order (%d)", activeOrder.OrderID)


use #%d format for the orderID

bbgokarma-bot · 2023-10-09T09:06:37Z

Re-estimated karma: this pull request may get 437 BBG

bbgokarma-bot · 2023-10-09T09:09:58Z

Re-estimated karma: this pull request may get 442 BBG

bbgokarma-bot · 2023-10-09T09:10:22Z

Re-estimated karma: this pull request may get 446 BBG

bbgokarma-bot · 2023-10-11T09:12:29Z

Re-estimated karma: this pull request may get 471 BBG

c9s · 2023-10-11T09:36:41Z

pkg/strategy/grid2/active_order_recover.go

+	var openOrdersExpired bool = false
+	for _, activeOrder := range activeOrders {
+		if _, exist := openOrdersMap[activeOrder.OrderID]; exist {
+			if openOrdersExpired || time.Now().After(openOrdersExpiredTime) {


pull out time.Now() to variable now?

bbgokarma-bot · 2023-10-11T09:36:45Z

Re-estimated karma: this pull request may get 508 BBG

c9s · 2023-10-11T09:37:12Z

pkg/strategy/grid2/active_order_recover.go

+	}
+
+	// update active orders not in open orders
+	var openOrdersExpired bool = false


explain how this variable is used in the comment?

bbgokarma-bot · 2023-10-12T06:20:52Z

Re-estimated karma: this pull request may get 518 BBG

…n on auth

…xpired

…5 min

bbgokarma-bot · 2023-10-13T08:51:16Z

Re-estimated karma: this pull request may get 712 BBG

c9s · 2023-10-13T09:18:14Z

pkg/strategy/grid2/strategy.go

-				s.recoverActiveOrders(ctx, session)
-			})
-		}
+		time.AfterFunc(util.MillisecondsJitter(5*time.Second, 1000*10), func() {


need to check if it's in backtest here, you need to run backtest

bbgokarma-bot · 2023-10-16T08:03:07Z

Re-estimated karma: this pull request may get 764 BBG

bbgokarma-bot · 2023-10-17T05:52:12Z

Re-estimated karma: this pull request may get 782 BBG

c9s · 2023-10-17T06:44:28Z

pkg/strategy/grid2/active_order_recover.go

+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	alreadyInitialize := false


isInitialized is much simpler word

c9s · 2023-10-17T06:45:29Z

pkg/strategy/grid2/active_order_recover.go

+	alreadyInitialize := false
+
+	if s.activeOrdersRecoverCh == nil {
+		s.logger.Info("initialize recover channel")


use word initializing

c9s · 2023-10-17T06:45:45Z

pkg/strategy/grid2/active_order_recover.go

+		s.logger.Info("initialize recover channel")
+		s.activeOrdersRecoverCh = make(chan struct{}, 1)
+	} else {
+		s.logger.Info("already initialize recover channel, trigger active orders recover")


recover channel is already already initialized

c9s · 2023-10-17T06:46:23Z

pkg/strategy/grid2/active_order_recover.go

+	} else {
+		s.logger.Info("already initialize recover channel, trigger active orders recover")
+		alreadyInitialize = true
+		s.activeOrdersRecoverCh <- struct{}{}


use select + default case to skip it if the channel is full?

c9s · 2023-10-17T06:47:44Z

pkg/strategy/grid2/active_order_recover.go

+				log.WithError(err).Errorf("unable to sync active orders")
+			}
+
+		case <-s.activeOrdersRecoverCh:


we usually end the channel var with C instead of Ch

bbgokarma-bot · 2023-10-17T07:20:42Z

Re-estimated karma: this pull request may get 839 BBG

bbgokarma-bot · 2023-10-17T08:13:26Z

Re-estimated karma: this pull request may get 883 BBG

kbearXD requested review from c9s and gx578007 September 28, 2023 08:16

c9s reviewed Oct 4, 2023

View reviewed changes

gx578007 approved these changes Oct 4, 2023

View reviewed changes

c9s reviewed Oct 6, 2023

View reviewed changes

pkg/strategy/grid2/strategy.go Outdated

s.recoverActiveOrders(ctx, session)

})

*/

Copy link

Owner

c9s Oct 6, 2023

Choose a reason for hiding this comment

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

remove unused code?

c9s reviewed Oct 6, 2023

View reviewed changes

kbearXD requested review from gx578007 and c9s October 6, 2023 10:05

c9s reviewed Oct 9, 2023

View reviewed changes

kbearXD requested a review from c9s October 11, 2023 09:16

c9s reviewed Oct 11, 2023

View reviewed changes

kbearXD requested a review from c9s October 12, 2023 08:24

c9s approved these changes Oct 12, 2023

View reviewed changes

kbearXD and others added 4 commits October 13, 2023 16:50

FEATURE: recover active orders with open orders periodically

ca80bdb

remove checker

4c9b1e7

FIX: fix some error and use chan to trigger active orders recover whe…

27294ac

…n on auth

grid2: refactor recoverActiveOrdersPeriodically

1347c8e

c9s and others added 6 commits October 13, 2023 16:50

grid2: improve some logging

5f9d020

grid2: invert if

a39925b

also sync orders already in active orderbook if the open orders are e…

c6d4ebf

…xpired

add open orders metrics

136c2cd

not add non existing open orders into active orderbook if updated in …

de1a884

…5 min

add test and remove recovered atmoic bool

c544937

kbearXD force-pushed the feature/grid2/recover-active-order-periodically branch from 5633d92 to c544937 Compare October 13, 2023 08:51

c9s reviewed Oct 13, 2023

View reviewed changes

move to onAuth

5ff3828

sleep 100ms to avoid DDOS

c257bc8

gx578007 approved these changes Oct 17, 2023

View reviewed changes

c9s reviewed Oct 17, 2023

View reviewed changes

fix nil metrics error

243b90a

fix

ccb7308

kbearXD merged commit 3bc03ff into main Oct 17, 2023
4 checks passed

kbearXD deleted the feature/grid2/recover-active-order-periodically branch October 17, 2023 09:33

FEATURE: recover active orders with open orders periodically #1328

FEATURE: recover active orders with open orders periodically #1328

Conversation

kbearXD commented Sep 28, 2023

bbgokarma-bot commented Sep 28, 2023

codecov bot commented Sep 28, 2023 • edited

Codecov Report

bbgokarma-bot commented Oct 2, 2023

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

bbgokarma-bot commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbgokarma-bot commented Oct 9, 2023

bbgokarma-bot commented Oct 9, 2023

bbgokarma-bot commented Oct 9, 2023

bbgokarma-bot commented Oct 11, 2023

Choose a reason for hiding this comment

bbgokarma-bot commented Oct 11, 2023

Choose a reason for hiding this comment

bbgokarma-bot commented Oct 12, 2023

bbgokarma-bot commented Oct 13, 2023

Choose a reason for hiding this comment

bbgokarma-bot commented Oct 16, 2023

bbgokarma-bot commented Oct 17, 2023

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

bbgokarma-bot commented Oct 17, 2023

bbgokarma-bot commented Oct 17, 2023

codecov bot commented Sep 28, 2023 •

edited