feat: add missing rpc methods and CORS headers #663
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughAdds runtime transaction counting and a Prometheus Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-aperture/src/server/http/dispatch.rs (2)
96-122: JSON-RPC error responses currently miss CORS headers
dispatchnow applies CORS only to the successfulOk(Response<…>)path inprocess. When a handler or parsing step returns anErr, theunwrap!macro callsResponseErrorPayload::encodeand returns that response without passing throughset_access_control_headers, so browser clients will see missing CORS headers on errors.Consider updating
unwrap!so error responses also get CORS:- macro_rules! unwrap { - ($result:expr, $id: expr) => { - match $result { - Ok(r) => r, - Err(error) => { - return Ok(ResponseErrorPayload::encode($id, error)); - } - } - }; - } + macro_rules! unwrap { + ($result:expr, $id: expr) => { + match $result { + Ok(r) => r, + Err(error) => { + let mut response = + ResponseErrorPayload::encode($id, error); + Self::set_access_control_headers(&mut response); + return Ok(response); + } + } + }; + }This keeps the central CORS behavior while ensuring clients can read error responses.
133-189: CORS application for successful RPC calls and new methods looks goodApplying
set_access_control_headersonly onOk(response)fromprocessis appropriate for the success path, and the new methodsGetRecentPerformanceSamplesandGetVoteAccountsare correctly wired to their handlers. Once the error-path CORS gap is addressed (see earlier comment), the routing + CORS story will be consistent.You might also consider simplifying
resulthandling by returning early from each arm with headers applied, but that’s purely stylistic.magicblock-aperture/src/requests/http/mocked.rs (1)
35-44: Doc comment forget_transaction_countis now outdatedThe comment still says “currently we don't keep track of transaction count”, but the implementation now calls
self.ledger.count_transactions()?and returns the actual count. Please update the comment to reflect the current behavior so it doesn’t mislead future readers.You may also want to mention that this is still a simplified/mock view if
count_transactionshas known limitations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
magicblock-aperture/src/requests/http/mocked.rs(5 hunks)magicblock-aperture/src/requests/mod.rs(3 hunks)magicblock-aperture/src/requests/payload.rs(1 hunks)magicblock-aperture/src/server/http/dispatch.rs(5 hunks)magicblock-aperture/tests/mocked.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-aperture/tests/mocked.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-aperture/src/requests/http/mocked.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-aperture/src/requests/http/mocked.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-aperture/src/requests/http/mocked.rs
🧬 Code graph analysis (2)
magicblock-aperture/tests/mocked.rs (1)
magicblock-aperture/tests/setup.rs (1)
latest_slot(226-228)
magicblock-aperture/src/requests/http/mocked.rs (2)
magicblock-aperture/src/requests/payload.rs (2)
encode_no_context(85-100)encode_no_context(138-148)magicblock-aperture/src/server/http/dispatch.rs (1)
new(65-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run_make_ci_lint
🔇 Additional comments (6)
magicblock-aperture/src/requests/mod.rs (2)
32-75: New HTTP method variants andas_strmappings look consistent
GetRecentPerformanceSamplesandGetVoteAccountsare correctly added toJsonRpcHttpMethod, and theiras_strvalues match the camelCase method names. UsingSelf::Variantin the matches improves clarity and is functionally equivalent.
93-138: HTTP/WebSocketas_strhelpers remain in sync with enum variantsThe refactored
as_strimplementations for bothJsonRpcHttpMethodandJsonRpcWsMethodcover all variants, including the newly added HTTP methods, and keep the wire method names aligned with the enums.magicblock-aperture/tests/mocked.rs (2)
161-167: Epoch info assertion correctly follows dynamic slot behaviorAsserting
absolute_slot == env.latest_slot()matches the newget_epoch_infoimplementation that derivesabsoluteSlotfrom the current block height. Keepingepoch == 0for this test setup is still valid.
179-184: Epoch schedule expectations now match mocked constantsChecking
slots_per_epoch == 432_000andwarmup == falseis consistent with the newSLOTS_IN_EPOCHconstant and the updatedget_epoch_schedulemock. This makes the tests better reflect realistic Solana settings.magicblock-aperture/src/requests/http/mocked.rs (2)
14-20: New constants/imports for epoch and vote/perf data are appropriateAdding
RpcPerfSample,RpcVoteAccountStatus, andSLOTS_IN_EPOCH = 432_000here matches the new mocked behaviors for epoch info, performance samples, and vote accounts. Keeping these concerns local to the mocked HTTP layer is reasonable.
186-194: Updatedget_epoch_schedulebehavior is consistent with constantsReturning
slotsPerEpoch: SLOTS_IN_EPOCHandwarmup: falselines up with the new constant and the tests. The rest of the schedule fields remain simple placeholders, which is acceptable for a mock.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-metrics/src/metrics/mod.rs (1)
236-243: RegisterTRANSACTION_COUNTso it’s exposed via Prometheus
TRANSACTION_COUNTis defined and used but never registered inregister(), so it won’t show up in the exported metrics even though it’s incremented and read.Add it alongside the other transaction-related counters:
register!(UNDELEGATION_REQUESTED_COUNT); register!(UNDELEGATION_COMPLETED_COUNT); register!(UNSTUCK_UNDELEGATION_COUNT); - register!(FAILED_TRANSACTIONS_COUNT); + register!(TRANSACTION_COUNT); + register!(FAILED_TRANSACTIONS_COUNT); register!(REMOTE_ACCOUNT_PROVIDER_A_COUNT); register!(TASK_INFO_FETCHER_A_COUNT); register!(TABLE_MANIA_A_COUNT); register!(TABLE_MANIA_CLOSED_A_COUNT);Also applies to: 339-347
magicblock-aperture/src/requests/http/mocked.rs (1)
37-38: Update the outdated comment.The comment states "currently we don't keep track of transaction count," but the implementation now uses
TRANSACTION_COUNT.get()(line 43), making this comment misleading. Additionally, there's a grammar error: "with new the new."Apply this diff to update the comment:
- /// Handles the `getTransactionCount` RPC request. - /// currently we don't keep track of transaction count, - /// but with new the new ledger implementation will + /// Handles the `getTransactionCount` RPC request. + /// Returns the current transaction count tracked by the TRANSACTION_COUNT metric.
♻️ Duplicate comments (2)
magicblock-aperture/src/requests/http/mocked.rs (2)
20-20: Consider extracting SLOTS_IN_EPOCH to a shared constants module.The
SLOTS_IN_EPOCHconstant is module-local but may be needed elsewhere in the codebase. Extracting it to a shared constants module would prevent duplication and ensure consistency across the application.
236-245: Consider adding a test for the vote accounts response structure.The
get_vote_accountsimplementation returns emptycurrentanddelinquentvectors, which is appropriate for a mocked handler. Adding a test to verify the response structure would guard against accidental API changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs(1 hunks)magicblock-aperture/src/requests/http/mocked.rs(5 hunks)magicblock-aperture/src/requests/http/mod.rs(1 hunks)magicblock-aperture/src/server/http/mod.rs(2 hunks)magicblock-api/src/magic_validator.rs(2 hunks)magicblock-metrics/src/metrics/mod.rs(1 hunks)magicblock-processor/src/executor/processing.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-metrics/src/metrics/mod.rsmagicblock-processor/src/executor/processing.rsmagicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-metrics/src/metrics/mod.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-api/src/magic_validator.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-api/src/magic_validator.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-api/src/magic_validator.rsmagicblock-aperture/src/requests/http/mocked.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-aperture/src/requests/http/mocked.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-aperture/src/requests/http/mocked.rs
🧬 Code graph analysis (4)
magicblock-metrics/src/metrics/mod.rs (1)
magicblock-aperture/src/state/subscriptions.rs (2)
new(342-355)new(410-413)
magicblock-aperture/src/requests/http/mod.rs (1)
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs (1)
get_recent_performance_samples(28-49)
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs (2)
magicblock-aperture/src/requests/mod.rs (1)
params(22-26)magicblock-aperture/src/state/blocks.rs (1)
block_height(95-97)
magicblock-aperture/src/requests/http/mocked.rs (2)
magicblock-aperture/src/requests/payload.rs (2)
encode_no_context(85-100)encode_no_context(138-148)magicblock-aperture/src/server/http/dispatch.rs (1)
new(65-78)
🪛 GitHub Actions: Run CI - Lint
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs
[error] 35-35: Clippy: redundant closure. Replace get_or_init(|| TreeIndex::default()) with TreeIndex::default.
[error] 62-62: Clippy: redundant closure. Replace get_or_init(|| TreeIndex::default()) with TreeIndex::default.
[error] 66-66: Clippy: casting to the same type is unnecessary (unnecessary_cast). Remove 'as u64' in 'count.saturating_sub(last_count) as u64'.
[error] 1-1: Build failed: could not compile due to previous clippy errors in the same file.
🔇 Additional comments (6)
magicblock-processor/src/executor/processing.rs (1)
12-14: Clarify whetherTRANSACTION_COUNTshould include load failures and replays
TRANSACTION_COUNT.inc()is called before you know ifprocesssucceeded and regardless ofis_replay, so the counter will include:
- Transactions that failed during load (Err branch), and
- Replayed transactions (
is_replay == true).Given the description “Total number of executed transactions” and its use in performance sampling, it’d be good to confirm that this broader definition is intentional. If you only want successfully processed, non‑replay traffic, you probably want to move the increment into the
Ok(processed)branch and optionally gate it on!is_replay.Also applies to: 54-72
magicblock-api/src/magic_validator.rs (1)
51-52: Check interaction ofTRANSACTION_COUNTseeding with ledger replayHere you seed
TRANSACTION_COUNTwithledger.count_transactions()?and later, on startup, you may replay the ledger viamaybe_process_ledger(). If ledger replay flows throughTransactionExecutor::execute(which also callsTRANSACTION_COUNT.inc()), historical transactions could be counted twice (once viainc_by, once during replay).If the goal is “total transactions ever seen” including replays, this is fine; otherwise you may want to:
- Either avoid seeding and rely purely on execution‑time increments, or
- Ensure replay paths don’t bump
TRANSACTION_COUNT.Also applies to: 275-293
magicblock-aperture/src/server/http/mod.rs (1)
67-87: HTTP server lifecycle + perf-samples collector wiring looks soundTaking
selfby value inrunand spawningdispatcher.run_perf_samples_collector(cancel.clone())is a clean way to attach the collector to the same cancellation flow as the listener. The updatedhandle(&self, ...)keeps per‑connection shutdown semantics unchanged and avoids unnecessary interior mutability.Also applies to: 93-127
magicblock-aperture/src/requests/http/mod.rs (1)
273-273: LGTM! Module declaration is consistent with existing patterns.The new
get_recent_performance_samplesmodule follows the naming conventions and is properly placed among other HTTP request handler modules.magicblock-aperture/src/requests/http/mocked.rs (2)
168-177: Dynamic epoch calculations are correct and well-implemented.The epoch-related fields are now properly derived from the current slot using
SLOTS_IN_EPOCH, andtransactionCountis sourced from the ledger. The modulo and division operations correctly computeepochandslotIndex.
191-192: Epoch schedule values are consistent with epoch info calculations.Using
SLOTS_IN_EPOCHforslotsPerEpochand settingwarmup: falsealigns with the simplified epoch model used throughout this mocked implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs
🧬 Code graph analysis (1)
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs (2)
magicblock-aperture/src/requests/mod.rs (1)
params(22-26)magicblock-aperture/src/state/blocks.rs (1)
block_height(95-97)
🔇 Additional comments (4)
magicblock-aperture/src/requests/http/get_recent_performance_samples.rs (4)
1-13: LGTM!Imports are well-organized and all dependencies are necessary for the functionality.
15-25: LGTM!Constants are well-documented with clear rationale. The 25% buffer in
ESTIMATED_SLOTS_PER_SAMPLEprovides safety margin for the pruning logic.
27-34: Past clippy issues have been resolved.The code correctly uses
TreeIndex::defaultwithout redundant closures (lines 47, 84). TheReverse<Slot>key ordering ensures newest samples are returned first, which aligns with RPC expectations. TheSamplestruct is appropriately simple.
37-62: LGTM!The handler correctly:
- Parses and validates the count parameter
- Caps requests at the maximum history size to prevent resource abuse
- Uses
Guardfor safe concurrent reads- Returns samples in newest-first order (Reverse key ordering)
- Sets
num_non_vote_transactionstoNone(not tracked separately, which is acceptable)
Summary by CodeRabbit
New Features
Improvements
Metrics
Tests
✏️ Tip: You can customize this high-level summary in your review settings.