feat(data): capture authenticated Binance fees - #777
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a Binance fee snapshot CLI for Spot and USDⓈ-M markets. It parses authenticated responses, extracts fees and Spot rules, validates snapshots, and publishes SHA-256-protected artifacts with atomic no-clobber handling. ChangesBinance fee snapshot
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant BinanceFeeSnapshot
participant BinanceAPI
participant ArtifactStore
Operator->>BinanceFeeSnapshot: Provide market, symbol, and credentials
BinanceFeeSnapshot->>BinanceAPI: Send signed fee and rule requests
BinanceAPI-->>BinanceFeeSnapshot: Return fee and symbol data
BinanceFeeSnapshot->>ArtifactStore: Publish validated snapshot artifacts
ArtifactStore-->>BinanceFeeSnapshot: Return paths and SHA-256 hashes
BinanceFeeSnapshot-->>Operator: Print artifact metadata as JSON
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4a84ce704
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pshot-v2 # Conflicts: # agent-worktree.yml # rust_hft/tools/collector/Cargo.toml
…pshot-v2 # Conflicts: # agent-worktree.yml
…pshot-v2 # Conflicts: # agent-worktree.yml
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
rust_hft/tools/collector/src/bin/binance-fee-snapshot.rs (2)
268-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases for
parse_spot_rules.The test covers only the success path. Add cases for a non-
TRADINGstatus and for a missingNOTIONALfilter. Both branches fail closed and both guard the published instrument rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/bin/binance-fee-snapshot.rs` around lines 268 - 285, Extend the tests around parses_spot_trading_rules to cover parse_spot_rules returning no rules for a symbol whose status is not TRADING and for a symbol missing the NOTIONAL filter. Keep the existing successful case unchanged and assert the fail-closed result for both invalid payloads.
94-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the Binance error body in the failure message.
error_for_status()drops the response body. Binance returns a JSONcodeandmsgfor signature, timestamp, and permission failures. Without that body the operator sees only the HTTP status. Read the status and body, then fail with both.♻️ Proposed error surfacing
- let response = request.send().await?.error_for_status()?; - let payload: Value = response - .json() - .await - .context("Binance fee response is invalid JSON")?; + let response = request.send().await?; + let status = response.status(); + let body = response + .text() + .await + .context("Binance fee response body is unreadable")?; + if !status.is_success() { + bail!("Binance fee request failed with {status}: {body}"); + } + let payload: Value = + serde_json::from_str(&body).context("Binance fee response is invalid JSON")?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/bin/binance-fee-snapshot.rs` around lines 94 - 98, Update the request handling before payload parsing to preserve the HTTP status and read the response body when Binance returns an error, then fail with an error message containing both status and body instead of calling error_for_status() directly. Keep successful responses flowing into the existing JSON parsing and “Binance fee response is invalid JSON” context.rust_hft/tools/collector/src/binance_fee_artifact.rs (3)
289-312: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the read itself, not only the post-read length.
read_to_endallocates the full current file size. If the file grows between themetadata()call and the read, the process allocates more thanmax_bytesbefore the length check rejects it. Use aRead::takelimit so the bound applies during the read.♻️ Proposed hard bound
let mut bytes = Vec::with_capacity(opened.len() as usize); - file.read_to_end(&mut bytes)?; + std::io::Read::by_ref(&mut file) + .take(max_bytes + 1) + .read_to_end(&mut bytes)?; if bytes.len() as u64 != opened.len() { bail!("fee artifact changed during readback"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/binance_fee_artifact.rs` around lines 289 - 312, Update read_bound_file to enforce max_bytes during reading, not only after read_to_end completes: wrap the file reader with Read::take using max_bytes plus one byte, read through that bounded reader, and reject when the resulting length exceeds max_bytes before accepting the data. Preserve the existing metadata and post-read size validation.
376-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
usdmpublication test and a duplicate-batch test.The single test covers only the
spotbranch. Theusdmvalidation branch at lines 69-74 and the no-clobber rename path at lines 171-173 and 209 stay untested. Add one test that publishes ausdmsnapshot and one that publishes the same snapshot twice and asserts the second call fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/binance_fee_artifact.rs` around lines 376 - 427, Add a test covering publication of a BinanceFeeSnapshot with market set to “usdm”, asserting the resulting artifact validates successfully through the USDⓈ-M branch. Add a separate test that calls publish_fee_snapshot twice with the same snapshot and asserts the second call fails, covering the no-clobber rename behavior. Reuse the existing temporary-directory and snapshot setup patterns.
174-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet an explicit mode for the staging directory before rename.
tempfile::Builder::new().tempdir_in(...)does not define a fixed directory mode, while the Binance reference publisher creates missing lake components withDirBuilder::new().mode(0o700). Set the staging directory mode to0o700beforefs::renameso the publishedbatch=partition does not inherit a more permissive umask-mode by accident.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/binance_fee_artifact.rs` around lines 174 - 209, Set the staging directory permissions to 0o700 immediately after creating it with tempfile::Builder in the artifact publishing flow, before rename_noreplace publishes it. Use the existing staging path and ensure the explicit mode is applied before fs::File::open(...).sync_all() and the final rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust_hft/tools/collector/src/bin/binance-fee-snapshot.rs`:
- Around line 224-231: Update decimal_field so its missing and invalid field
context does not specifically identify a fee response when the helper also
processes exchangeInfo filters; either accept a source-label parameter and use
it in both context messages, or replace the wording with neutral “Binance
response” text while preserving the existing field validation.
- Around line 56-59: Validate the normalized symbol’s charset immediately after
constructing it in the command setup, before any exchangeInfo or signed request
is built or sent. Reuse the same allowed-character rule enforced by
BinanceFeeSnapshot::validate so invalid symbols are rejected early while
preserving the existing absolute output-root check and validation behavior.
- Around line 135-141: Update required_env to read variables with
std::env::var_os instead of std::env::var, converting valid values to String and
returning a generic missing/invalid-variable error without propagating VarError.
Preserve the existing empty-after-trimming validation while ensuring non-UTF-8
secret values never appear in anyhow error output.
- Line 214: Update the notional filter selection in the snapshot-building flow
to use only filter("NOTIONAL") and propagate its absence as an error; remove the
or_else fallback to filter("MIN_NOTIONAL") so obsolete filters are rejected
rather than silently accepted.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/bin/binance-fee-snapshot.rs`:
- Around line 268-285: Extend the tests around parses_spot_trading_rules to
cover parse_spot_rules returning no rules for a symbol whose status is not
TRADING and for a symbol missing the NOTIONAL filter. Keep the existing
successful case unchanged and assert the fail-closed result for both invalid
payloads.
- Around line 94-98: Update the request handling before payload parsing to
preserve the HTTP status and read the response body when Binance returns an
error, then fail with an error message containing both status and body instead
of calling error_for_status() directly. Keep successful responses flowing into
the existing JSON parsing and “Binance fee response is invalid JSON” context.
In `@rust_hft/tools/collector/src/binance_fee_artifact.rs`:
- Around line 289-312: Update read_bound_file to enforce max_bytes during
reading, not only after read_to_end completes: wrap the file reader with
Read::take using max_bytes plus one byte, read through that bounded reader, and
reject when the resulting length exceeds max_bytes before accepting the data.
Preserve the existing metadata and post-read size validation.
- Around line 376-427: Add a test covering publication of a BinanceFeeSnapshot
with market set to “usdm”, asserting the resulting artifact validates
successfully through the USDⓈ-M branch. Add a separate test that calls
publish_fee_snapshot twice with the same snapshot and asserts the second call
fails, covering the no-clobber rename behavior. Reuse the existing
temporary-directory and snapshot setup patterns.
- Around line 174-209: Set the staging directory permissions to 0o700
immediately after creating it with tempfile::Builder in the artifact publishing
flow, before rename_noreplace publishes it. Use the existing staging path and
ensure the explicit mode is applied before fs::File::open(...).sync_all() and
the final rename.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d67d571-0ab8-45da-ade5-51fb003c15e9
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
agent-worktree.ymlrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/binance-fee-snapshot.rsrust_hft/tools/collector/src/binance_fee_artifact.rsrust_hft/tools/collector/src/lib.rs
…pshot-v2 # Conflicts: # agent-worktree.yml
Change contract
Capture authenticated Binance Spot and USD-M account commission schedules as immutable data/manifest/_SUCCESS triplets; Spot snapshots also bind tick size, step size, and minimum notional from official exchangeInfo.
Issue relationship
Refs #773
Out of scope
Research materialization, periodic production scheduling, OSS transport, order submission, and Live trading.
Dependencies and merge order
Stacked on #774 for the USD-M Reference V3 contract. Retarget to main and rerun exact-head CI after #774 merges.
Focused validation
Rollout and rollback
No production schedule or credentials are changed by this PR. A later collector rollout must use a read-only API key, produce a verified triplet for each market, upload/read back it, and keep Live disabled.
Scope exception
None.
Summary by CodeRabbit
New Features
Bug Fixes