pending trades(?) and historical data fetching

[sixtysixx]

honestly rust syntax is so confusing to me and half of this is AI lmfao so im not sure what exactly works or not

Summary by CodeRabbit

• New Features

  • Added historical data retrieval for trading assets
  • Added pending trades/orders functionality
  • Added asset validation for trading operations
  • Extended client with trade result checking capabilities

• Bug Fixes

  • Fixed module naming and import paths (spelling corrections)
  • Corrected error messaging

• Documentation

  • Expanded API reference with configuration parameters
  • Updated README with installation, usage examples, and feature status

• Refactor

  • Updated Rust edition compatibility (2024 → 2021)
  • Modernized TLS implementation
  • Improved CI/CD workflow structure

• Chores

  • Updated dependencies and build configuration
  • Reorganized project structure and test infrastructure

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[sixtysixx]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 6

Caution

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

⚠️ Outside diff range comments (4)

README.md (2)

202-221: Add explicit connect() in real-time streaming example.

The example subscribes immediately after client creation, but earlier guidance requires calling connect() first. Please add an explicit connection step to avoid a race or failed subscription.

Proposed fix

 async def main():
     client = PocketOptionAsync(ssid="your_ssid_here")
+    await client.connect()
     
     # Subscribe to 60-second candles
     subscription = await client.subscribe_symbol("EURUSD_otc", 60)

---

231-245: Add explicit connect() in Raw Handler example.

The raw handler is created before establishing a connection. Please call await client.connect() first to ensure the WebSocket is ready.

Proposed fix

 async def main():
     client = PocketOptionAsync(ssid="your_ssid_here")
+    await client.connect()
     
     # Create raw handler with validator
     validator = Validator.contains("price")

crates/binary_options_tools/src/expertoptions/connect.rs (1)

39-43: Incorrect log target names.

The log targets reference "PocketConnect" but this is the ExpertOptions connector. This will make log filtering and debugging confusing.

Suggested fix

             Ok(stream) => {
-                    info!(target: "PocketConnect", "Successfully connected to ExpertOptions");
+                    info!(target: "ExpertConnect", "Successfully connected to ExpertOptions");
                     return Ok(stream);
                 }
-                Err((e, u)) => warn!(target: "PocketConnect", "Failed to connect to {}: {}", u, e),
+                Err((e, u)) => warn!(target: "ExpertConnect", "Failed to connect to {}: {}", u, e),

crates/binary_options_tools/src/pocketoption/utils.rs (1)

33-41: Potential panic from unwrap() on API response.

Lines 37-38 use .unwrap() which will panic if the ip-api.com response doesn't contain valid lat/lon fields. This is inconsistent with the safer error handling now used in get_public_ip (lines 60-66).

Suggested fix: consistent error handling

 pub async fn get_user_location(ip_address: &str) -> PocketResult<(f64, f64)> {
     let response = reqwest::get(format!("{IP_API_URL}{ip_address}")).await?;
     let json: Value = response.json().await?;

-    let lat = json["lat"].as_f64().unwrap();
-    let lon = json["lon"].as_f64().unwrap();
+    let lat = json["lat"].as_f64().ok_or_else(|| {
+        PocketError::General(format!("Missing or invalid 'lat' in API response: {:?}", json))
+    })?;
+    let lon = json["lon"].as_f64().ok_or_else(|| {
+        PocketError::General(format!("Missing or invalid 'lon' in API response: {:?}", json))
+    })?;

     Ok((lat, lon))
 }

🤖 Fix all issues with AI agents

In `@BinaryOptionsToolsV2/BinaryOptionsToolsV2/__init__.py`:
- Around line 4-9: The package __all__ currently only combines __pocket_all__
plus ["tracing","validator"], so symbols imported via from .BinaryOptionsToolsV2
import * (the Rust/core bindings in module BinaryOptionsToolsV2) are not
exported; update the package-level __all__ to include the core module's public
names (e.g. import __all__ from .BinaryOptionsToolsV2 or retrieve it as
__core_all__) and combine it with __pocket_all__ and ["tracing","validator"] so
that core exports are exposed when users do from BinaryOptionsToolsV2 import *;
locate the imports at the top of __init__.py (BinaryOptionsToolsV2 import *,
BinaryOptionsToolsV2 import __doc__) and add/merge the core __all__ into the
final __all__ definition.

In `@BinaryOptionsToolsV2/BinaryOptionsToolsV2/pocketoption/synchronous.py`:
- Around line 307-310: The return type annotation for the synchronous wrapper
method payout must allow None values to match the async contract; update the
payout signature (the payout method) so its return type includes None for the
whole return and for contained elements (i.e., allow dict[str, int] | list[int]
| int to become Optional[dict[str, Optional[int]]] |
Optional[list[Optional[int]]] | Optional[int] or equivalent union types),
ensuring the annotation reflects that the method can return None and that
dict/list entries may be None.

In `@BinaryOptionsToolsV2/src/lib.rs`:
- Around line 19-21: The module is marked as free-threaded but uses GIL-bound
APIs; remove the gil_used = false attribute from the #[pymodule(...)] on
BinaryOptionsTools (i.e., change #[pymodule(gil_used = false)] fn
BinaryOptionsTools(...) to just #[pymodule(name = "BinaryOptionsTools")] fn
BinaryOptionsTools(...)) so the interpreter will ensure the GIL when calling
methods that use Python<'py> tokens; this fixes unsafe calls from
RawValidator/RawPocketOption methods (send_text, send_binary, wait_next) and
StreamLogsIterator::__anext__ that use future_into_py and Py<PyAny> references.

In `@BinaryOptionsToolsV2/src/validator.rs`:
- Around line 211-219: The comment and implementation mismatch: replace the
deprecated Python::with_gil usage inside RawValidator::Custom so it uses the
non-deprecated Python::attach API (like the other occurrence at line 263) and
remove the #[allow(deprecated)] block; call py_custom.custom.as_ref().call1
using Python::attach and then extract the bool result (returning false on
errors) to match the surrounding pattern.

In `@crates/binary_options_tools/src/pocketoption/modules/historical_data.rs`:
- Around line 199-200: Replace the unsafe format!-based JSON assembly for the
"changeSymbol" payload with a proper serializer: build a JSON object containing
the event and data (using serde_json::json! or a small serde-serializable struct
with fields asset and period), serialize it with serde_json::to_string, prepend
the "42" prefix, then call
self.to_ws_sender.send(Message::text(serialized_msg)).await?; this ensures asset
and period are properly escaped and avoids JSON injection risks around the msg
variable and the Message::text call.

In `@crates/core-pre/target/.rustc_info.json`:
- Line 1: The repo has committed build artifacts (e.g., the tracked
.rustc_info.json and many files under target/) which must be removed from
version control: remove all tracked files under the target/ directory from the
Git index (so they remain on disk but are no longer tracked), add a root-level
.gitignore entry for target/ to prevent re-adding, and commit the removal and
updated .gitignore; verify that .rustc_info.json and other machine-generated
files under target/ are no longer tracked.

♻️ Duplicate comments (3)

README.md (2)

38-41: Resolve availability contradictions for trade history and open/closed deals.

“Trade history (history)” and “Closed deals management” are listed as unavailable, but the Features section advertises “Open/Closed Deals” and “trade history.” Please align these statements.

Also applies to: 53-60

---

40-41: Payout availability is inconsistent across sections.

“Payout information retrieval” is marked unavailable, but Market Data lists “Payout Information” as available. Please reconcile the two sections.

Also applies to: 60-60

.github/workflows/CI.yml (1)

342-344: Past review feedback addressed.

The actions/setup-node has been correctly updated from v3 to v6.

🧹 Nitpick comments (9)

.github/workflows/CI.yml (1)

401-406: Consider uploading all wheels to GitHub Release and updating action version.

Only wasm-wheels/*.whl is uploaded to GitHub Release while all other platform wheels (wheels-*/*) are published to PyPI but not attached to the release. If intentional, this is fine. Otherwise, consider including all wheels.

Also, softprops/action-gh-release@v1 is outdated—v2 is available with improvements.

Suggested changes

-      - name: Upload to GitHub Release
-        uses: softprops/action-gh-release@v1
-        with:
-          files: |
-            wasm-wheels/*.whl
-          prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') }}
+      - name: Upload to GitHub Release
+        uses: softprops/action-gh-release@v2
+        with:
+          files: |
+            wheels-*/*
+          prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') }}

crates/binary_options_tools/src/expertoptions/connect.rs (1)

62-71: Silent certificate loading failures may cause TLS handshake errors.

The current implementation silently discards certificate loading errors in two places:

1. certs_result.errors is never checked - if native cert loading partially or fully fails, there's no visibility
2. .ok() on root_store.add(cert) silently ignores individual cert parsing failures

If all certificates fail to load, the root_store will be empty and TLS connections will fail with cryptic errors.

Suggested improvement: log certificate loading issues

     let mut root_store = rustls::RootCertStore::empty();
     let certs_result = rustls_native_certs::load_native_certs();
+    if !certs_result.errors.is_empty() {
+        warn!(target: "ExpertConnect", "Certificate loading errors: {:?}", certs_result.errors);
+    }
     for cert in certs_result.certs {
-        root_store.add(cert).ok();
+        if let Err(e) = root_store.add(cert) {
+            warn!(target: "ExpertConnect", "Failed to add certificate: {:?}", e);
+        }
     }
+    if root_store.is_empty() {
+        return Err(ConnectorError::Custom("No valid TLS certificates loaded".into()));
+    }

Additionally, this TLS setup is duplicated in pocketoption/utils.rs. Consider extracting it into a shared utility function.

crates/binary_options_tools/src/pocketoption/utils.rs (1)

73-82: Same TLS certificate loading issue as expertoptions/connect.rs.

This is identical code to the TLS setup in connect.rs with the same silent error handling concerns. Certificate loading failures are silently ignored, which could lead to an empty root store and cryptic TLS errors.

Consider extracting this TLS configuration into a shared utility:

// In a shared module, e.g., src/tls.rs
pub fn create_rustls_connector() -> ConnectorResult<Connector> {
    let mut root_store = rustls::RootCertStore::empty();
    let certs_result = rustls_native_certs::load_native_certs();
    
    for err in &certs_result.errors {
        tracing::warn!("Certificate loading error: {:?}", err);
    }
    for cert in certs_result.certs {
        if let Err(e) = root_store.add(cert) {
            tracing::warn!("Failed to add certificate: {:?}", e);
        }
    }
    if root_store.is_empty() {
        return Err(ConnectorError::Custom("No valid TLS certificates loaded".into()));
    }
    
    let tls_config = rustls::ClientConfig::builder()
        .with_root_certificates(root_store)
        .with_no_client_auth();
    
    Ok(Connector::Rustls(std::sync::Arc::new(tls_config)))
}

This would eliminate duplication and ensure consistent error handling across both connectors.

BinaryOptionsToolsV2/src/validator.rs (2)

104-127: Consider tracking or removing stale TODO comments.

These TODO blocks contain commented-out code referencing RawWebsocketMessage which appears to be from an older API. If this functionality is still needed, consider creating issues to track implementation. Otherwise, removing dead code improves maintainability.

---

194-197: Inefficient implementation: check converts to CrateValidator unnecessarily.

RawValidator already has a call method (lines 201-221) that handles all variants directly. The current implementation clones, converts to CrateValidator, then calls—adding overhead especially for Custom validators where a new PyCustomValidator is created.

Suggested simplification

     pub fn check(&self, msg: String) -> bool {
-        let validator: CrateValidator = self.clone().into();
-        validator.call(&msg)
+        self.call(&msg)
     }

crates/binary_options_tools/src/pocketoption/modules/historical_data.rs (3)

122-122: CommandResponse::Error doesn't correlate req_id.

Unlike CommandResponse::Success, the Error variant doesn't include a req_id. If errors could ever arrive out-of-order or for a different request (however unlikely with the lock), this could return an error from a previous request. Consider adding req_id to the Error variant for consistency, or document this limitation.

♻️ Suggested change

 #[derive(Debug, Clone)]
 pub enum CommandResponse {
     Success { req_id: Uuid, candles: Vec<Candle> },
-    Error(String),
+    Error { req_id: Uuid, message: String },
 }

Then update the match arm accordingly.

---

259-261: Silent failures during candle conversion may hide data corruption.

When Candle::try_from fails, the error is silently ignored and the candle is dropped. Consider logging a warning for failed conversions or collecting error counts to help diagnose issues with malformed server data.

♻️ Add warning log for conversion failures

                                         if let Ok(candle) = Candle::try_from((base_candle, symbol.clone())) {
                                             candles.push(candle);
+                                        } else {
+                                            warn!(
+                                                target: "HistoricalDataApiModule",
+                                                "Failed to convert candle at index {} for asset {}",
+                                                i, symbol
+                                            );
                                         }

---

306-491: Consider adding tests for error and edge-case paths.

The happy-path tests are solid, but coverage for the following scenarios would improve robustness:

• ServerResponse::Fail error handling
• Timeout behavior
• MAX_MISMATCH_RETRIES exceeded
• Array length mismatch in HistoryResponse

BinaryOptionsToolsV2/BinaryOptionsToolsV2/__init__.py (1)

5-6: Clean up Ruff suppressions for star imports.

Ruff flags an unused # noqa: F401 on Line 5 and a missing # noqa: F403 on the pocketoption star import.

🧹 Lint-clean variant

-from .BinaryOptionsToolsV2 import __doc__  # noqa: F401
+from .BinaryOptionsToolsV2 import __doc__
@@
-from .pocketoption import *
+from .pocketoption import *  # noqa: F403

[theshadow76]

Hello @Rick-29 Please check this and make your review.

[sixtysixx]

Final check using Gemini and CodeRabbit:

[sixtysixx]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sixtysixx]

/gemini review

[sixtysixx]

@coderabbitai summary

[coderabbitai]

✅ Actions performed

Summary regeneration triggered.

[gemini-code-assist]

Code Review

This pull request includes several changes, including downgrading the Rust edition from 2024 to 2021, removing the CI workflow file, adding a get_trade_history function and get_deal_end_time function, adding a HistoricalDataApiModule for handling historical data requests, adding a PendingTradesApiModule for handling pending trade orders, and adding various improvements and fixes. The code review comments highlight the critical issue of removing the CI workflow file, request confirmation on downgrading the Rust edition, suggest including message content in a warning message, and commend the logic for handling duplicate history requests and the robust error handling in get_server_datetime.

I am having trouble creating individual review comments. Click here to see my feedback.

BinaryOptionsToolsV2/.github/workflows/CI.yml (1-169)

The removal of the CI workflow file is a critical issue. Without automated testing and build processes, it becomes significantly harder to maintain code quality, catch regressions, and ensure the reliability of the project. This should be reinstated or replaced with an equivalent automated process.

BinaryOptionsToolsV2/Cargo.toml (4)

The Rust edition has been downgraded from 2024 to 2021. While 2021 is a stable edition, downgrading might indicate compatibility issues with other tools or dependencies that were not explicitly mentioned. Please ensure this change is intentional and doesn't introduce any limitations or prevent future upgrades to newer Rust features.

crates/binary_options_tools/Cargo.toml (4)

The Rust edition has been downgraded from 2024 to 2021. Please confirm this change is intentional and necessary, as it might limit the use of newer Rust language features or indicate compatibility constraints with other parts of the project or toolchain.

crates/binary_options_tools/src/pocketoption/modules/deals.rs (237-239)

The warning message Received unexpected binary message when no header was seen. is good for debugging. However, it might be beneficial to include the actual message content (or a truncated version) in the warning to provide more context when this occurs, helping to identify the source of unexpected messages.

crates/binary_options_tools/src/pocketoption/modules/subscriptions.rs (446-465)

The added logic to handle duplicate history requests and send a HistoryFailed response is a good improvement for robustness. It prevents the system from being overwhelmed by redundant requests and provides clear feedback to the caller.

crates/binary_options_tools/src/pocketoption/state.rs (198-206)

The added match statement for DateTime::from_timestamp in get_server_datetime provides more robust error handling. Instead of unwrap_or_else, it now logs a warning if the timestamp conversion fails and defaults to Utc::now(), which is a safer approach.

crates/binary_options_tools/src/pocketoption/utils.rs (58-66)

Using the IPIFY_URL constant and adding a match statement to handle the json["ip"].as_str() result improves robustness. It prevents an unwrap() call from panicking if the 'ip' field is missing or not a string, providing a more graceful error handling mechanism.

[sixtysixx]

@coderabbitai summary

[coderabbitai]

✅ Actions performed

Summary regeneration triggered.