Ship credentialed Binance fee snapshot timers - #793
Conversation
…ding # Conflicts: # agent-worktree.yml
…codex/binance-fee-oss-transport # Conflicts: # agent-worktree.yml
…codex/binance-fee-oss-transport
…-transport # Conflicts: # agent-worktree.yml
…temd # Conflicts: # agent-worktree.yml
|
Exact-head review requested for 6b1441d. Please verify systemd credential isolation, 60-second freshness cadence, fail-closed missing-credential behavior, fixed OSS destination, and that merge alone cannot enable production units. |
|
Warning Review limit reached
Next review available in: 3 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 (3)
📝 WalkthroughWalkthroughThe PR adds credentialed Binance Spot and USD-M fee snapshot services, periodic OSS uploads, persistent upload failure tracking, collector health checks, release-contract tests, and deterministic ACR release metadata. ChangesBinance fee release
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotTimer as Snapshot timers
participant Snapshot as binance-fee-snapshot
participant Spool as Binance fee spool
participant Upload as binance-fee-upload
participant OSS as Alibaba Cloud OSS
participant Health as monday-collector-health.sh
SnapshotTimer->>Snapshot: Run Spot and USD-M snapshots
Snapshot->>Spool: Write fee artifacts
Upload->>Spool: Discover pending artifacts
Upload->>OSS: Upload verified artifacts
Health->>SnapshotTimer: Check timer and service state
Health->>Upload: Check upload status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Exact-head review requested for 811927b. In addition to the credentialed units, the ACR release now publishes a digest-bound fee release receipt covering both binaries, the control manifest, and the deterministic control archive. Please review this exact head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b07d29253
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e7f33d333
ℹ️ 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".
…temd # Conflicts: # agent-worktree.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a81505f3c0
ℹ️ 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".
|
@codex review exact head after the initial failure-count fix. A fresh fee lane now breaches on nonzero failure_count even before its first monitor baseline; the focused health suite is 59/59. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2360092ee
ℹ️ 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".
…temd # Conflicts: # agent-worktree.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@deployment/aliyun/monday-collector-health.sh`:
- Around line 306-318: Update the upload-status validation jq filter in the
health-check flow to require a present, non-null failure_count before validating
that it is a non-negative integer; remove the // 0 fallback from validation and
extraction so missing values trigger record_breach. Add a fixture omitting
failure_count and assert that it produces a breach.
In `@deployment/aliyun/test-binance-fee-release-contract.sh`:
- Around line 16-27: Strengthen the credential-isolation assertions in the test
by rejecting producer implementations that accept relative account-secret paths,
rather than only checking for the PathBuf field. Update the service environment
scan to detect credential-bearing entries case-insensitively, including
uppercase names such as BINANCE_API_KEY, while preserving the existing service
credential and path checks.
- Around line 32-35: Update the timer assertions in the loop over spot_timer and
usdm_timer to also require the exact setting AccuracySec=1s, alongside the
existing OnUnitActiveSec and Persistent checks.
🪄 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: db6cf07e-e48f-4dff-9675-d624f3825a8a
📒 Files selected for processing (16)
.github/workflows/acr-publish.yml.github/workflows/ci.ymlagent-worktree.ymldeployment/aliyun/binance-fee-snapshot-spot.servicedeployment/aliyun/binance-fee-snapshot-spot.timerdeployment/aliyun/binance-fee-snapshot-usdm.servicedeployment/aliyun/binance-fee-snapshot-usdm.timerdeployment/aliyun/binance-fee-upload.envdeployment/aliyun/binance-fee-upload.servicedeployment/aliyun/binance-fee-upload.timerdeployment/aliyun/binance-fee.confdeployment/aliyun/monday-collector-health.shdeployment/aliyun/test-binance-fee-release-contract.shdeployment/aliyun/test-monday-collector-health.shrust_hft/tools/collector/src/bin/binance-fee-snapshot.rsrust_hft/tools/collector/src/binance_fee_upload.rs
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3970ba38ce
ℹ️ 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".
|
Fixed at exact head 1e65e68: mandatory failure_count is now scoped to binance-fee-upload; existing uploaders preserve the prior optional-counter contract. Added a focused USD-M Reference status-without-counter regression test. Local evidence: collector health 64 passed / 0 failed; fee release contract passed; bash syntax and diff checks passed. Please review exact head 1e65e68. |
Materialize verified Binance Spot/USD-M market tapes into the V3 outer materialization carrying CexReplaySnapshotV2: PIT instrument rules, account-bound maker/taker fees, independent funding and OI timelines, aggregate-trade flow, and signed runtime lifecycle latency evidence. Rebased onto origin/main be9e746, which already contains the signed arrival-cost evidence (#787), its verification (#788), fee-account binding (#789), and fee snapshot timers (#793). Refs #794.
* feat(research): publish verified CEX ResearchSnapshot V2 Materialize verified Binance Spot/USD-M market tapes into the V3 outer materialization carrying CexReplaySnapshotV2: PIT instrument rules, account-bound maker/taker fees, independent funding and OI timelines, aggregate-trade flow, and signed runtime lifecycle latency evidence. Rebased onto origin/main be9e746, which already contains the signed arrival-cost evidence (#787), its verification (#788), fee-account binding (#789), and fee snapshot timers (#793). Refs #794. * fix(research): close lob-pit-materializer review threads - Delegate runtime latency verification to the hardened alpha_domain::runtime_latency_evidence verifier instead of a drifted collector-local copy: event IDs are deduplicated before filtering (conflicting duplicates fail closed), every fill must carry an authenticated instrument_market_{market} identity, and USD-M fails closed until a derivatives execution path exists. - Republish verified fee and reference evidence triplets into the materialization artifact directory under content-addressed names so the digests recorded in the snapshot resolve to immutable bytes. - Restructure the test fixture per market: the happy path is Spot (signed LiveSmall latency evidence exists only for Spot), a new test proves USD-M materialization fails closed, and the aggregate-trade assertions now match the (previous, current] bucket semantics. * fix(research): anchor latency cohort and holding-interval funding - Publish the complete digest-anchored runtime feedback log and the trusted-key document into the artifact directory under their content digests, so independent readers can reverify every signed lifecycle event and no resolvable-evidence gap remains for the key document. - Charge USD-M funding settlements over each row's actual (current, future] holding interval, matching how the evaluator applies funding_bps to the held position; a settlement that precedes the holding interval is no longer charged and multi-bucket horizons collect every settlement. --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
Change contract
Publish fail-closed systemd oneshots/timers for Binance Spot and USD-M BTCUSDT fee snapshots plus the verified OSS uploader. The producer now accepts the account bundle only through an absolute credential file, allowing systemd
LoadCredential=to keep API secrets out of ordinary environment files.Issue relationship
Closes #792
Out of scope
Credential provisioning or disclosure, live unit installation/enablement, PM Reference, LOB collectors, evaluator/materializer behavior, and all order creation.
Dependencies and merge order
PR #791 merged into
f9cd212e3d6c63c5978bbeb5a0a6eb6d7d6282ffand is integrated in this branch.Focused validation
deployment/aliyun/test-binance-fee-release-contract.shshellcheck deployment/aliyun/test-binance-fee-release-contract.shbinance-fee-snapshottests with--no-run; native macOS execution remains blocked by the pre-existinglibc::RUSAGE_THREADissue inpolymarket_upload.rs.systemd-analyze verifypassed for all six service/timer assets in Ubuntu 24.04.Rollout and rollback
Merge publishes disabled assets only. A separate runtime issue will pin exact release and rollback identities and requires OSS data/manifest/_SUCCESS readback before success.
Scope exception
None.
Summary by CodeRabbit