feat: expose platform bootstrap metadata in masternode responses - #14
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds optional masternode platform address fields and an atomic cache snapshot with refresh metadata. The ChangesMasternode metadata and cache freshness
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The masternodes response adds optional bootstrap metadata and cache freshness while retaining existing response compatibility. No concrete current-head merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Client
participant get_masternodes
participant MasternodeCache
Client->>get_masternodes: request /masternodes
get_masternodes->>MasternodeCache: get_snapshot()
MasternodeCache-->>get_masternodes: cached list and last_updated
get_masternodes->>get_masternodes: apply host overrides
get_masternodes-->>Client: data and optional lastUpdated
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.97.1)Clippy execution failed 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 |
|
✅ Final review complete — no blockers (commit 63e1ec8) · triage: normal · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied Phase-2 evidence against head 63e1ec8 and confirmed one non-blocking test-coverage suggestion; no blocking defects were identified. The successful refresh publication path is not exercised by the tests, although the implementation publishes metadata and freshness together and preserves the snapshot after failed refreshes. Independent locked, offline build, all 8 tests, and Clippy passed with warnings; formatting validation failed.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
normalbygpt-6-astra(effort low) — The change is contained to API metadata serialization, endpoint host overrides, and atomic in-memory cache timestamps, with compatibility and refresh-failure risks but no changes to consensus, cryptography, persistent storage, or network connection behavior. - Phase 1 reviewers: not run (skipped for throughput: 42 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/masternode_cache.rs`:
- [SUGGESTION] src/masternode_cache.rs:275-283: Exercise successful snapshot publication through refresh()
Both populated-cache tests seed cache.data directly with a fabricated MasternodeSnapshot. The only test call to refresh() uses an invalid RPC URL and exits before successful publication, so the tests would still pass if update_cache_internal() stopped publishing snapshots or published an incorrect timestamp. Add a successful-refresh test using a local JSON-RPC fixture with POSE_BANNED entries to avoid outbound gRPC probes. Assert that refresh() replaces an older snapshot with the returned metadata and a timestamp bounded by the refresh call, then make a subsequent refresh fail and verify that the successfully published list and timestamp remain unchanged. This covers the production state transition behind the new lastUpdated contract rather than only serialization and reads of manually seeded data.
| async fn should_serve_fields_and_original_refresh_time_without_rpc() { | ||
| let mut config = Config::default(); | ||
| // Even an accidentally attempted RPC cannot reach the network. | ||
| config.rpc.url = "not a URL".into(); | ||
| let cache = Arc::new(MasternodeCache::new(config.clone())); | ||
| *cache.data.write().unwrap() = Some(snapshot()); | ||
| let first = response(config.clone(), cache.clone(), "/masternodes").await; | ||
| assert_eq!(first["success"], true); | ||
| assert_eq!(first["lastUpdated"], 1234567890); |
There was a problem hiding this comment.
🟡 Suggestion: Exercise successful snapshot publication through refresh()
Both populated-cache tests seed cache.data directly with a fabricated MasternodeSnapshot. The only test call to refresh() uses an invalid RPC URL and exits before successful publication, so the tests would still pass if update_cache_internal() stopped publishing snapshots or published an incorrect timestamp. Add a successful-refresh test using a local JSON-RPC fixture with POSE_BANNED entries to avoid outbound gRPC probes. Assert that refresh() replaces an older snapshot with the returned metadata and a timestamp bounded by the refresh call, then make a subsequent refresh fail and verify that the successfully published list and timestamp remain unchanged. This covers the production state transition behind the new lastUpdated contract rather than only serialization and reads of manually seeded data.
source: ['claude']
3 participants
The
/masternodesresponse drops the Platform node ID and P2P port already present in Core's deterministic masternode list. Consumers such as Dashmate's release tooling therefore cannot construct Tenderdash seeds from the quorum server.Preserve
platformNodeID,platformP2PPort, and separately registeredaddressesalongside the existing fields. AddlastUpdated(Unix seconds) to successful masternode responses, publishing it atomically with the cached list. Reads and failed refreshes retain the original timestamp. The existingdataarray, legacy responses, and cache-only request handling remain compatible; configured host overrides also apply to the additional endpoints.Validation:
cargo build --locked,cargo test --locked(8 passing), andcargo clippy --locked --all-targetspassed. Existing compiler/Clippy warnings remain. Tests cover DML field preservation, legacy entries, ban filtering, HTTP response compatibility, host overrides, and timestamp preservation after a failed refresh.Supports the quorum-server source planned for dashpay/platform#4539. This change has not been deployed.
This pull request was created by Codex.
Summary by CodeRabbit
New Features
/masternodesendpoint now includes an optionallastUpdatedtimestamp showing when cached data was last successfully refreshed.Documentation