-1 balance hotfix and some minor fixes/alterations#73
Merged
theshadow76 merged 6 commits intoChipaDevTeam:masterfrom Mar 5, 2026
Merged
-1 balance hotfix and some minor fixes/alterations#73theshadow76 merged 6 commits intoChipaDevTeam:masterfrom
theshadow76 merged 6 commits intoChipaDevTeam:masterfrom
Conversation
Updated installation instructions and code examples in README.md. Changed section titles and improved clarity on building from source.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughResilient message parsing and state management were introduced across PocketOption: rule types replaced with MultiPatternRule, SSID parsing/normalization tightened, a full async thread-safe State and TradeState added, pending-trade call serialization added, WebSocket reconnection logic refactored, and integration tests added for text/binary message handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant WebSocketClient
participant Module (Balance/Deals)
participant State
Server->>WebSocketClient: send "42[...]" or "451-..." message
WebSocketClient->>Module: deliver Message (text or binary placeholder)
Module->>Module: MultiPatternRule matches event/pattern
Module->>State: parse payload → update balance/trades/assets
State-->>Module: notify waiters (balance_updated/assets_updated)
Module-->>WebSocketClient: respond/resolve waiting requests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a crucial fix for a negative balance display bug and significantly enhances the client's ability to parse WebSocket messages from the PocketOption platform. The core changes involve making the message processing more resilient to varying Socket.IO message formats and strengthening the session ID (SSID) parsing to prevent common issues. These improvements contribute to a more stable and reliable client, alongside minor code quality enhancements and updated documentation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with balance calculation and introduces several other fixes and alterations. Key changes include refactoring the WebSocket message parsing to be more resilient, replacing unwrap() calls with expect() for improved error reporting, and hardening SSID parsing against potential double-encoding issues. I have one suggestion regarding the new parsing logic in deals.rs to improve its efficiency.
Comment on lines
+242
to
+246
| if let Ok(data) = serde_json::to_string(&arr[1]) { | ||
| self.process_text_data(&data, current_expected).await; | ||
| expected = ExpectedMessage::None; | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
The current implementation serializes the JSON value arr[1] back to a string using serde_json::to_string, only for it to be immediately deserialized from a string inside process_text_data. This serialization-deserialization cycle is inefficient.
A better approach would be to refactor process_text_data to a new function, for example process_json_data, which accepts a &serde_json::Value. This would allow you to deserialize your types directly from the JSON value, avoiding the unnecessary overhead and improving performance. The existing process_text_data could then be updated to parse the string into a Value and call the new function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/general/client.rs (1)
269-282:⚠️ Potential issue | 🔴 CriticalCritical: Match result is discarded, causing function to always return error instead of successful reconnection.
The match expression (lines 269–277) evaluates but its result is immediately discarded. Line 278 then creates a separate
Err(ReconnectionAttemptFailure)that becomes the final return value. This means:
- Successful reconnection from
handle_reconnection_failurenever reaches the caller (lines 209–211 expectOk(websocket)to updatewrite/read)- The function always returns
Err(ReconnectionAttemptFailure)regardless of the match outcomeThe match result must be returned directly. Either add
returnin each match arm, or return the entire match expression:Proposed fix
- match try_join3(listener_future, sender_future, callback).await { + return match try_join3(listener_future, sender_future, callback).await { Ok(_) => { Self::handle_reconnection_failure(connector, credentials, config, loops).await } Err(e) => { warn!("Error in event loop, {e}, reconnecting..."); Self::handle_reconnection_failure(connector, credentials, config, loops).await } - } - Err(BinaryOptionsToolsError::ReconnectionAttemptFailure { - number: *loops, - max: config.get_max_allowed_loops()?, - }) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/general/client.rs` around lines 269 - 282, The match on try_join3(listener_future, sender_future, callback).await is evaluated but its result is discarded, causing the subsequent Err(BinaryOptionsToolsError::ReconnectionAttemptFailure { ... }) to always be returned; change the code so the match expression is returned (or add explicit returns in each arm) so that the results from Self::handle_reconnection_failure(connector, credentials, config, loops).await propagate to the caller instead of being ignored, and only return Err(BinaryOptionsToolsError::ReconnectionAttemptFailure { number: *loops, max: config.get_max_allowed_loops()? }) when appropriate.
🧹 Nitpick comments (10)
crates/core/src/general/client.rs (2)
291-306: Consider logging the connection error for observability.The connection error from
connector.connect()is discarded in theelsebranch. Logging the specific failure reason would help with debugging reconnection issues.♻️ Proposed fix
- if let Ok(websocket) = connector.connect(credentials.clone(), config).await { - return Ok(websocket); - } else { + match connector.connect(credentials.clone(), config).await { + Ok(websocket) => return Ok(websocket), + Err(e) => { + warn!("Connection attempt failed: {e}"); + } + } + { *loops += 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/general/client.rs` around lines 291 - 306, The reconnection branch discards the error returned by connector.connect(); capture that Err value and include it in the warning log so failures are observable. Update the if-let to match on connector.connect(...).await as Err(e) (or use match) and log the error (e) along with the existing message (including variables loops and max_loops from the surrounding scope), then keep the existing sleep and max-loop handling; ensure BinaryOptionsToolsError::MaxReconnectAttemptsReached remains returned when loops >= max_loops.
295-300: Minor: Redundantget_sleep_interval()call.
config.get_sleep_interval()?is called twice—once for logging and once for sleeping. Store it in a single variable.♻️ Proposed fix
*loops += 1; let sleep_interval = config.get_sleep_interval()?; let max_loops = config.get_max_allowed_loops()?; warn!( "Error reconnecting... trying again in {sleep_interval} seconds (try {loops} of {max_loops})" ); - sleep(Duration::from_secs(config.get_sleep_interval()?)).await; + sleep(Duration::from_secs(sleep_interval)).await; if *loops >= max_loops {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/general/client.rs` around lines 295 - 300, The code calls config.get_sleep_interval()? twice; fetch it once into a local variable (e.g., let sleep_interval = config.get_sleep_interval()? already present) and reuse that variable in the warn! message and in the sleep(Duration::from_secs(...)). Also reuse the existing max_loops variable from config.get_max_allowed_loops()?; update the warn! invocation and the sleep(...) call to reference sleep_interval instead of calling config.get_sleep_interval()? again so the function is only invoked once.crates/binary_options_tools/src/pocketoption/pocket_client.rs (1)
168-175: Remove the duplicated rustdoc block abovenew().This repeats the constructor documentation already present just above and can drift over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/pocket_client.rs` around lines 168 - 175, Remove the duplicated rustdoc block that precedes the async constructor function PocketClient::new(ssid: impl ToString) -> PocketResult<Self>; keep only the original/first doc comment for the constructor and delete the repeated comment immediately above the pub async fn new(...) declaration so the constructor has a single, canonical docstring.crates/binary_options_tools/src/pocketoption/modules/keep_alive.rs (1)
94-97: Use an exact auth-frame prefix check here.Line 94 checks only
'4', which can let non-auth Socket.IO frames bypass wrapping. Prefer checking for42["auth",explicitly before deciding not to wrap.Proposed patch
- if !ssid_str.starts_with('4') { + if !ssid_str.starts_with(r#"42["auth","#) { debug!(target: "InitModule", "SSID does not start with Socket.IO packet type; wrapping in 42[\"auth\",...]"); ssid_str = format!(r#"42["auth",{}]"#, ssid_str); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/modules/keep_alive.rs` around lines 94 - 97, The current check on ssid_str uses starts_with('4') and can miss non-auth Socket.IO frames; change the logic in the keep-alive handling (where ssid_str is modified) to test for the exact auth-frame prefix like 42["auth", (e.g., use starts_with with the full prefix string) and only wrap when that exact prefix is absent; update the conditional around ssid_str and adjust the debug message (in the same block that currently logs "SSID does not start with Socket.IO packet type; wrapping in 42[\"auth\",...]") to reflect the exact-prefix check and wrapping action.crates/binary_options_tools/src/pocketoption/state.rs (1)
335-342: Avoid quadratic lookup inupdate_closed_deals.Line 341 uses
ids.contains(id)with aVec, which becomes O(opened_deals × closed_batch). Use aHashSetfor membership checks.Proposed patch
use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, sync::{Arc, RwLock as SyncRwLock}, time::Instant, }; @@ - let ids: Vec<_> = deals.iter().map(|deal| deal.id).collect(); + let ids: HashSet<_> = deals.iter().map(|deal| deal.id).collect(); @@ - .retain(|id, _| !ids.contains(id)); + .retain(|id, _| !ids.contains(id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/state.rs` around lines 335 - 342, The retain call in update_closed_deals is doing a quadratic membership check by using ids.contains(id) on a Vec; change to build a HashSet of ids (e.g., let id_set: HashSet<_> = deals.iter().map(|d| d.id).collect()) and then call retain(|id, _| !id_set.contains(id)) on self.opened_deals to get O(1) lookups; import std::collections::HashSet and ensure the deal id type implements Hash + Eq.crates/binary_options_tools/src/pocketoption/modules/deals.rs (3)
373-384: Comment is slightly incomplete.The comment mentions only
451-prefix messages, but based on theMultiPatternRuleimplementation in the context snippet, it matches both451-and42prefixed messages (any Socket.IO format with[array notation).📝 Update comment
fn rule(_: Arc<State>) -> Box<dyn Rule + Send + Sync> { // This rule will match messages like: - // 451-["updateOpenedDeals",...] - // 451-["updateClosedDeals",...] - // 451-["successcloseOrder",...] + // 42["updateOpenedDeals",...] or 451-["updateOpenedDeals",...] + // 42["updateClosedDeals",...] or 451-["updateClosedDeals",...] + // 42["successcloseOrder",...] or 451-["successcloseOrder",...] Box::new(MultiPatternRule::new(vec![🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/modules/deals.rs` around lines 373 - 384, The inline comment in the rule() function is incomplete: update the comment above the Box::new(MultiPatternRule::new(...)) to indicate this rule matches both "451-" prefixed messages and Socket.IO "42" prefixed messages (any array-style payload like [...]) for the patterns EV_UPDATE_CLOSED_DEALS, EV_UPDATE_OPENED_DEALS, and EV_SUCCESS_CLOSE_ORDER; reference the MultiPatternRule and the EV_* constants so readers know the rule covers both transport prefixes and array-formatted payloads.
271-339: Consider extracting common waiter notification logic.The binary message handling (lines 271-339) duplicates the waiter notification pattern found in
process_text_data. The pattern of iteratingdeals, checkingwaiting_requests, and sending results appears in multiple places.If this module grows, extracting a helper method like
notify_waiters_for_closed_deals(&mut self, deals: Vec<Deal>)would reduce duplication. Not urgent for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/modules/deals.rs` around lines 271 - 339, Extract the duplicated waiter-notification logic into a helper method named notify_waiters_for_closed_deals(&mut self, deals: Vec<Deal>) that: calls self.state.trade_state.update_closed_deals(deals.clone()).await, iterates over each deal, removes waiters via self.waiting_requests.remove(&deal.id), logs (info) the closed deal, and sends Ok(deal.clone()) to each tx; then replace the repeated blocks inside Message::Binary arms (ExpectedMessage::UpdateClosedDeals and ExpectedMessage::SuccessCloseOrder fallback) and the similar code in process_text_data to call notify_waiters_for_closed_deals(self, deals) instead of duplicating the loop and send logic.
247-258: Redundant placeholder check in else branch.At line 240, the condition
arr.len() >= 2filters to 1-step messages. In thiselsebranch,arr.len() < 2, meaningarr.len() == 1(since we know!arr.is_empty()from line 229).The
has_placeholdercheck on lines 249-251 iteratesarr.iter().skip(1), which yields an empty iterator whenarr.len() == 1. Thus,has_placeholderis alwaysfalsein this branch, making the condition at line 253 equivalent to justarr.len() == 1.♻️ Simplify the condition
} else { - // Check for binary placeholder in the whole array if it's not 1-step - let has_placeholder = arr.iter().skip(1).any(|v| { - v.as_object().is_some_and(|obj| obj.contains_key("_placeholder")) - }); - - if has_placeholder || arr.len() == 1 { - tracing::debug!(target: "DealsApiModule", "Detected binary placeholder, waiting for binary payload for {:?}", current_expected); + // Event name only, expect binary payload next + if arr.len() == 1 { + tracing::debug!(target: "DealsApiModule", "Event header only, waiting for binary payload for {:?}", current_expected); expected = current_expected; continue; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/modules/deals.rs` around lines 247 - 258, The else branch after the arr.len() >= 2 check contains a redundant has_placeholder computation that always evaluates false because arr.iter().skip(1) is empty when arr.len() == 1; update the logic in the DealsApiModule handling (the block referencing arr, has_placeholder, expected and current_expected) to remove the has_placeholder iteration and simply check arr.len() == 1 (or otherwise only detect the 1-step case) before setting expected = current_expected and emitting the same tracing::debug message; keep the existing behavior for setting expected and continuing, but eliminate the needless iterator and boolean to simplify the code.crates/binary_options_tools/src/pocketoption/modules/resilient_parsing_tests.rs (2)
71-109: Same flaky pattern applies here.This test has the same sleep-based synchronization and untracked spawned task issues as the balance test. Apply the same fix pattern for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/modules/resilient_parsing_tests.rs` around lines 71 - 109, The test unsafely relies on tokio::time::sleep and an untracked tokio::spawn for module.run(), making it flaky; change the pattern to track and await the spawned task or signal completion explicitly instead of sleeping: capture the JoinHandle from tokio::spawn(async move { let _ = module.run().await; }) (or have module.run() return when idle), then after sending msg_tx.send(...).await.unwrap() await that JoinHandle (or use a oneshot/Notify that module.run() triggers after processing) before reading state.trade_state.opened_deals.read().await and asserting the UUID; remove the sleep and untracked spawn so the test deterministically waits for processing of msg_deal_42.
34-46: Sleep-based synchronization is flaky and spawned task errors are silent.The tests use
tokio::time::sleep(Duration::from_millis(100))for synchronization, which can lead to intermittent failures under load. Additionally, the spawned task at lines 34-36 is not tracked—ifmodule.run()panics, the test won't detect it.Consider using a more deterministic synchronization approach and tracking the spawned task:
♻️ Suggested improvement using JoinHandle
- // Run the module in background - tokio::spawn(async move { - let _ = module.run().await; - }); + // Run the module in background with handle for error propagation + let module_handle = tokio::spawn(async move { + module.run().await + }); // Send 451 message msg_tx.send(Arc::new(msg_451)).await.unwrap(); tokio::time::sleep(std::time::Duration::from_millis(100)).await; assert_eq!(*state.balance.read().await, Some(dec!(123.45))); // Send 42 message msg_tx.send(Arc::new(msg_42)).await.unwrap(); tokio::time::sleep(std::time::Duration::from_millis(100)).await; assert_eq!(*state.balance.read().await, Some(dec!(678.90))); + + // Cleanup: drop sender to allow module to exit, then check for panics + drop(msg_tx); + module_handle.await.expect("Module task panicked").expect("Module run failed");For more robust synchronization, consider polling state changes with a timeout instead of fixed sleeps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/binary_options_tools/src/pocketoption/modules/resilient_parsing_tests.rs` around lines 34 - 46, The test currently uses tokio::spawn without tracking the JoinHandle and uses fixed tokio::time::sleep for synchronization which is flaky; change the spawn to capture the JoinHandle returned by tokio::spawn for the task running module.run() and, after the test actions, await that handle (and propagate any panic/error) so task failures are visible; replace the fixed tokio::time::sleep calls after msg_tx.send(Arc::new(msg_451)) and msg_tx.send(Arc::new(msg_42)) with a deterministic wait that polls state.balance (using state.balance.read().await in a loop with tokio::time::timeout) until it equals the expected dec!(123.45) and dec!(678.90) respectively, failing the test on timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/binary_options_tools/src/pocketoption/ssid.rs`:
- Around line 149-152: The double-encoding check currently only inspects the raw
input via trimmed, missing cases where JSON parsing yields a string (e.g.
42["auth","{...}"]) — update the logic to also inspect parsed when it is a JSON
string: if parsed.is_string(), get the inner string (parsed.as_str()) and run
the same quoted detection (starts_with('"') && ends_with('"') or
starts_with('\'')) and return CoreError::SsidParsing("Invalid SSID format:
double-encoding detected") when true; apply this same additional check to the
other validation block that mirrors lines 155-164 so both paths detect
wrapped-string double-encoding.
---
Outside diff comments:
In `@crates/core/src/general/client.rs`:
- Around line 269-282: The match on try_join3(listener_future, sender_future,
callback).await is evaluated but its result is discarded, causing the subsequent
Err(BinaryOptionsToolsError::ReconnectionAttemptFailure { ... }) to always be
returned; change the code so the match expression is returned (or add explicit
returns in each arm) so that the results from
Self::handle_reconnection_failure(connector, credentials, config, loops).await
propagate to the caller instead of being ignored, and only return
Err(BinaryOptionsToolsError::ReconnectionAttemptFailure { number: *loops, max:
config.get_max_allowed_loops()? }) when appropriate.
---
Nitpick comments:
In `@crates/binary_options_tools/src/pocketoption/modules/deals.rs`:
- Around line 373-384: The inline comment in the rule() function is incomplete:
update the comment above the Box::new(MultiPatternRule::new(...)) to indicate
this rule matches both "451-" prefixed messages and Socket.IO "42" prefixed
messages (any array-style payload like [...]) for the patterns
EV_UPDATE_CLOSED_DEALS, EV_UPDATE_OPENED_DEALS, and EV_SUCCESS_CLOSE_ORDER;
reference the MultiPatternRule and the EV_* constants so readers know the rule
covers both transport prefixes and array-formatted payloads.
- Around line 271-339: Extract the duplicated waiter-notification logic into a
helper method named notify_waiters_for_closed_deals(&mut self, deals: Vec<Deal>)
that: calls self.state.trade_state.update_closed_deals(deals.clone()).await,
iterates over each deal, removes waiters via
self.waiting_requests.remove(&deal.id), logs (info) the closed deal, and sends
Ok(deal.clone()) to each tx; then replace the repeated blocks inside
Message::Binary arms (ExpectedMessage::UpdateClosedDeals and
ExpectedMessage::SuccessCloseOrder fallback) and the similar code in
process_text_data to call notify_waiters_for_closed_deals(self, deals) instead
of duplicating the loop and send logic.
- Around line 247-258: The else branch after the arr.len() >= 2 check contains a
redundant has_placeholder computation that always evaluates false because
arr.iter().skip(1) is empty when arr.len() == 1; update the logic in the
DealsApiModule handling (the block referencing arr, has_placeholder, expected
and current_expected) to remove the has_placeholder iteration and simply check
arr.len() == 1 (or otherwise only detect the 1-step case) before setting
expected = current_expected and emitting the same tracing::debug message; keep
the existing behavior for setting expected and continuing, but eliminate the
needless iterator and boolean to simplify the code.
In `@crates/binary_options_tools/src/pocketoption/modules/keep_alive.rs`:
- Around line 94-97: The current check on ssid_str uses starts_with('4') and can
miss non-auth Socket.IO frames; change the logic in the keep-alive handling
(where ssid_str is modified) to test for the exact auth-frame prefix like
42["auth", (e.g., use starts_with with the full prefix string) and only wrap
when that exact prefix is absent; update the conditional around ssid_str and
adjust the debug message (in the same block that currently logs "SSID does not
start with Socket.IO packet type; wrapping in 42[\"auth\",...]") to reflect the
exact-prefix check and wrapping action.
In
`@crates/binary_options_tools/src/pocketoption/modules/resilient_parsing_tests.rs`:
- Around line 71-109: The test unsafely relies on tokio::time::sleep and an
untracked tokio::spawn for module.run(), making it flaky; change the pattern to
track and await the spawned task or signal completion explicitly instead of
sleeping: capture the JoinHandle from tokio::spawn(async move { let _ =
module.run().await; }) (or have module.run() return when idle), then after
sending msg_tx.send(...).await.unwrap() await that JoinHandle (or use a
oneshot/Notify that module.run() triggers after processing) before reading
state.trade_state.opened_deals.read().await and asserting the UUID; remove the
sleep and untracked spawn so the test deterministically waits for processing of
msg_deal_42.
- Around line 34-46: The test currently uses tokio::spawn without tracking the
JoinHandle and uses fixed tokio::time::sleep for synchronization which is flaky;
change the spawn to capture the JoinHandle returned by tokio::spawn for the task
running module.run() and, after the test actions, await that handle (and
propagate any panic/error) so task failures are visible; replace the fixed
tokio::time::sleep calls after msg_tx.send(Arc::new(msg_451)) and
msg_tx.send(Arc::new(msg_42)) with a deterministic wait that polls state.balance
(using state.balance.read().await in a loop with tokio::time::timeout) until it
equals the expected dec!(123.45) and dec!(678.90) respectively, failing the test
on timeout.
In `@crates/binary_options_tools/src/pocketoption/pocket_client.rs`:
- Around line 168-175: Remove the duplicated rustdoc block that precedes the
async constructor function PocketClient::new(ssid: impl ToString) ->
PocketResult<Self>; keep only the original/first doc comment for the constructor
and delete the repeated comment immediately above the pub async fn new(...)
declaration so the constructor has a single, canonical docstring.
In `@crates/binary_options_tools/src/pocketoption/state.rs`:
- Around line 335-342: The retain call in update_closed_deals is doing a
quadratic membership check by using ids.contains(id) on a Vec; change to build a
HashSet of ids (e.g., let id_set: HashSet<_> = deals.iter().map(|d|
d.id).collect()) and then call retain(|id, _| !id_set.contains(id)) on
self.opened_deals to get O(1) lookups; import std::collections::HashSet and
ensure the deal id type implements Hash + Eq.
In `@crates/core/src/general/client.rs`:
- Around line 291-306: The reconnection branch discards the error returned by
connector.connect(); capture that Err value and include it in the warning log so
failures are observable. Update the if-let to match on
connector.connect(...).await as Err(e) (or use match) and log the error (e)
along with the existing message (including variables loops and max_loops from
the surrounding scope), then keep the existing sleep and max-loop handling;
ensure BinaryOptionsToolsError::MaxReconnectAttemptsReached remains returned
when loops >= max_loops.
- Around line 295-300: The code calls config.get_sleep_interval()? twice; fetch
it once into a local variable (e.g., let sleep_interval =
config.get_sleep_interval()? already present) and reuse that variable in the
warn! message and in the sleep(Duration::from_secs(...)). Also reuse the
existing max_loops variable from config.get_max_allowed_loops()?; update the
warn! invocation and the sleep(...) call to reference sleep_interval instead of
calling config.get_sleep_interval()? again so the function is only invoked once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8e8cdc3-4fa7-4351-858d-26495f267951
📒 Files selected for processing (13)
.serena/project.ymlREADME.mdcrates/binary_options_tools/src/pocketoption/modules/balance.rscrates/binary_options_tools/src/pocketoption/modules/deals.rscrates/binary_options_tools/src/pocketoption/modules/keep_alive.rscrates/binary_options_tools/src/pocketoption/modules/mod.rscrates/binary_options_tools/src/pocketoption/modules/pending_trades.rscrates/binary_options_tools/src/pocketoption/modules/resilient_parsing_tests.rscrates/binary_options_tools/src/pocketoption/pocket_client.rscrates/binary_options_tools/src/pocketoption/ssid.rscrates/binary_options_tools/src/pocketoption/state.rscrates/binary_options_tools/src/pocketoption/utils.rscrates/core/src/general/client.rs
Comment on lines
+149
to
152
| // Security: Direct validation to prevent double-JSON injection | ||
| if (trimmed.starts_with('"') && trimmed.ends_with('"')) || trimmed.starts_with("'") { | ||
| return Err(CoreError::SsidParsing("Invalid SSID format: double-encoding detected".into())); | ||
| } |
Contributor
There was a problem hiding this comment.
Double-encoding detection misses the wrapped-string case.
Line 150 validates only the full input. For payloads like 42["auth","{...}"], parsed is still a quoted JSON string, so this path returns a generic JSON parse error instead of the intended "double-encoding detected" error (and likely fails the assertion on Line 386).
Proposed patch
let parsed = if let Some(stripped) = trimmed.strip_prefix(prefix) {
stripped.strip_suffix("]").ok_or_else(|| {
CoreError::SsidParsing("Error parsing ssid: missing closing bracket".into())
})?
} else {
trimmed
};
+
+ // Security: also reject double-encoded payload after removing Socket.IO wrapper
+ if (parsed.starts_with('"') && parsed.ends_with('"')) || parsed.starts_with('\'') {
+ return Err(CoreError::SsidParsing(
+ "Invalid SSID format: double-encoding detected".into(),
+ ));
+ }
let mut ssid: Demo = serde_json::from_str(parsed)
.map_err(|e| CoreError::SsidParsing(format!("JSON parsing error: {e}")))?;Also applies to: 155-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/binary_options_tools/src/pocketoption/ssid.rs` around lines 149 - 152,
The double-encoding check currently only inspects the raw input via trimmed,
missing cases where JSON parsing yields a string (e.g. 42["auth","{...}"]) —
update the logic to also inspect parsed when it is a JSON string: if
parsed.is_string(), get the inner string (parsed.as_str()) and run the same
quoted detection (starts_with('"') && ends_with('"') or starts_with('\'')) and
return CoreError::SsidParsing("Invalid SSID format: double-encoding detected")
when true; apply this same additional check to the other validation block that
mirrors lines 155-164 so both paths detect wrapped-string double-encoding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Overview
-1 balance hotfix and some minor fixes/alterations such as unsafe unwraps, and updating pattern for balance gathering
Changes
Type of Change
Validation
Describe how the changes were tested.
Environment
Checklist
Screenshots (Optional)
Add relevant visuals if applicable.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor