Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: add ttl for position/grid2.profit_stats persistence #1396

Merged
merged 5 commits into from Nov 8, 2023

Conversation

kbearXD
Copy link
Collaborator

@kbearXD kbearXD commented Nov 6, 2023

No description provided.

@kbearXD kbearXD requested a review from c9s November 6, 2023 10:52
@bbgokarma-bot
Copy link

Welcome back! @kbearXD, This pull request may get 242 BBG.

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #1396 (52d4f50) into main (e773bb0) will decrease coverage by 0.02%.
Report is 9 commits behind head on main.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1396      +/-   ##
==========================================
- Coverage   21.12%   21.10%   -0.02%     
==========================================
  Files         568      569       +1     
  Lines       40956    41069     +113     
==========================================
+ Hits         8653     8669      +16     
- Misses      31667    31763      +96     
- Partials      636      637       +1     
Files Coverage Δ
pkg/strategy/grid2/strategy.go 33.37% <0.00%> (-0.16%) ⬇️
pkg/strategy/grid2/profit_stats.go 16.66% <0.00%> (-0.99%) ⬇️
pkg/types/position.go 39.65% <0.00%> (-0.70%) ⬇️
pkg/strategy/grid2/active_order_recover.go 31.63% <0.00%> (-1.35%) ⬇️

... and 3 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 e773bb0...52d4f50. Read the comment docs.

s.ttl = ttl
}

func (s *Position) Expiration() time.Duration {
Copy link
Owner

Choose a reason for hiding this comment

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

do we check the returned Expiration() in the caller? e.g., skip 0 expiration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we don't need to do it. Because if the value is 0, it means it will not expire anymore.

@@ -1835,13 +1836,17 @@ func (s *Strategy) Run(ctx context.Context, _ bbgo.OrderExecutor, session *bbgo.
s.ProfitSpread = s.Market.TruncatePrice(s.ProfitSpread)
}

s.logger.Infof("ttl: %s", s.PersistenceTTL.Duration())
Copy link
Owner

Choose a reason for hiding this comment

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

using persistence ttl: %s

if s.GridProfitStats == nil {
s.GridProfitStats = newGridProfitStats(s.Market)
}
s.GridProfitStats.SetTTL(s.PersistenceTTL.Duration())
Copy link
Owner

Choose a reason for hiding this comment

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

should only call SetTTL when .PersistenceTTL is set and > 0

@bbgokarma-bot
Copy link

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

@bbgokarma-bot
Copy link

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

@kbearXD kbearXD requested a review from c9s November 7, 2023 05:42
if err := syncActiveOrders(ctx, opts); err != nil {
log.WithError(err).Errorf("unable to sync active orders")
s.recoverC <- struct{}{}
bbgo.Sync(ctx, s)
Copy link
Owner

Choose a reason for hiding this comment

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

you can remove this?

@@ -30,7 +30,7 @@ func (s *Strategy) initializeRecoverC() bool {

if s.recoverC == nil {
s.logger.Info("initializing recover channel")
s.recoverC = make(chan struct{}, 1)
s.recoverC = make(chan struct{}, 10)
Copy link
Owner

Choose a reason for hiding this comment

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

why use 10?

@bbgokarma-bot
Copy link

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

@kbearXD kbearXD requested a review from c9s November 8, 2023 03:05
@bbgokarma-bot
Copy link

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

@kbearXD kbearXD merged commit 20dccc0 into main Nov 8, 2023
2 of 4 checks passed
@kbearXD kbearXD deleted the chiahung/grid2/persistence-ttl branch November 8, 2023 05:50
@bbgokarma-bot
Copy link

Hi @kbearXD,

Well done! 346 BBG has been sent to your polygon wallet. Please check the following tx:

https://polygonscan.com/tx/0x0f7c74be38027f9546371757068be79f84ee0ab5781f0d39522c3a2fa47be956

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants