Fix the bollinger band strategy

[hwde]

Describe Request

As mentioned in #269 the strategy seems to be invers

Fixed #269

Change Type

correction for the signal actions

Summary by CodeRabbit

• Bug Fixes
  • Corrected signal-to-action mapping in the Bollinger Bands strategy: closes above the upper band now trigger Sell, and closes below the lower band trigger Buy.
  • Other behavior remains unchanged, including idle period handling and action sequencing.
  • Users may observe different trade executions around band crossings.
  • No changes to settings or public interfaces.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.79%. Comparing base (9d73f49) to head (a5da5ba).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   91.73%   91.79%   +0.05%     
==========================================
  Files         191      191              
  Lines        6797     6797              
==========================================
+ Hits         6235     6239       +4     
+ Misses        497      494       -3     
+ Partials       65       64       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Walkthrough

The BollingerBandsStrategy.Compute function updates the action mapping: price closing above the upper band now yields Sell, and price closing below the lower band now yields Buy. All other logic, including inputs, idle-period handling, and action shifting, remains unchanged.

Changes

Cohort / File(s)	Summary
Bollinger Bands strategy logic inversion strategy/volatility/bollinger_bands_strategy.go	Invert action mapping: closing > upper → Sell (was Buy); closing < lower → Buy (was Sell). Preserve inputs, IdlePeriod skip, and final Shift of actions.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Market as Market Data
  participant BB as BollingerBandsStrategy.Compute
  participant Mapper as Action Mapper
  participant Out as Actions

  Market->>BB: close, upper, lower
  BB->>Mapper: Evaluate band-cross conditions

  alt close > upper
    note over Mapper: CHANGED: map to Sell
    Mapper-->>Out: Action = Sell
  else close < lower
    note over Mapper: CHANGED: map to Buy
    Mapper-->>Out: Action = Buy
  else within bands
    Mapper-->>Out: Action = Hold/None
  end

  BB->>Out: Apply IdlePeriod skip and Shift (unchanged)

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny peeks at bands so wide,
Hops above the upper—“time to slide!” 🥕
Dips below the lower—“now I buy!”
Ears twitch, logic flipped, oh my!
With tidy paws I mark the trade,
Carrots saved, corrections made.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Fix the bollinger band strategy" is concise and directly reflects the primary change—correcting the Bollinger Bands strategy's inverted buy/sell logic—so it clearly communicates the main intent to reviewers.
Linked Issues Check	✅ Passed	The change in strategy/volatility/bollinger_bands_strategy.go reverses the buy/sell mapping so that closing > upper yields Sell and closing < lower yields Buy, which directly addresses the bug reported in issue [#269]; the PR summary's Codecov comments also indicate the modified lines are covered by tests and overall coverage is unchanged, supporting that the coding requirement is met.
Out of Scope Changes Check	✅ Passed	Only the Bollinger Bands strategy implementation was modified (strategy/volatility/bollinger_bands_strategy.go) and there are no changes to exported APIs or unrelated files, so I did not find out-of-scope changes and the edits align with the objective of fixing issue [#269].
Description Check	✅ Passed	The PR description follows the repository template by including a Describe Request section, referencing the linked issue ("Fixed #269"), and specifying the Change Type as a correction for signal actions, so the required template fields are present and sufficiently filled.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

strategy/volatility/bollinger_bands_strategy.go (1)

15-16: Doc comment contradicts code and has typos — update to mean-reversion wording

Replace the header in strategy/volatility/bollinger_bands_strategy.go with the diff below; also fix spelling in strategy/volatility/super_trend_strategy.go ('suggets' → 'suggests', 'indivates' → 'indicates').

-// BollingerBandsStrategy represents the configuration parameters for calculating the Bollinger Bands strategy.
-// A closing value crossing above the upper band suggets a Buy signal, while crossing below the lower band
-// indivates a Sell signal.
+// BollingerBandsStrategy represents the configuration parameters for calculating the Bollinger Bands strategy.
+// Mean-reversion variant: a close above the upper band suggests a Sell signal (overbought),
+// while a close below the lower band suggests a Buy signal (oversold).

🧹 Nitpick comments (2)

strategy/volatility/bollinger_bands_strategy.go (2)

51-53: Nit: prefer closing < lower for readability/consistency.

Reads more naturally alongside the upper-band check.

Apply this diff:

-        if lower > closing {
+        if closing < lower {
           return strategy.Buy
         }

---

47-53: Boundary condition: should equality trigger a signal?

Decide whether closes exactly on a band should count (>= / <=) or remain strict. If inclusive is desired, change to closing >= upper and closing <= lower and note this in docs.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d73f49 and a5da5ba.

⛔ Files ignored due to path filters (1)

• strategy/volatility/testdata/bollinger_bands_strategy.csv is excluded by !**/*.csv

📒 Files selected for processing (1)

• strategy/volatility/bollinger_bands_strategy.go (1 hunks)

🔇 Additional comments (1)

strategy/volatility/bollinger_bands_strategy.go (1)

46-53: Signal inversion looks correct (mean‑reversion).

Mapping close > upper → Sell and close < lower → Buy aligns with the reported issue and typical mean‑reversion BB usage.

[cinar]

Thank you!