Fixed parsing in get_candle with context for Pocket Option API changes #23
Conversation
The get_candle function was failing due to a 16s timeout. The receiver was waiting for a LoadHistoryPeriod stream, which should have been matched by MessageInfo::LoadHistoryPeriod. However, in the updated Pocket Option WebSocket API, the response format changed and was instead caught as MessageInfo::UpdateStream.
Updated the parsing logic to handle this case by attempting to deserialize LoadHistoryPeriodResult under UpdateStream:
rust
Copy
Edit
MessageInfo::UpdateStream => {
if let Ok(stream) = from_str::<UpdateStream>(&data) {
return Self::UpdateStream(stream);
}
if let Ok(stream) = from_str::<LoadHistoryPeriodResult>(&data) {
return Self::LoadHistoryPeriod(stream);
}
}
This ensures compatibility with the new API response structure and prevents the timeout issue.
|
""" WalkthroughThe change adds support for a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSocketHandler
participant EventEmitter
Client->>WebSocketHandler: receive JSON message
alt event == "loadHistoryPeriod" or "loadHistoryPeriodFast"
WebSocketHandler->>EventEmitter: emit candles_received with event data
else
WebSocketHandler->>EventEmitter: handle other events
end
sequenceDiagram
participant User
participant PocketOptionClient
User->>PocketOptionClient: call get_candles_advanced(asset, time, period, offset)
PocketOptionClient->>PocketOptionClient: send request to fetch candles with parameters
PocketOptionClient-->>User: return candle data JSON
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/ch/ro/chrono, error: Permission denied (os error 13) Caused by: ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Let me review it with @Rick-29 |
|
it looks good but currently i am working on a 100% rework on the core of the crate and the data handling methods. |
@Rick-29 I’ll add it to this PR. 
|
|
Ok, good to know, currently i am implementing supoort for buying and selling, then i will work on checking trade results, i will work on receiving the data. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
BinaryOptionsToolsJs/index.d.ts(2 hunks)BinaryOptionsToolsJs/src/pocketoption.rs(1 hunks)Cargo.toml(1 hunks)crates/binary_options_tools/src/pocketoption/parser/message.rs(5 hunks)crates/binary_options_tools/src/pocketoption/types/info.rs(1 hunks)crates/binary_options_tools/src/pocketoption/types/update.rs(2 hunks)crates/core/data/client_enhanced.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/core/data/client_enhanced.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- crates/binary_options_tools/src/pocketoption/parser/message.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
BinaryOptionsToolsJs/src/pocketoption.rs (4)
crates/binary_options_tools/src/pocketoption/pocket_client.rs (1)
get_candles_advanced(441-477)BinaryOptionsToolsV2/BinaryOptionsToolsV2/pocketoption/syncronous.py (1)
get_candles_advanced(146-169)BinaryOptionsToolsV2/BinaryOptionsToolsV2/pocketoption/asyncronous.py (1)
get_candles_advanced(248-271)BinaryOptionsToolsV2/src/pocketoption.rs (1)
get_candles_advanced(173-187)
🔇 Additional comments (6)
crates/binary_options_tools/src/pocketoption/types/info.rs (1)
28-28: LGTM! Clean enum variant addition.The new
LoadHistoryPeriodFastvariant is correctly placed and follows the existing naming conventions.BinaryOptionsToolsJs/src/pocketoption.rs (1)
281-306: LGTM! Well-implemented method with comprehensive documentation.The
get_candles_advancedmethod follows established patterns for async methods in this file. The documentation includes a clear example, and error handling is consistent with other methods.BinaryOptionsToolsJs/index.d.ts (2)
283-302: LGTM! Accurate TypeScript declarations with good documentation.The
getCandlesAdvancedmethod declaration correctly reflects the Rust implementation with proper parameter types and comprehensive documentation.
581-582: LGTM! Simple and correct method declaration.The
getServerTimemethod declaration is accurate and follows the established pattern.crates/binary_options_tools/src/pocketoption/types/update.rs (2)
39-39: LGTM! Consistent enum variant addition.The new
ProcessedNewvariant follows the established pattern and naming convention.
43-51: LGTM! Well-structured candle data representation.The
ProcessedCandleNewstruct properly usesi64for the timestamp to avoid precision issues, and the OHLC fields are appropriately typed asf64.
|
I figured out why the get_candle result was returning an UpdateStream type. The issue was caused by a missing LoadHistoryPeriodFast handler, which led the data to be marked as UpdateStream. After adding support for LoadHistoryPeriodFast, it now works as expected without needing the case patch on UpdateStream. I also exposed get_candle_advanced to provide an option for retrieving candles based on time and offset without relying on server time. Additionally, I fixed the Cargo.toml file to exclude the JS and Python libraries, preventing duplication issues when working with Rust. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/JsPublish.yml (2)
28-32: Explicit pinning tomacos-13is sensible but may age quicklyPinning the Intel runner to
macos-13guarantees determinism today, yet GitHub often deprecates older macOS images with short notice. Keep an eye on deprecation emails and be prepared to move tomacos-14(or later) to avoid sudden job failures.
48-51: Consider pinning the Apple-silicon runner to a concrete imageWhile the comment clarifies that this leg builds on ARM64, leaving the host as
macos-latestmeans the image will float (currentlymacos-14-arm64). A future bump (e.g.,macos-15) could introduce SDK or tool-chain surprises.- - host: macos-latest # (arm64 processor) + - host: macos-14 # (arm64 processor, Apple Silicon)Pinning avoids unwelcome CI breakage at release time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/JsPublish.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/JsPublish.yml (1)
214-217: Align test matrix with build matrix for stabilityYou pinned the build job to
macos-13but left tests previously on the floating runner; updating tests to the same pinned image keeps parity—good move. As above, watch for deprecation announcements.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/CI.yml(1 hunks).github/workflows/JsPublish.yml(5 hunks)BinaryOptionsToolsV2/Cargo.toml(1 hunks)Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- BinaryOptionsToolsV2/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- .github/workflows/JsPublish.yml
.github/workflows/CI.yml
Outdated
| # 🛠 Enable vendored OpenSSL for all platforms as fallback | ||
| - name: Set OPENSSL vendored fallback | ||
| run: | | ||
| echo "OPENSSL_NO_VENDOR=0" >> $GITHUB_ENV | ||
| echo "OPENSSL_STATIC=1" >> $GITHUB_ENV |
There was a problem hiding this comment.
OPENSSL_NO_VENDOR here disables – not enables – vendored OpenSSL
The comment says “Enable vendored OpenSSL”, but merely setting OPENSSL_NO_VENDOR (even to 0) disables vendoring in the openssl crate.
Remove it and keep only OPENSSL_STATIC=1 (or switch to OPENSSL_VENDOR=1 if you need to force vendoring).
- echo "OPENSSL_NO_VENDOR=0" >> $GITHUB_ENV
echo "OPENSSL_STATIC=1" >> $GITHUB_ENV
+ # Leave OPENSSL_NO_VENDOR unset so the build can fall back to the vendored copy📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 🛠 Enable vendored OpenSSL for all platforms as fallback | |
| - name: Set OPENSSL vendored fallback | |
| run: | | |
| echo "OPENSSL_NO_VENDOR=0" >> $GITHUB_ENV | |
| echo "OPENSSL_STATIC=1" >> $GITHUB_ENV | |
| # 🛠 Enable vendored OpenSSL for all platforms as fallback | |
| - name: Set OPENSSL vendored fallback | |
| run: | | |
| echo "OPENSSL_STATIC=1" >> $GITHUB_ENV | |
| # Leave OPENSSL_NO_VENDOR unset so the build can fall back to the vendored copy |
🤖 Prompt for AI Agents
In .github/workflows/CI.yml around lines 56 to 60, the environment variable
OPENSSL_NO_VENDOR is incorrectly set to 0, which disables vendored OpenSSL
instead of enabling it. To fix this, remove the line setting OPENSSL_NO_VENDOR
entirely and keep only the line setting OPENSSL_STATIC=1, or alternatively
replace it with OPENSSL_VENDOR=1 if you want to explicitly enable vendored
OpenSSL.
.github/workflows/CI.yml
Outdated
| - name: Setup cross-compilation for armv7 | ||
| if: matrix.platform.target == 'armv7' | ||
| run: | | ||
| sudo dpkg --add-architecture armhf | ||
| sudo apt-get update | ||
| sudo apt-get install -y gcc-arm-linux-gnueabihf libssl-dev:armhf pkg-config | ||
| echo "OPENSSL_DIR=/usr/arm-linux-gnueabihf" >> $GITHUB_ENV | ||
| echo "PKG_CONFIG_PATH=/usr/arm-linux-gnueabihf/pkgconfig" >> $GITHUB_ENV | ||
| echo "CC_armv7_unknown_linux_gnueabihf=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | ||
| echo "PKG_CONFIG_ALLOW_CROSS=1" >> $GITHUB_ENV | ||
There was a problem hiding this comment.
Missing Rust target & incorrect linker env name break the armv7 build
maturin will invoke cargo, but rustup is never told to install the armv7-unknown-linux-gnueabihf target, and Cargo will look for CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER, not CC_armv7_unknown_linux_gnueabihf.
Without both, the cross-compile step will fail at link time.
@@
- sudo apt-get install -y gcc-arm-linux-gnueabihf libssl-dev:armhf pkg-config
+ sudo apt-get install -y gcc-arm-linux-gnueabihf libssl-dev:armhf pkg-config
+
+ # Install the Rust std lib for the target
+ rustup target add armv7-unknown-linux-gnueabihf
@@
- echo "CC_armv7_unknown_linux_gnueabihf=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV
+ echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Setup cross-compilation for armv7 | |
| if: matrix.platform.target == 'armv7' | |
| run: | | |
| sudo dpkg --add-architecture armhf | |
| sudo apt-get update | |
| sudo apt-get install -y gcc-arm-linux-gnueabihf libssl-dev:armhf pkg-config | |
| echo "OPENSSL_DIR=/usr/arm-linux-gnueabihf" >> $GITHUB_ENV | |
| echo "PKG_CONFIG_PATH=/usr/arm-linux-gnueabihf/pkgconfig" >> $GITHUB_ENV | |
| echo "CC_armv7_unknown_linux_gnueabihf=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| echo "PKG_CONFIG_ALLOW_CROSS=1" >> $GITHUB_ENV | |
| - name: Setup cross-compilation for armv7 | |
| if: matrix.platform.target == 'armv7' | |
| run: | | |
| sudo dpkg --add-architecture armhf | |
| sudo apt-get update | |
| sudo apt-get install -y gcc-arm-linux-gnueabihf libssl-dev:armhf pkg-config | |
| # Install the Rust std lib for the target | |
| rustup target add armv7-unknown-linux-gnueabihf | |
| echo "OPENSSL_DIR=/usr/arm-linux-gnueabihf" >> $GITHUB_ENV | |
| echo "PKG_CONFIG_PATH=/usr/arm-linux-gnueabihf/pkgconfig" >> $GITHUB_ENV | |
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| echo "PKG_CONFIG_ALLOW_CROSS=1" >> $GITHUB_ENV |
🤖 Prompt for AI Agents
In .github/workflows/CI.yml around lines 44 to 55, the Rust target
armv7-unknown-linux-gnueabihf is not installed via rustup, and the environment
variable for the linker is incorrectly named CC_armv7_unknown_linux_gnueabihf
instead of CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER. Fix this by adding
a step to install the Rust target using rustup and rename the linker environment
variable to CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER with the correct
linker command.
|
@davidantoon if you want to help on developping this crate (for which i am really glad), i would prefer if you checked the pocketoption_pre folder since it's where i am working currently, my new architecture is composed of 2 kind of modules (mainly), the lightweight modules that are automatic systems for example a keep alive module that sends a ping each 20 seconds, an update balance module that listens for messages of the server, and the api modules, that are composed of 1 module that is run in it's own thread and a handle that comunicates with the module, the handle is intended to be used by the user in functions like buy sell, check_results, etc... Also I updated on github the latest code i have and will not work on it untill tomorrow afternoon. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/CI.yml (3)
147-151: Condition still checksenv.PYPI_API_TOKEN– step will never runThe
env:block is applied after theif:is evaluated, so this is always empty.
Use the secret directly (identical to the past review).- if: env.PYPI_API_TOKEN != '' + if: secrets.PYPI_API_TOKEN != ''
184-188: Sameif:issue on macOS publish step – apply the fix here as well.
227-230: Release job uses the same fragile guard – switch tosecrets.PYPI_API_TOKENand you can drop the intermediateenv:indirection entirely.
🧹 Nitpick comments (1)
.github/workflows/CI.yml (1)
58-63: Perl-dependency step runs for every Linux targetOnly armv7 requires the vendored OpenSSL tool-chain; running
apt-get,cpanm, etc. on x86_64, aarch64 … wastes minutes. Guard it the same way as the OpenSSL step:- - name: Install perl dependencies for OpenSSL + - name: Install perl dependencies for OpenSSL + if: matrix.platform.target == 'armv7'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/CI.yml(7 hunks)BinaryOptionsToolsJs/Cargo.toml(1 hunks)BinaryOptionsToolsV2/Cargo.toml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- BinaryOptionsToolsJs/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- BinaryOptionsToolsV2/Cargo.toml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/CI.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/CI.yml (2)
24-24: Good call disablingfail-faston the matrix
Avoids aborting the whole job when a single target flakes.
33-34: Runner downgraded toubuntu-22.04only for armv7 – OK
Matches the need for a newer cross-compiler while keeping other targets on the default runner.
| - runner: ubuntu-22.04 | ||
| target: armv7 | ||
| steps: |
There was a problem hiding this comment.
musllinux job: armv7 lacks cross-compile setup
Unlike the glibc job, the musllinux armv7 leg has no steps to install the Rust target, tool-chain or linker env vars, so it will fail at compile/link time. Mirror the fixes from the Linux job.
🤖 Prompt for AI Agents
In .github/workflows/CI.yml around lines 93 to 95, the musllinux job for armv7
is missing the setup steps for cross-compilation such as installing the Rust
target, toolchain, and setting linker environment variables. To fix this, add
the necessary steps to install the Rust armv7 target, configure the appropriate
cross-compilation toolchain, and set the linker environment variables, mirroring
the setup used in the glibc Linux job for armv7.
.github/workflows/CI.yml
Outdated
| - name: Configure vendored OpenSSL for armv7 | ||
| if: matrix.platform.target == 'armv7' | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y gcc-arm-linux-gnueabihf | ||
| # Force openssl-sys to build vendored OpenSSL | ||
| echo "OPENSSL_NO_VENDOR=0" >> $GITHUB_ENV | ||
| echo "OPENSSL_STATIC=1" >> $GITHUB_ENV | ||
| # Use the cross-compiler as CC and tell Cargo to use it | ||
| echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | ||
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | ||
There was a problem hiding this comment.
Vendored-OpenSSL block still has three blocking problems
OPENSSL_NO_VENDOR=0disables vendoring even when set to0– the mere presence of the var turns it off (see past review).- Overwriting global
CCforces the host build (build-scripts, proc-macros) to use the ARM cross-compiler and will fail.
UseCARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_CC(or thelinkerkey already set) instead. - The Rust std-lib for the target is still not installed (
rustup target add …), so the build will fail before link time.
sudo apt-get install -y gcc-arm-linux-gnueabihf
+
+ # Install Rust std-lib for the target
+ rustup target add armv7-unknown-linux-gnueabihf
-
- echo "OPENSSL_NO_VENDOR=0" >> $GITHUB_ENV
- echo "OPENSSL_STATIC=1" >> $GITHUB_ENV
-
- echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV
+ # Force openssl-sys to build its vendored copy statically
+ echo "OPENSSL_VENDOR=1" >> $GITHUB_ENV
+ echo "OPENSSL_STATIC=1" >> $GITHUB_ENV
+
+ # Tell Cargo which compiler to use *only* for the armv7 target
+ echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENVAlso, YAML-lint flags trailing whitespace on lines 49 & 53 – remove to keep the file lint-clean.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Configure vendored OpenSSL for armv7 | |
| if: matrix.platform.target == 'armv7' | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y gcc-arm-linux-gnueabihf | |
| # Force openssl-sys to build vendored OpenSSL | |
| echo "OPENSSL_NO_VENDOR=0" >> $GITHUB_ENV | |
| echo "OPENSSL_STATIC=1" >> $GITHUB_ENV | |
| # Use the cross-compiler as CC and tell Cargo to use it | |
| echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| - name: Configure vendored OpenSSL for armv7 | |
| if: matrix.platform.target == 'armv7' | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y gcc-arm-linux-gnueabihf | |
| # Install Rust std-lib for the target | |
| rustup target add armv7-unknown-linux-gnueabihf | |
| # Force openssl-sys to build its vendored copy statically | |
| echo "OPENSSL_VENDOR=1" >> $GITHUB_ENV | |
| echo "OPENSSL_STATIC=1" >> $GITHUB_ENV | |
| # Tell Cargo which compiler to use *only* for the armv7 target | |
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/CI.yml around lines 44 to 57, fix the vendored OpenSSL
configuration by removing the OPENSSL_NO_VENDOR variable entirely since its
presence disables vendoring regardless of value. Replace the global CC
environment variable with CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_CC to avoid
forcing the host build to use the ARM cross-compiler. Add a step to install the
Rust standard library for the armv7 target using rustup target add before the
build step. Also, remove trailing whitespace on lines 49 and 53 to pass YAML
lint checks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
.github/workflows/CI.yml (5)
44-53: armv7 step still missesrustup target addand wrongly overrides globalCCPrevious reviews already pointed this out.
Without installing thearmv7-unknown-linux-gnueabihftarget, the build will fail long before link time, and exporting a globalCCforces the host build to use the cross-compiler. Use onlyCARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER(already present) and add the rustup target install.
54-60:OPENSSL_NO_VENDOR=0still disables vendored OpenSSLSetting this var — regardless of value — tells the
opensslcrate not to vendor.
Drop it entirely or switch toOPENSSL_VENDOR=1.
Same feedback as the earlier review.
151-156:if: env.PYPI_API_TOKENnever evaluates as intendedThe
env:block is applied after the condition is parsed, so this step is always skipped.
Gate on the secret directly:if: secrets.PYPI_API_TOKEN != ''.
188-193: Same misplacedif:condition as in Windows jobUse
if: secrets.PYPI_API_TOKEN != ''here as well.
231-235: Release job:if:check suffers from the same problemReplace with
if: secrets.PYPI_API_TOKEN != ''to ensure the upload actually runs when the secret is present.
🧹 Nitpick comments (1)
.github/workflows/CI.yml (1)
49-60: YAML-lint: trailing whitespace on lines 49 / 56 / 60Remove the stray spaces to keep the workflow lint-clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/CI.yml(7 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/CI.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/CI.yml (3)
204-208: Same token-guard issue in the macOS job
Apply the identicalsecrets.PYPI_API_TOKENfix here as well.
167-172: Publish step will never run – wrongif:expression
env.PYPI_API_TOKENisn’t defined when the condition is evaluated. Gate on the secret instead:- if: env.PYPI_API_TOKEN != '' + if: secrets.PYPI_API_TOKEN != ''
247-251: UndefinedPYPI_API_TOKENin release jobThe env block never assigns
PYPI_API_TOKEN, yet you reference it twice.- if: env.PYPI_API_TOKEN != '' + if: secrets.PYPI_API_TOKEN != '' @@ - TWINE_PASSWORD: ${{ env.PYPI_API_TOKEN }} + TWINE_PASSWORD: ${{ secrets.PYPI_API_TOKEN }}
🧹 Nitpick comments (1)
.github/workflows/CI.yml (1)
45-59: YAML-lint failures: trailing whitespace on 54 & 59
These two spaces are the only things blocking the linter.- perl␠ + perl @@ - echo "OPENSSL_STATIC=1" >> $GITHUB_ENV␠ + echo "OPENSSL_STATIC=1" >> $GITHUB_ENV
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/CI.yml(8 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/CI.yml
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/CI.yml (2)
24-24: Good call disabling matrix fail-fast
Turning offfail-fastimproves diagnostics across the whole matrix.
236-236: Release job may miss Linux wheelsRemoving
linux&musllinuxfromneedsallows the release job to start before those artifacts exist:- needs: [ macos, windows ] # [macos, windows, linux, musllinux] + needs: [ macos, windows, linux, musllinux ]
.github/workflows/CI.yml
Outdated
| - name: Configure vendored OpenSSL for armv7 | ||
| if: matrix.platform.target == 'armv7' | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y gcc-arm-linux-gnueabihf | ||
| # Use the cross-compiler as CC and tell Cargo to use it | ||
| echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | ||
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV |
There was a problem hiding this comment.
🛠️ Refactor suggestion
armv7 leg still incomplete & over-scoped CC
- Add the target std-lib or the build aborts before linking:
rustup target add armv7-unknown-linux-gnueabihf - Don’t overwrite global
CC; restrict the cross-compiler to the target:
+ rustup target add armv7-unknown-linux-gnueabihf
- echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENVThe target-specific var you already set is sufficient.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Configure vendored OpenSSL for armv7 | |
| if: matrix.platform.target == 'armv7' | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y gcc-arm-linux-gnueabihf | |
| # Use the cross-compiler as CC and tell Cargo to use it | |
| echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV | |
| - name: Configure vendored OpenSSL for armv7 | |
| if: matrix.platform.target == 'armv7' | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y gcc-arm-linux-gnueabihf | |
| # Install the Rust standard library for the armv7 target | |
| rustup target add armv7-unknown-linux-gnueabihf | |
| # Use the target-specific linker only (no global CC override) | |
| echo "CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 65-65: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/CI.yml around lines 60 to 68, the workflow is missing the
addition of the Rust target armv7-unknown-linux-gnueabihf, which causes build
failures before linking. Add a step to run `rustup target add
armv7-unknown-linux-gnueabihf` before building. Also, remove the line that sets
the global CC environment variable to the cross-compiler and only keep the
target-specific linker environment variable to avoid over-scoping the compiler
setting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/CI.yml (2)
60-69: armv7 cross-compile block still misses Rust target and over-scopesCCExactly the same issues were pointed out in previous review rounds:
rustup target add armv7-unknown-linux-gnueabihfis still missing – the build will stop before linking.- Overwriting global
CCforces host-build crates (proc-macros, build-scripts) to use the ARM cross-compiler and will break.
Keep only the target-specific variable that Cargo actually consults (CARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_LINKER), or use the more explicitCARGO_TARGET_ARMV7_UNKNOWN_LINUX_GNUEABIHF_CC.+ # Install the Rust std-lib for the target + rustup target add armv7-unknown-linux-gnueabihf - echo "CC=arm-linux-gnueabihf-gcc" >> $GITHUB_ENV
167-172:if: env.PYPI_API_TOKENstill never matches – step is always skipped
env.PYPI_API_TOKENis populated after theif:is evaluated, so the publish steps never run.
Gate directly on the secret, as previously suggested:- if: env.PYPI_API_TOKEN != '' + if: secrets.PYPI_API_TOKEN != ''Also applies to: 204-208, 247-252
🧹 Nitpick comments (1)
.github/workflows/CI.yml (1)
45-56: Trailing whitespace trips YAML-lintLines 54 and 65 still contain trailing spaces which will fail
yamllint. Strip them to keep the workflow lint-clean.- perl␠ + perl … - +Also applies to: 63-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/CI.yml(8 hunks)BinaryOptionsToolsJs/Cargo.toml(1 hunks)BinaryOptionsToolsV2/Cargo.toml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- BinaryOptionsToolsJs/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- BinaryOptionsToolsV2/Cargo.toml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/CI.yml
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
i will check, @davidantoon what is your discord username so we can talk there as i want to ask you some questions, and also are you sure the |
Yes it's me davidantoon on discord, let's move this conversation to discord channel. |
|
Awesome, I will keep this PR open. If needed let me know if it needs to be closed. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/JsPublish.yml(8 hunks)crates/binary_options_tools/src/pocketoption/types/update.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/JsPublish.yml
🔇 Additional comments (7)
crates/binary_options_tools/src/pocketoption/types/update.rs (7)
2-2: LGTM - Appropriate imports for new functionality.The new imports for
fmtandBTreeMapare correctly added to support the Display trait implementation and candle aggregation logic respectively.Also applies to: 4-4
24-24: LGTM - New field for snapshot candles.The
candlesfield addition toUpdateHistoryNewFastappropriately supports the new fast history loading feature with snapshot data.
40-43: LGTM - New enum variants for enhanced candle types.The new
ProcessedNewandUpdateNewvariants appropriately extend theCandleenum to support the enhanced candle data structures.
45-53: Good design choice for timestamp handling.Using
i64for the timestamp inProcessedCandleNewavoids potential precision issues that could occur with floating-point timestamps. The field naming and structure are appropriate for the new candle format.
91-99: LGTM - Consistent structure with proper timestamp type.The
UpdateCandleNewstruct appropriately usesDateTime<Utc>for the timestamp field, maintaining consistency with the existing codebase patterns.
185-197: Excellent error handling for timestamp conversion.The implementation properly addresses the previous review comment by using
unwrap_or_elsewith a fallback to current time and logging the error. This prevents panics while maintaining functionality.
199-208: LGTM - Direct timestamp usage for UpdateNew variant.The implementation correctly uses the
DateTime<Utc>timestamp directly from theUpdateCandleNewstruct, avoiding unnecessary conversions.
| // 1) snapshot candles (descending) | ||
| let existing: Vec<DataCandle> = self.candles.iter().map(DataCandle::from).collect(); | ||
| let mut history_dc: Vec<DataCandle> = self.history.iter().map(DataCandle::from).collect(); | ||
|
|
||
| history_dc.sort_unstable_by_key(|c| c.time.timestamp()); | ||
|
|
||
| // 3) cutoff: up through the last snapshot candle | ||
| let last_cutoff = existing | ||
| .first() | ||
| .map(|c| c.time.timestamp()) | ||
| .unwrap_or_else(|| { | ||
| let ts = history_dc | ||
| .first() | ||
| .expect("history cannot be empty") | ||
| .time | ||
| .timestamp(); | ||
| (ts / self.period) * self.period - self.period | ||
| }); | ||
|
|
||
| // 4) aggregate into period-buckets via a BTreeMap | ||
| let mut buckets: BTreeMap<i64, (f64, f64, f64, f64)> = BTreeMap::new(); | ||
| for dc in &history_dc { | ||
| let ts = dc.time.timestamp(); | ||
| let bucket = (ts / self.period) * self.period; | ||
| if bucket <= last_cutoff { | ||
| continue; | ||
| } | ||
|
|
||
| let price = dc.close; // or whichever price you want | ||
| buckets | ||
| .entry(bucket) | ||
| .and_modify(|e| { | ||
| e.1 = e.1.max(price); // high | ||
| e.2 = e.2.min(price); // low | ||
| e.3 = price; // close | ||
| }) | ||
| .or_insert((price, price, price, price)); | ||
| } | ||
|
|
||
| // 5) build new candles (descending) and prepend snapshot | ||
| let mut new_candles: Vec<DataCandle> = buckets | ||
| .into_iter() | ||
| .rev() | ||
| .map(|(bucket_ts, (open, high, low, close))| { | ||
| let dt = DateTime::from_timestamp(bucket_ts, 0).unwrap(); | ||
| DataCandle { | ||
| time: dt, | ||
| open, | ||
| high, | ||
| low, | ||
| close, | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| new_candles.extend(existing); | ||
| new_candles | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex method needs refactoring for maintainability.
The candle_data method implements sophisticated aggregation logic but has several issues that should be addressed:
- Panic risk: Line 247 uses
expect("history cannot be empty")which will panic if the history is empty - Another panic risk: Line 278 uses
unwrap()onDateTime::from_timestampwithout error handling - High complexity: The method is quite large and handles multiple responsibilities (sorting, filtering, bucketing, aggregating)
Apply these fixes:
- .first()
- .expect("history cannot be empty")
+ .first()
+ .ok_or_else(|| "history cannot be empty")?- let dt = DateTime::from_timestamp(bucket_ts, 0).unwrap();
+ let dt = DateTime::from_timestamp(bucket_ts, 0).unwrap_or_else(|| {
+ eprintln!("Invalid bucket timestamp {}, using current time", bucket_ts);
+ Utc::now()
+ });Consider breaking this method into smaller, focused functions:
sort_and_filter_historyaggregate_into_bucketscreate_candles_from_bucketsmerge_snapshot_and_aggregated
This would improve readability, testability, and maintainability.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/binary_options_tools/src/pocketoption/types/update.rs from lines 234
to 291, the candle_data method is too complex and has panic risks due to using
expect on empty history and unwrap on DateTime::from_timestamp. Refactor by
splitting the method into smaller functions: one to sort and filter history
safely without panics, another to aggregate data into buckets, a third to create
candles from these buckets with proper error handling instead of unwrap, and a
final one to merge snapshot and aggregated candles. Replace expect and unwrap
with error handling or safe defaults to avoid panics and improve
maintainability.
4 participants
📝 Description
✅ Ensures compatibility with updated Pocket Option API.
Fixes: #17
Summary by CodeRabbit
Summary by CodeRabbit