chore: update toolchain and fix clippy #708
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. |
📝 WalkthroughWalkthroughThis PR updates the Rust toolchain to 1.91.1 and performs widespread API shape changes: many error enum variants now box inner errors and add explicit From impls (replacing #[from]); several function return types gained explicit lifetime annotations; a few arithmetic checks were replaced with is_multiple_of; unsafe NonZeroUsize::new_unchecked was replaced with safe NonZeroUsize::new(...).unwrap(); a test scaffold was removed; plus minor CI/workflow and doc-formatting tweaks. No new runtime features introduced. Possibly related PRs
Suggested reviewers
✨ 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 |
|
❌ Failed to trigger deploy for branch ``. Make sure you have permission or use the manual deploy link in the PR 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tools/ledger-stats/src/transaction_details.rs (1)
53-64: Remove unnecessary nested braces in else blocks.The refactored conditional blocks contain redundant inner braces that create unnecessary nesting and reduce readability. The double closing braces (
} }) on lines 62, 75, and 87 indicate an extra block expression that should be removed.Apply this diff to remove the unnecessary braces:
let pre_token_balances = status_meta.pre_token_balances.as_ref().map_or_else( || "None".to_string(), |b| { - if b.is_empty() { "None".to_string() } else { { + if b.is_empty() { + "None".to_string() + } else { b.iter() .map(|b| b.ui_token_amount.amount.to_string()) .collect::<Vec<_>>() .join(" | ") - } } + } }, ); let post_token_balances = status_meta.post_token_balances.as_ref().map_or_else( || "None".to_string(), |b| { - if b.is_empty() { "None".to_string() } else { { + if b.is_empty() { + "None".to_string() + } else { b.iter() .map(|b| b.ui_token_amount.amount.to_string()) .collect::<Vec<_>>() .join(" | ") - } } + } }, ); let rewards = status_meta.rewards.as_ref().map_or_else( || "None".to_string(), |r| { - if r.is_empty() { "None".to_string() } else { { + if r.is_empty() { + "None".to_string() + } else { r.iter() .map(|r| r.lamports.to_formatted_string(&Locale::en)) .collect::<Vec<_>>() .join(" | ") - } } + } }, );Also applies to: 66-77, 79-89
tools/ledger-stats/src/accounts.rs (1)
126-151: Owner filter semantics look good; consider avoiding variable shadowing for clarityThe new
is_none_orcondition correctly preserves the previous behavior (no filter whenownerisNone, filter by equality whenSome). To improve readability, avoid shadowing the outerownerby renaming the closure parameter:- if !owner.is_none_or(|owner| acc.owner.eq(&owner)) { + if !owner.is_none_or(|expected_owner| acc.owner.eq(&expected_owner)) {programs/guinea/src/lib.rs (1)
157-165: Change is correct; preferslice::from_refoverstd::slice::from_reffor consistencyThe switch to using
from_refis good and avoids the extraAccountInfoclone. Since the file already importscore::slice, it’s a bit cleaner (and moreno_std‑friendly if that ever matters) to use that import instead of hard‑coding thestdpath:- invoke(&ix, std::slice::from_ref(payer_info))?; + invoke(&ix, slice::from_ref(payer_info))?;magicblock-ledger/src/store/api.rs (1)
417-429: Doc comment tweaks are helpful; consider fixing a small typoThe added/reshaped docs for
get_confirmed_signatures_for_address,write_transaction, anditer_transaction_statusesimprove clarity and better explain parameter semantics and filtering; they align with the current implementations.Minor nit while you’re here: there’s a typo just above the new Geyser note:
- /// * `transaction` - Transaction to be written, we take a SanititizedTransaction here + /// * `transaction` - Transaction to be written, we take a SanitizedTransaction hereFixing this will keep the documentation consistent with the actual
SanitizedTransactiontype.Also applies to: 851-857, 1047-1050
magicblock-validator-admin/src/claim_fees.rs (1)
70-101: LGTM! Error boxing refactor is correct.The error handling updates properly wrap the underlying RPC errors in
Boxto reduce Result type size, which is a standard Rust pattern. The async flow is safe and the logic correctly handles fee claiming.Optional improvement:
Consider creating the
RpcClientonce and reusing it across invocations if the periodic task runs frequently, as establishing new connections on each tick may add unnecessary overhead:pub fn start(&mut self, tick_period: Duration, url: String) { // ... existing checks ... let token = self.token.clone(); let handle = tokio::spawn(async move { info!("Starting claim fees task"); let rpc_client = RpcClient::new_with_commitment(url, CommitmentConfig::confirmed()); let start_time = Instant::now() + tick_period; let mut interval = tokio::time::interval_at(start_time, tick_period); loop { tokio::select! { _ = interval.tick() => { if let Err(err) = claim_fees_with_client(&rpc_client).await { error!("Failed to claim fees: {:?}", err); } }, _ = token.cancelled() => break, } } info!("Claim fees task stopped"); }); self.handle = Some(handle); }magicblock-accounts/src/errors.rs (1)
60-71: BoxingCommittorServiceErrorinScheduledCommitsProcessorErroris sound; consider exposing inner error in DisplayStoring
CommittorServiceErrorasBox<...>and adding theFrom<CommittorServiceError>impl preserves?conversions while aligning with the boxed-error pattern used elsewhere.If you want easier debugging when this bubbles up, you could optionally change the message to include the inner error (e.g.,
"CommittorSerivceError: {0}") so logs show more than just the variant name.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (39)
.github/workflows/ci-lint.yml(1 hunks).github/workflows/ci-test-integration.yml(1 hunks).github/workflows/ci-test-unit.yml(1 hunks).github/workflows/deploy-testnet-by-pr-comment.yml(1 hunks)magicblock-accounts-db/src/index.rs(1 hunks)magicblock-accounts-db/src/lib.rs(2 hunks)magicblock-accounts/src/errors.rs(1 hunks)magicblock-api/src/errors.rs(3 hunks)magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/errors.rs(2 hunks)magicblock-committor-program/src/state/changeset.rs(1 hunks)magicblock-committor-program/src/state/chunks.rs(3 hunks)magicblock-committor-service/src/compute_budget.rs(1 hunks)magicblock-committor-service/src/error.rs(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-committor-service/src/persist/db.rs(4 hunks)magicblock-committor-service/src/service_ext.rs(1 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(4 hunks)magicblock-committor-service/src/transaction_preparator/error.rs(1 hunks)magicblock-config/src/lib.rs(2 hunks)magicblock-ledger/src/blockstore_processor/mod.rs(1 hunks)magicblock-ledger/src/database/db.rs(1 hunks)magicblock-ledger/src/database/rocks_db.rs(5 hunks)magicblock-ledger/src/store/api.rs(5 hunks)magicblock-magic-program-api/src/instruction.rs(2 hunks)magicblock-rpc-client/src/lib.rs(8 hunks)magicblock-rpc-client/src/utils.rs(4 hunks)magicblock-table-mania/src/lookup_table_rc.rs(1 hunks)magicblock-task-scheduler/src/errors.rs(2 hunks)magicblock-validator-admin/src/claim_fees.rs(2 hunks)programs/guinea/src/lib.rs(1 hunks)rust-toolchain.toml(1 hunks)test-integration/rust-toolchain.toml(1 hunks)test-kit/src/lib.rs(1 hunks)tools/ledger-stats/src/accounts.rs(1 hunks)tools/ledger-stats/src/transaction_details.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
magicblock-config/src/lib.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:
programs/guinea/src/lib.rsmagicblock-task-scheduler/src/errors.rsmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-rpc-client/src/lib.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:
programs/guinea/src/lib.rsmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-validator-admin/src/claim_fees.rsmagicblock-accounts-db/src/lib.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-rpc-client/src/lib.rs
📚 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-accounts/src/errors.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/service_ext.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.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-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-chainlink/src/remote_account_provider/errors.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-committor-program/src/state/changeset.rsmagicblock-accounts-db/src/lib.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-accounts-db/src/lib.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-ledger/src/blockstore_processor/mod.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-committor-service/src/intent_executor/task_info_fetcher.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-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
.github/workflows/ci-test-integration.yml
🧬 Code graph analysis (16)
magicblock-accounts/src/errors.rs (3)
magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(128-130)from(134-136)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)magicblock-committor-service/src/service_ext.rs (2)
from(232-234)new(44-58)
magicblock-committor-service/src/intent_executor/mod.rs (1)
test-integration/test-tools/src/conversions.rs (1)
err(8-8)
magicblock-task-scheduler/src/errors.rs (2)
magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(128-130)from(134-136)magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(93-95)
magicblock-committor-service/src/service_ext.rs (2)
magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(128-130)from(134-136)
magicblock-committor-program/src/state/changeset.rs (2)
magicblock-committor-service/src/stubs/changeset_committor_stub.rs (1)
len(39-41)magicblock-committor-program/src/state/chunks.rs (1)
chunk_size(130-132)
magicblock-committor-service/src/error.rs (1)
magicblock-rpc-client/src/lib.rs (1)
error(199-201)
magicblock-committor-service/src/transaction_preparator/error.rs (3)
magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(128-130)from(134-136)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
magicblock-committor-service/src/intent_executor/error.rs (5)
from(29-31)from(152-154)from(276-281)value(121-133)value(248-256)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
from(281-283)new(53-61)magicblock-committor-service/src/transaction_preparator/error.rs (2)
from(20-22)from(35-40)
magicblock-ledger/src/database/db.rs (1)
magicblock-ledger/src/database/rocks_db.rs (2)
raw_iterator_cf(209-211)batch(213-215)
magicblock-api/src/errors.rs (3)
magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-committor-service/src/service_ext.rs (2)
from(232-234)new(44-58)magicblock-task-scheduler/src/errors.rs (1)
from(50-54)
magicblock-ledger/src/database/rocks_db.rs (1)
magicblock-ledger/src/database/db.rs (1)
raw_iterator_cf(105-107)
magicblock-ledger/src/blockstore_processor/mod.rs (2)
magicblock-accounts-db/src/lib.rs (1)
slot(234-236)magicblock-ledger/src/database/columns.rs (8)
slot(131-131)slot(181-183)slot(249-251)slot(329-331)slot(404-406)slot(503-505)slot(577-579)slot(649-651)
magicblock-committor-service/src/intent_executor/error.rs (3)
magicblock-api/src/errors.rs (4)
from(104-106)from(110-112)from(116-118)from(122-124)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
from(281-283)new(53-61)magicblock-rpc-client/src/lib.rs (3)
from(80-82)from(234-236)new(241-243)
tools/ledger-stats/src/accounts.rs (1)
magicblock-committor-program/src/state/changeset.rs (1)
owner(119-124)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-rpc-client/src/lib.rs (3)
magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(128-130)from(134-136)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
from(513-515)from(548-552)from(574-576)
🔇 Additional comments (47)
magicblock-magic-program-api/src/instruction.rs (1)
29-29: Also applies to: 46-46magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
18-18: LGTM! Documentation formatting improvement.The reformatted doc comment improves alignment and readability with no functional impact.
magicblock-table-mania/src/lookup_table_rc.rs (1)
173-177: Display impl change to iterate viakeys()is correct and idiomaticUsing
pubkeys.read().expect(...).keys().map(|key| key.to_string())keeps the same behavior as iterating over(key, _)pairs, holds only a read lock while building the string, and matches Clippy’s recommendation to use.keys()when values are unused. Looks good as-is.magicblock-committor-service/src/persist/db.rs (3)
856-857: Switch tostd::slice::from_refis correct and more efficientPassing
std::slice::from_ref(&row)avoids an unnecessary clone and is fully compatible withinsert_commit_status_rows’ read-only use of the slice.
871-872: Samefrom_refchange here is soundThis mirrors the previous test: single borrowed row, no post-call mutation, so the borrow via
from_refis correct and avoids an extra allocation/clone.
904-905: Finalfrom_refusage in tests looks goodUsing
std::slice::from_ref(&row)here continues the same pattern and keeps the tests free of redundant cloning..github/workflows/deploy-testnet-by-pr-comment.yml (1)
151-162: No changes requiring comment.The modification is a minor whitespace/indentation adjustment with no functional impact on the workflow logic or behavior.
test-integration/rust-toolchain.toml (1)
1-2: Properly sets test-integration toolchain.Adding an explicit
rust-toolchain.tomlfor the test-integration directory ensures consistent Rust version (1.91.1) alignment with the root configuration and CI workflows..github/workflows/ci-test-integration.yml (2)
24-28: Rust toolchain updated to 1.91.1 in build job.The build step now correctly uses the updated toolchain version.
65-70: Both jobs already use the same toolchain version (1.84.1).There is no mismatch between the
buildandrun_integration_testsjobs; both userust_toolchain_release: "1.84.1". The suggested update to 1.91.1 is not supported by the current state of the file. The review comment was based on an incorrect assumption about the build job's toolchain version.Likely an incorrect or invalid review comment.
rust-toolchain.toml (1)
1-2: Root toolchain correctly updated to 1.91.1.The toolchain channel is now aligned with the CI workflow specifications and test-integration configuration.
.github/workflows/ci-lint.yml (1)
23-28: Lint CI workflow correctly updated to 1.91.1.The setup-build-env action now uses the updated Rust toolchain version, consistent with the root configuration and other CI workflows.
.github/workflows/ci-test-unit.yml (1)
23-28: Unit test CI workflow correctly updated to 1.91.1.The setup-build-env action now uses the updated Rust toolchain version, consistent with the root configuration and other CI workflows.
magicblock-committor-program/src/state/changeset.rs (1)
372-372: LGTM: Idiomatic divisibility check.The replacement of
len % chunk_size > 0with!len.is_multiple_of(chunk_size as usize)is functionally equivalent and more idiomatic. This standard library method improves code readability without changing behavior.magicblock-committor-program/src/state/chunks.rs (2)
100-100: LGTM: Consistent use of idiomatic divisibility checks.Both replacements of modulo-based offset validation with
is_multiple_ofimprove readability and align with modern Rust idioms. The logic remains functionally equivalent.Also applies to: 115-115
187-187: LGTM: Explicit lifetime annotation improves clarity.The explicit lifetime annotation
ChunksIter<'_>clarifies that the iterator borrows fromself. This is good practice and aligns with newer Rust edition requirements for explicitness in lifetime elision.magicblock-ledger/src/store/api.rs (1)
207-212: Explicit lifetime onRwLockReadGuardreturn types looks correctMaking the
RwLockReadGuardlifetime explicit incheck_lowest_cleanup_slotandensure_lowest_cleanup_slotmatches what newer Rust/clippy expect and correctly ties the guard’s lifetime to&self. Call sites continue to bind the guard (e.g.,let _lock = …/let (_lock, _) = …), so the lock is still held for the intended scope and behavior is unchanged.Please re-run the existing ledger tests on Rust 1.91.1 to confirm there are no additional borrow/lifetime warnings or regressions around these helpers.
Also applies to: 230-233
magicblock-validator-admin/src/claim_fees.rs (1)
84-84: No action needed. Error context is properly preserved after boxing.The
MagicBlockRpcClientErrorenum uses thethiserrorcrate with#[derive(Debug, Error)], and the error format strings for bothGetLatestBlockhashandSendTransactionvariants explicitly include both Display ({0}) and Debug ({0:?}) representations. Boxing the errors does not obscure debugging information—thethiserrormacro automatically implementsDisplayandDebugtraits that format the boxed error with full context.magicblock-ledger/src/blockstore_processor/mod.rs (1)
59-59: LGTM! Improved readability withis_multiple_of.The refactor from modulo arithmetic to
is_multiple_ofis semantically equivalent and more readable. This aligns with the PR's goal of adopting idiomatic Rust patterns.test-kit/src/lib.rs (1)
294-294: LGTM! Explicit lifetime annotation.The explicit lifetime parameter makes the borrow relationship clear and aligns with the broader lifetime annotation updates across the codebase (e.g.,
AccountsScanner<'_, F>in magicblock-accounts-db/src/lib.rs).magicblock-config/src/lib.rs (1)
70-70: LGTM! Consistent error boxing pattern.Boxing
figment::error::Erroraligns with the project-wide refactor to reduce error enum sizes and standardize error handling, as seen in magicblock-api/src/errors.rs and other modules.Also applies to: 94-94
magicblock-accounts-db/src/lib.rs (2)
189-189: LGTM! Explicit lifetime parameter.The lifetime annotation
AccountsScanner<'_, F>ties the scanner to theAccountsDbborrow, consistent with similar changes in magicblock-accounts-db/src/index.rs (AccountOffsetFinder<'_>).
249-249: LGTM! Clearer snapshot trigger logic.The refactor to
!slot.is_multiple_of(self.snapshot_frequency)maintains the same semantics (early return when not a multiple) while improving readability. This aligns with the similar change in magicblock-ledger/src/blockstore_processor/mod.rs.magicblock-chainlink/src/remote_account_provider/errors.rs (1)
10-10: LGTM! Consistent error boxing with explicitFromimplementation.Boxing
PubsubClientErrorand replacing#[from]with an explicitFromimpl aligns with the project-wide pattern seen in magicblock-api/src/errors.rs and magicblock-rpc-client/src/lib.rs.Also applies to: 90-96
magicblock-accounts-db/src/index.rs (1)
362-362: LGTM! Explicit lifetime annotation.The lifetime parameter
AccountOffsetFinder<'_>clarifies the borrow relationship and is consistent with similar changes likeAccountsScanner<'_, F>in magicblock-accounts-db/src/lib.rs.magicblock-rpc-client/src/lib.rs (1)
53-53: LGTM! Comprehensive error boxing refactor.Boxing RPC client errors across all enum variants and adding the
Fromimplementation provides consistent error handling. The changes at error propagation sites (lines 251, 258, 279, 317, 359, 406, 584) correctly wrap errors inBox::new().Also applies to: 56-56, 59-59, 65-65, 79-83
magicblock-rpc-client/src/utils.rs (1)
115-115: LGTM! Correct dereferencing and boxing for updated error types.The dereferencing at lines 115 and 121 (
*err) properly handles the boxedRpcClientErrorandSendTransactionvariants. The updated return type and error constructions intry_map_client_errorconsistently box errors to match the new signature.Also applies to: 121-121, 146-146, 155-159, 168-171
magicblock-committor-service/src/intent_executor/mod.rs (1)
687-693: Owned rpc error mapping from boxed InternalError looks correctPattern-matching
InternalError::MagicBlockRpcClientError(err)and passing*errintomap_magicblock_client_errorcorrectly moves theMagicBlockRpcClientErrorout of theBox, which matches the new boxed-InternalError design. The box is consumed and not reused afterwards, so there’s no double-use or borrow issue.magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
363-370: Test import cleanup is safeSwitching to only
HashSetin the test imports matches actual usage (the tests useHashSetbut notHashMap), so this just removes an unused import and quiets lints without changing behavior.magicblock-committor-service/src/error.rs (1)
42-50: Boxing init changeset RPC errors is consistent with the new error pattern
FailedToSendInitChangesetAccountandFailedToConfirmInitChangesetAccountnow storeBox<solana_rpc_client_api::client_error::Error>, which aligns with the repo’s move to boxed inner errors and keepsCommittorServiceError’s size in check. Since these variants never had#[from], callers already had to construct them explicitly; they now just need to wrap withBox::new(err)instead of passing the error by value.magicblock-task-scheduler/src/errors.rs (1)
11-15: Boxed PubsubClientError variant with explicitFromkeeps?behavior intactWrapping
PubsubClientErrorinBox<...>for thePubsubvariant and adding a manualFrom<PubsubClientError>preserves the ability to use?on pubsub calls while matching the project-wide boxed-error pattern. The#[error(transparent)]annotation continues to delegate formatting and source to the inner error via the boxed wrapper.Also applies to: 47-55
magicblock-committor-service/src/service_ext.rs (1)
221-235: CommittorServiceExtError now boxes CommittorServiceError correctlySwitching
CommittorServiceExtError::CommittorServiceErrorto hold aBox<CommittorServiceError>and adding the explicitFrom<CommittorServiceError>keeps theschedule_base_intents_waitingawait??flow working while matching the rest of the stack’s boxed-error conventions.magicblock-committor-service/src/intent_executor/error.rs (1)
20-32: BoxedMagicBlockRpcClientErrorinInternalErrorand updatedFromchain look correctStoring
MagicBlockRpcClientErrorasBox<...>inInternalErrorand providingFrom<MagicBlockRpcClientError>ensures all?-based RPC error paths (e.g., insend_prepared_message) still compile while keeping the enum size reasonable. TheFrom<MagicBlockRpcClientError> for TransactionStrategyExecutionErrornow routing throughInternalError::from(value)correctly produces theInternalErrorvariant rather than duplicating logic, andInternalError::signature()still works viaBox’s deref.Also applies to: 151-155
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
16-17: SaferNonZeroUsizeconst initializationUsing
NonZeroUsize::new(5).unwrap()andNonZeroUsize::new(1000).unwrap()in consts removes the need for unsafe initialization and is provably safe given the non-zero literals. This is compatible with the newer toolchain and keeps clippy happy.Also applies to: 54-55
271-284: TaskInfoFetcherError now boxes RPC client errors with properFrom
TaskInfoFetcherError::MagicBlockRpcClientError(Box<MagicBlockRpcClientError>)plus the newFrom<MagicBlockRpcClientError>keeps?-based propagation from RPC calls (e.g., infetch_metadata) working while aligning with the boxed-error pattern. The retry loop infetch_metadata_with_retriesstill behaves correctly by moving the boxed error intolast_err, andsignature()continues to delegate to the underlying rpc error viaBox’s deref.magicblock-ledger/src/database/db.rs (1)
104-117: LGTM! Correct lifetime annotations for RocksDB types.The explicit
'_lifetime annotations onDBRawIterator<'_>andWriteBatch<'_>correctly tie these returned types to the borrow of&self. This is semantically accurate sinceWriteBatchholds column family references obtained fromself.backend.magicblock-committor-service/src/transaction_preparator/error.rs (1)
15-23: LGTM! Boxing the error type follows the established codebase pattern.The change from
#[from]attribute to an explicitFromimplementation with boxing is consistent with the error-handling patterns across the codebase (e.g.,magicblock-api/src/errors.rs,magicblock-accounts/src/errors.rs). This reduces enum size while preserving ergonomic?operator usage.magicblock-ledger/src/database/rocks_db.rs (3)
83-90: LGTM! Correct lifetime annotation for pinned slice.The
DBPinnableSlice<'_>lifetime annotation correctly reflects that the pinned data is borrowed from the database and must not outlive the&selfreference.
102-117: LGTM! Lifetime annotations on batched multi-get results.The
Vec<LedgerResult<Option<DBPinnableSlice<'_>>>>return type correctly propagates the lifetime constraint from the underlying RocksDB API.
172-210: LGTM! Iterator lifetime annotations are correct.The
DBIterator<'_>andDBRawIterator<'_>return types correctly tie the iterators to the database borrow, ensuring safe iteration over database contents.magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
346-354: LGTM! Correct unboxing when mapping the client error.The
*errdereference at line 349 correctly extracts the ownedMagicBlockRpcClientErrorfrom theBoxbefore passing it tomap_magicblock_client_error.
508-516: LGTM! Boxing pattern for TransactionSendError is consistent.The explicit
Fromimplementation with boxing follows the established error-handling pattern in the codebase.
547-553: LGTM! BufferExecutionError conversion correctly boxes the error.The
From<MagicBlockRpcClientError>implementation properly boxes the error before wrapping inTransactionSendError.
565-577: LGTM! InternalError boxing follows the same pattern.Consistent with other error types in this file and across the codebase.
magicblock-api/src/errors.rs (3)
15-36: LGTM! Error variants correctly updated to use boxed types.Boxing these error types reduces the overall
ApiErrorenum size, which addresses clippy'svariant_size_differenceslint while maintaining the same public API behavior.
92-95: LGTM! TaskSchedulerServiceError variant updated consistently.
102-137: LGTM! Explicit From implementations follow established codebase patterns.The explicit
Fromimplementations with boxing are consistent with patterns inmagicblock-accounts/src/errors.rsand other modules. This approach replaces the#[from]derive macro to enable boxing while preserving ergonomic?operator usage.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/ledger-stats/src/transaction_details.rs (1)
53-68: LGTM! Consider removing unnecessary inner braces.The refactoring from
bool::thento explicitif-elseimproves clarity. However, lines 60-65 have redundant braces that can be removed.Apply this diff to simplify:
|b| { if b.is_empty() { "None".to_string() } else { - { - b.iter() - .map(|b| b.ui_token_amount.amount.to_string()) - .collect::<Vec<_>>() - .join(" | ") - } + b.iter() + .map(|b| b.ui_token_amount.amount.to_string()) + .collect::<Vec<_>>() + .join(" | ") } },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
magicblock-api/src/errors.rs(3 hunks)magicblock-chainlink/src/remote_account_provider/errors.rs(2 hunks)magicblock-committor-service/src/compute_budget.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-rpc-client/src/lib.rs(8 hunks)magicblock-table-mania/src/lookup_table_rc.rs(1 hunks)magicblock-task-scheduler/src/errors.rs(2 hunks)magicblock-validator-admin/src/claim_fees.rs(2 hunks)tools/ledger-stats/src/transaction_details.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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-validator-admin/src/claim_fees.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-rpc-client/src/lib.rsmagicblock-api/src/errors.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-chainlink/src/remote_account_provider/errors.rsmagicblock-task-scheduler/src/errors.rsmagicblock-rpc-client/src/lib.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-chainlink/src/remote_account_provider/errors.rs
📚 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-committor-service/src/intent_executor/task_info_fetcher.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-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-task-scheduler/src/errors.rs
🧬 Code graph analysis (4)
magicblock-chainlink/src/remote_account_provider/errors.rs (3)
magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(130-134)from(138-140)magicblock-task-scheduler/src/errors.rs (1)
from(52-56)magicblock-accounts/src/errors.rs (1)
from(68-70)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
magicblock-rpc-client/src/lib.rs (3)
new(243-245)from(82-84)from(236-238)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-rpc-client/src/lib.rs (3)
magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(130-134)from(138-140)magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(95-97)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-api/src/errors.rs (4)
magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(95-97)magicblock-task-scheduler/src/errors.rs (1)
from(52-56)magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
🔇 Additional comments (13)
magicblock-table-mania/src/lookup_table_rc.rs (1)
173-179: Approved: idiomatic HashMap key iteration refactoring.The change from iterating over key-value pairs to using
.keys()directly is more idiomatic and clearly expresses the intent to iterate only over keys. The functional output remains identical, and this is a solid clippy fix that improves code clarity.magicblock-committor-service/src/compute_budget.rs (1)
56-64: Idiomatic use ofsaturating_addpreserves previous overflow semanticsSwitching from a manual
checked_add(...).unwrap_or(u32::MAX)pattern to.saturating_add(self.base_budget)keeps the same clamping behavior while simplifying the code and avoiding explicitunwrap_or. The overflow path still cleanly saturates atu32::MAXafter theu32::try_fromstep.magicblock-chainlink/src/remote_account_provider/errors.rs (2)
10-12: LGTM: Error boxing reduces enum size.Boxing the
PubsubClientErrorvariant helps reduce the overall size of theRemoteAccountProviderErrorenum, which can improve performance by reducing stack allocation and copy costs.
92-98: LGTM: Explicit From implementation maintains ergonomic error conversion.The explicit
Fromimplementation preserves the convenience of the?operator for error propagation while boxing the error.magicblock-rpc-client/src/lib.rs (3)
53-65: LGTM: Consistent error boxing across all RPC error variants.All RPC-related error variants now consistently use
Box<...>to reduce the enum size. This is a standard optimization for error types containing large variants.
79-85: LGTM: From implementation enables ergonomic error conversion.The
Fromimplementation allows seamless conversion fromsolana_rpc_client_api::client_error::Errorusing the?operator while applying the boxing strategy.
250-252: LGTM: All error propagation sites correctly updated.All error construction and propagation sites have been consistently updated to box errors, maintaining correctness while applying the new error boxing pattern.
Also applies to: 259-260, 280-280, 318-318, 360-360, 407-409, 587-587
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
16-16: LGTM: Safe NonZeroUsize construction replaces unsafe code.Replacing
NonZeroUsize::new_uncheckedwithNonZeroUsize::new(...).unwrap()eliminates unsafe code while maintaining the same semantics for these compile-time constant values. This is a safer approach that satisfies Clippy lints in the new toolchain.Also applies to: 53-53
275-282: LGTM: Error boxing maintains consistency with codebase patterns.Boxing
MagicBlockRpcClientErrorwith an explicitFromimplementation follows the same pattern applied across the codebase and helps reduce enum size.magicblock-task-scheduler/src/errors.rs (1)
12-16: LGTM: Error boxing with explicit From implementation.The boxing of
PubsubClientErrorwith its accompanyingFromimplementation follows the consistent pattern applied throughout this PR, reducing enum size while maintaining ergonomic error conversion.Also applies to: 49-57
magicblock-api/src/errors.rs (2)
16-16: LGTM: Comprehensive error boxing across API error variants.All major inner error types are now consistently boxed, reducing the size of the
ApiErrorenum. This is particularly beneficial for this top-level error type that aggregates errors from multiple subsystems.Also applies to: 19-19, 22-22, 25-25, 34-36, 93-95
103-141: LGTM: Complete From implementations for all boxed error types.All six
Fromimplementations correctly box their respective error types, maintaining ergonomic error propagation with the?operator while applying the boxing optimization.magicblock-validator-admin/src/claim_fees.rs (1)
96-96: LGTM! Consistent error boxing pattern.The error boxing for
SendTransactionis consistent with theGetLatestBlockhasherror handling above (lines 82-83) and follows the same refactoring pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
magicblock-committor-program/src/lib.rs(1 hunks)magicblock-committor-program/src/state/changeset.rs(1 hunks)magicblock-committor-program/src/state/chunks.rs(1 hunks)test-integration/programs/flexi-counter/src/processor.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(2 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(1 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(1 hunks)test-integration/test-runner/src/env_config.rs(1 hunks)test-integration/test-tools/src/integration_test_context.rs(10 hunks)test-integration/test-tools/src/transactions.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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-committor-program/src/state/changeset.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-committor-program/src/state/changeset.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/schedulecommit/src/utils/mod.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
magicblock-committor-program/src/lib.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-tools/src/integration_test_context.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:
test-integration/programs/flexi-counter/src/processor.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:
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs
🧬 Code graph analysis (2)
magicblock-committor-program/src/state/changeset.rs (1)
magicblock-committor-program/src/state/chunks.rs (1)
chunk_size(130-132)
magicblock-committor-program/src/state/chunks.rs (1)
magicblock-ledger/src/database/db.rs (1)
iter(67-80)
🔇 Additional comments (10)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
286-286: LGTM! Explicit lifetime improves clarity.The addition of the explicit lifetime annotation
<'_>aligns with modern Rust best practices and clippy recommendations. The placeholder'_correctly allows Rust to infer the lifetime tied to&self, making the code more explicit without changing behavior.magicblock-committor-program/src/state/changeset.rs (1)
371-376: Semantically equivalent remainder check; no behavior changeChanging the condition from
len % chunk_size as usize > 0tolen % chunk_size as usize != 0is semantically equivalent forusize(remainder is always ≥ 0), sochunk_countcomputation is unchanged while satisfying clippy. No further action needed here.magicblock-committor-program/src/state/chunks.rs (1)
187-187: LGTM! Explicit lifetime annotation improves clarity.The addition of
'_makes the lifetime dependency explicit—the returned iterator borrows fromself. This change aligns with newer Rust toolchain requirements and best practices while maintaining the same behavior.test-integration/test-runner/src/env_config.rs (2)
52-57: LGTM! More idiomatic Option API.The refactoring from
map_or(true, ...)tois_none_or(...)maintains identical behavior while using a more idiomatic and readable API introduced in newer Rust versions.
59-64: LGTM! Consistent refactoring.The same idiomatic refactoring applied here maintains behavioral equivalence and improves code clarity.
test-integration/test-tools/src/transactions.rs (1)
19-19: Lint suppressions appropriate for test utilities.The
#[allow(clippy::result_large_err)]suppressions are pragmatic for test helper functions. Sinceclient_error::Erroris an external type from the Solana SDK and these are test utilities, the performance impact is negligible.Note: If similar patterns appear in production code paths, consider boxing the error (
Result<T, Box<client_error::Error>>) to reduce the Result size.Also applies to: 39-39, 52-52, 71-71, 83-83
test-integration/test-tools/src/integration_test_context.rs (1)
670-670: Lint suppressions selectively and correctly applied.The
#[allow(clippy::result_large_err)]attributes are added only to methods returningResult<..., client_error::Error>, while methods returningResult<..., anyhow::Error>(lines 771-919) correctly omit the suppression sinceanyhow::Erroris already heap-allocated. This selective application demonstrates understanding of when the warning applies.For integration test context utilities, this suppression approach is appropriate.
Also applies to: 687-687, 704-704, 721-721, 738-738, 755-755, 857-857, 879-879, 891-891, 903-903
test-integration/programs/schedulecommit/src/utils/mod.rs (1)
89-89: LGTM! Good optimization to avoid unnecessary cloning.Using
std::slice::from_ref(account_info)instead of cloning is more efficient and idiomatic. Sinceinvoke_signedonly needs a reference to a single-element slice, there's no need to clone theAccountInfo. This change eliminates unnecessary allocation while maintaining identical functionality.Also applies to: 98-98
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
711-735: Use ofis_multiple_ofkeeps behavior while matching modern Rust idiomsSwitching from
remaining_tries % 4 == 0toremaining_tries.is_multiple_of(4)is equivalent here foru8with a constant divisor of 4, including theremaining_tries == 0case, and makes the intent clearer/aligns with newer std APIs. No functional or test semantics concerns from this change.Please confirm your toolchain (Rust 1.91.1 per PR description) indeed has
u8::is_multiple_ofstabilized and thatcargo test -p test-committor-service --test test_ix_commit_localpasses end‑to‑end with this change.test-integration/programs/flexi-counter/src/processor.rs (1)
492-492: LGTM! Idiomatic slice construction.The change to
std::slice::from_ref(payer_info)is the idiomatic way to create a single-element slice from a reference without copying, and is functionally equivalent to the previous&[payer_info]pattern. This improvement aligns with Rust best practices and was likely recommended by clippy as part of the toolchain update.
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs
Outdated
Show resolved
Hide resolved
Dodecahedr0x
left a comment
Dodecahedr0x
left a comment
There was a problem hiding this comment.
Few questions, but LGTM
| @@ -1,3 +1,5 @@ | |||
| #![allow(clippy::manual_is_multiple_of)] | |||
There was a problem hiding this comment.
Is this needed because build-sbf does not have the feature enabled? Would be nice to avoid it
There was a problem hiding this comment.
yes, unfortunately solana uses older toolchain to build, but clippy runs with newer one, we have to silence it until solana upgrades the toolchain to 1.87 or higher.
| (player, player_pda) | ||
| } | ||
|
|
||
| #[allow(clippy::result_large_err)] |
There was a problem hiding this comment.
Why stopping the error boxing now?
There was a problem hiding this comment.
I suppose because these are tests, so perf optimizations don't matter as much.
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
After addressing the nits I pointed out LGTM.
| )); | ||
| } | ||
|
|
||
| #[allow(clippy::result_large_err)] |
There was a problem hiding this comment.
Ideally #![allow(clippy::result_large_err)] at the test module level so you don't have to repeat this everywhere.
|
|
||
| use crate::conversions::stringify_simulation_result; | ||
|
|
||
| #[allow(clippy::result_large_err)] |
There was a problem hiding this comment.
ditto on module level pragma may be better
bmuddha
force-pushed
the
bmuddha/chore/toolchain-bump
branch
from
f43835c to
18c9fcc
Compare
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 (3)
magicblock-committor-service/src/persist/db.rs (1)
926-926: Consider applying the same borrow pattern for consistency.Three other tests still use
&[row.clone()]for single-row inserts. For consistency with the refactored tests, consider updating these to usestd::slice::from_ref(&row)as well:
- Line 926 in
test_get_signatures_by_commit- Line 959 in
test_empty_data_handling- Line 972 in
test_undelegate_flagApply this diff to update
test_get_signatures_by_commit:- db.insert_commit_status_rows(&[row.clone()]).unwrap(); + db.insert_commit_status_rows(std::slice::from_ref(&row)).unwrap();Similar changes can be applied to the other two tests.
Also applies to: 959-959, 972-972
magicblock-accounts/src/errors.rs (1)
59-71: Preserve innerCommittorServiceErrordetails in DisplayBoxing
CommittorSerivceErroris fine, but the current variant no longer prints the inner error, which makes logs and user-facing messages less informative compared to the other variants in this file.You can keep boxing while still exposing the underlying error by including
{0}in the error message:- #[error("CommittorSerivceError")] - CommittorSerivceError(Box<CommittorServiceError>), + #[error("CommittorSerivceError: {0}")] + CommittorSerivceError(Box<CommittorServiceError>),magicblock-ledger/src/database/db.rs (1)
109-147: Align allWriteBatchusages with the lifetime parameter
WriteBatchis defined asWriteBatch<'a>, andbatch(&self) -> WriteBatch<'_>correctly ties the batch's lifetime to&self. However, the following methods acceptWriteBatchwithout the lifetime parameter:
- Line 119:
pub fn write(&self, batch: WriteBatch)- Line 132:
pub fn delete_range_cf<C>(&self, batch: &mut WriteBatch, ...)Update these signatures to:
- pub fn write(&self, batch: WriteBatch) -> Result<(), LedgerError> { + pub fn write(&self, batch: WriteBatch<'_>) -> Result<(), LedgerError> { self.backend.write(batch.write_batch) } @@ - pub fn delete_range_cf<C>( - &self, - batch: &mut WriteBatch, + pub fn delete_range_cf<C>( + &self, + batch: &mut WriteBatch<'_>, from: Slot, to: Slot, ) where
♻️ Duplicate comments (3)
test-integration/test-tools/src/transactions.rs (1)
1-2: Crate-level lint suppression added.The crate-level
#![allow(clippy::result_large_err)]suppresses the lint for the entire file. As noted in previous reviews, a module-level pragma might offer better granularity if specific modules within the crate need different lint configurations in the future.test-integration/test-tools/src/integration_test_context.rs (1)
672-672: Remove redundant function-level lint suppressions.Since the crate-level
#![allow(clippy::result_large_err)]at Line 1 already suppresses this lint for the entire file, the function-level#[allow(clippy::result_large_err)]attributes onconfirm_transaction_chain,confirm_transaction_ephem, andsend_transaction_ephemare redundant and should be removed.Apply this diff to remove the redundant attributes:
- #[allow(clippy::result_large_err)] pub fn confirm_transaction_chain( &self, sig: &Signature, tx: Option<&Transaction>, ) -> Result<bool, client_error::Error> { - #[allow(clippy::result_large_err)] pub fn confirm_transaction_ephem( &self, sig: &Signature, tx: Option<&Transaction>, ) -> Result<bool, client_error::Error> { - #[allow(clippy::result_large_err)] pub fn send_transaction_ephem( &self, tx: &mut Transaction, signers: &[&Keypair], ) -> Result<Signature, client_error::Error> {Also applies to: 689-689, 706-706
magicblock-committor-service/src/compute_budget.rs (1)
55-64: Saturating add change is correct; optional consistency refactor inComputeBudget::total_budget.The new
.saturating_add(self.base_budget)on Line 63 preserves the previous “cap atu32::MAX” behavior while being more idiomatic thanchecked_add(..).unwrap_or(u32::MAX), and it composes correctly with the earliertry_from(..).unwrap_or(u32::MAX)logic. No correctness issues here.As a small, optional follow‑up (and echoing the earlier bot comment), you could make
ComputeBudget::total_budget(Lines 202‑207) use saturating arithmetic for consistency, e.g.:- fn total_budget(&self, committee_count: u32) -> u32 { - self.per_committee() - .checked_mul(committee_count) - .and_then(|product| product.checked_add(self.base_budget())) - .unwrap_or(u32::MAX) - } + fn total_budget(&self, committee_count: u32) -> u32 { + self.per_committee() + .saturating_mul(committee_count) + .saturating_add(self.base_budget()) + }This is purely stylistic and not required for the toolchain/clippy bump.
Also applies to: 202-207
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.github/workflows/ci-lint.yml(1 hunks).github/workflows/ci-test-integration.yml(1 hunks).github/workflows/ci-test-unit.yml(1 hunks).github/workflows/deploy-testnet-by-pr-comment.yml(1 hunks)magicblock-accounts-db/src/index.rs(1 hunks)magicblock-accounts-db/src/lib.rs(2 hunks)magicblock-accounts/src/errors.rs(1 hunks)magicblock-api/src/errors.rs(3 hunks)magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/errors.rs(2 hunks)magicblock-committor-program/src/lib.rs(1 hunks)magicblock-committor-program/src/state/changeset.rs(1 hunks)magicblock-committor-program/src/state/chunks.rs(1 hunks)magicblock-committor-service/src/compute_budget.rs(1 hunks)magicblock-committor-service/src/error.rs(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-committor-service/src/persist/db.rs(4 hunks)magicblock-committor-service/src/service_ext.rs(1 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(4 hunks)magicblock-committor-service/src/transaction_preparator/error.rs(1 hunks)magicblock-config/src/lib.rs(2 hunks)magicblock-ledger/src/blockstore_processor/mod.rs(1 hunks)magicblock-ledger/src/database/db.rs(1 hunks)magicblock-ledger/src/database/rocks_db.rs(5 hunks)magicblock-ledger/src/store/api.rs(5 hunks)magicblock-magic-program-api/src/instruction.rs(2 hunks)magicblock-rpc-client/src/lib.rs(8 hunks)magicblock-rpc-client/src/utils.rs(4 hunks)magicblock-table-mania/src/lookup_table_rc.rs(1 hunks)magicblock-task-scheduler/src/errors.rs(2 hunks)magicblock-validator-admin/src/claim_fees.rs(2 hunks)programs/guinea/src/lib.rs(1 hunks)rust-toolchain.toml(1 hunks)test-integration/programs/flexi-counter/src/processor.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(2 hunks)test-integration/rust-toolchain.toml(1 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(1 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(1 hunks)test-integration/test-runner/src/env_config.rs(1 hunks)test-integration/test-tools/src/integration_test_context.rs(4 hunks)test-integration/test-tools/src/transactions.rs(1 hunks)test-kit/src/lib.rs(1 hunks)tools/ledger-stats/src/accounts.rs(1 hunks)tools/ledger-stats/src/transaction_details.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-12-03T09:36:01.504Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.504Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-validator-admin/src/claim_fees.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/error.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/remote_account_provider/errors.rstools/ledger-stats/src/accounts.rstools/ledger-stats/src/transaction_details.rsmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-rpc-client/src/lib.rsmagicblock-committor-program/src/state/changeset.rsmagicblock-accounts-db/src/lib.rsmagicblock-accounts-db/src/index.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-config/src/lib.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-api/src/errors.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.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-validator-admin/src/claim_fees.rsmagicblock-committor-service/src/error.rsmagicblock-magic-program-api/src/instruction.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-rpc-client/src/lib.rsmagicblock-committor-program/src/state/changeset.rsmagicblock-accounts-db/src/lib.rs
📚 Learning: 2025-12-03T09:33:48.684Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.684Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-validator-admin/src/claim_fees.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/error.rsmagicblock-committor-program/src/state/changeset.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/persist/db.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-tools/src/integration_test_context.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rstest-kit/src/lib.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:
test-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-program/src/state/changeset.rsmagicblock-accounts-db/src/lib.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-task-scheduler/src/errors.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.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-task-scheduler/src/errors.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-rpc-client/src/lib.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:
test-integration/test-tools/src/transactions.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-kit/src/lib.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-ledger/src/blockstore_processor/mod.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-rpc-client/src/lib.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
magicblock-rpc-client/src/lib.rs.github/workflows/ci-test-integration.yml
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-accounts-db/src/lib.rs
📚 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-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
magicblock-config/src/lib.rs
🧬 Code graph analysis (15)
magicblock-committor-service/src/error.rs (1)
magicblock-rpc-client/src/lib.rs (1)
error(201-203)
magicblock-task-scheduler/src/errors.rs (2)
magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(95-97)magicblock-rpc-client/src/lib.rs (3)
from(82-84)from(236-238)new(243-245)
magicblock-committor-program/src/state/chunks.rs (1)
magicblock-ledger/src/database/db.rs (1)
iter(67-80)
magicblock-chainlink/src/remote_account_provider/errors.rs (3)
magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(130-134)from(138-140)magicblock-task-scheduler/src/errors.rs (1)
from(52-56)magicblock-accounts/src/errors.rs (1)
from(68-70)
tools/ledger-stats/src/accounts.rs (1)
magicblock-committor-program/src/state/changeset.rs (1)
owner(119-124)
magicblock-committor-service/src/service_ext.rs (2)
magicblock-api/src/errors.rs (4)
from(104-106)from(110-112)from(116-118)from(122-124)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-ledger/src/blockstore_processor/mod.rs (1)
magicblock-ledger/src/database/columns.rs (8)
slot(131-131)slot(181-183)slot(249-251)slot(329-331)slot(404-406)slot(503-505)slot(577-579)slot(649-651)
magicblock-committor-program/src/state/changeset.rs (2)
magicblock-committor-service/src/stubs/changeset_committor_stub.rs (1)
len(39-41)magicblock-committor-program/src/state/chunks.rs (1)
chunk_size(130-132)
magicblock-ledger/src/database/db.rs (1)
magicblock-ledger/src/database/rocks_db.rs (2)
raw_iterator_cf(209-211)batch(213-215)
magicblock-ledger/src/database/rocks_db.rs (1)
magicblock-ledger/src/database/db.rs (1)
raw_iterator_cf(105-107)
magicblock-committor-service/src/transaction_preparator/error.rs (3)
magicblock-api/src/errors.rs (3)
from(104-106)from(110-112)from(116-118)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
from(513-515)from(548-552)from(574-576)new(49-59)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
magicblock-api/src/errors.rs (5)
from(104-106)from(110-112)from(116-118)from(122-124)from(130-134)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-api/src/errors.rs (6)
magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(95-97)magicblock-task-scheduler/src/errors.rs (1)
from(52-56)magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (3)
new(51-64)new(105-122)new(698-704)magicblock-committor-service/src/intent_executor/mod.rs (1)
new(124-138)
magicblock-committor-service/src/intent_executor/error.rs (3)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
from(279-281)new(52-59)magicblock-rpc-client/src/lib.rs (3)
from(82-84)from(236-238)new(243-245)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
from(513-515)from(548-552)from(574-576)new(49-59)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
from(279-281)new(52-59)magicblock-rpc-client/src/lib.rs (4)
from(82-84)from(236-238)new(243-245)error(201-203)magicblock-committor-service/src/intent_executor/error.rs (5)
from(29-31)from(152-154)from(276-281)value(121-133)value(248-256)
🔇 Additional comments (56)
.github/workflows/deploy-testnet-by-pr-comment.yml (1)
151-162: Non-functional formatting change approved.The whitespace/newline adjustment to the "Add failure comment" step does not affect workflow logic, GitHub API calls, or authorization checks. The YAML structure and script behavior remain unchanged.
magicblock-magic-program-api/src/instruction.rs (1)
28-30: Documentation formatting improvements approved.The wrapped account reference documentation in both
ScheduleCommit(line 28-30) andScheduleCommitAndUndelegate(line 45-47) improves readability without changing semantics or public API. These are appropriate companion changes during the toolchain update.Also applies to: 45-47
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
17-18: Doc comment formatting aligned per clippy recommendations.The continuation line of the
remote_slot_in_bankparameter documentation is now properly aligned with the documentation style.rust-toolchain.toml (1)
2-2: Toolchain version update is correct.The channel update to 1.91.1 aligns with the PR objective.
.github/workflows/ci-test-unit.yml (1)
26-26: CI unit test toolchain is correctly updated.The rust_toolchain_release matches the root rust-toolchain.toml update to 1.91.1.
.github/workflows/ci-lint.yml (1)
26-26: CI lint toolchain is correctly updated.The rust_toolchain_release matches the root rust-toolchain.toml update to 1.91.1.
test-integration/rust-toolchain.toml (1)
1-2: Test-integration toolchain is correctly initialized.The new file ensures the test-integration environment uses the updated 1.91.1 toolchain, maintaining consistency across the repository.
test-kit/src/lib.rs (1)
294-296: Explicit lifetime onget_payeris correct and consistentThe updated signature
-> CommitableAccount<'_>is consistent withget_accountand theCommitableAccount<'db>definition, and keeps the borrow correctly tied to&self. No behavioral changes or soundness issues introduced here.magicblock-committor-program/src/state/chunks.rs (1)
187-187: LGTM! Explicit lifetime annotation improves clarity.The explicit
'_lifetime annotation in the return type correctly matches the lifetime parameter ofChunksIter<'a>and aligns with clippy recommendations in the updated toolchain. This test-only change is functionally equivalent to the previous elided form and improves code explicitness.magicblock-table-mania/src/lookup_table_rc.rs (1)
176-177: LGTM! More idiomatic iteration over HashMap keys.Using
.keys()is clearer and more idiomatic than.iter().map(|(key, _)| ...)when only keys are needed. This change maintains identical behavior while likely addressing a clippy warning.magicblock-committor-program/src/state/changeset.rs (1)
371-377: Remainder predicate change is behavior‑preserving and clippy‑friendly
len % chunk_size as usize > 0→!= 0is equivalent for this unsigned remainder case, andchunk_size == 0is already short‑circuited above, so there’s no risk of division by zero. Chunk counting semantics remain unchanged; this is a safe, lint‑driven cleanup.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
711-735: Semantics of retry logging preserved withis_multiple_ofSwitching from
remaining_tries % 4 == 0toremaining_tries.is_multiple_of(4)keeps the behavior (log every 4th remaining try when validation is failing) and reads cleaner. The only requirement is thatis_multiple_ofis available foru8in your Rust 1.91.1 toolchain (either as a std method or via an in-scope trait); if the build passes, this change is fully compatible.Please confirm that
is_multiple_ofresolves correctly onu8under the updated toolchain and that clippy is satisfied with this style in this file as well.test-integration/test-tools/src/integration_test_context.rs (1)
1-2: Crate-level lint suppression added.The crate-level
#![allow(clippy::result_large_err)]at the top of the file suppresses theresult_large_errlint for the entire crate, making function-level suppressions elsewhere in this file unnecessary.magicblock-committor-service/src/persist/db.rs (1)
856-857: LGTM! Idiomatic borrow pattern avoids unnecessary clones.The use of
std::slice::from_ref(&row)instead of&[row.clone()]is an appropriate optimization for these test cases. This pattern creates a single-element slice by borrowing the existing row rather than cloning it, which is both more efficient and more idiomatic for test setup.Also applies to: 871-872, 886-887, 904-905
test-integration/programs/schedulecommit/src/utils/mod.rs (1)
83-100: Switch tostd::slice::from_refis correct and avoids unnecessary cloningUsing
std::slice::from_ref(account_info)for theinvoke_signedaccount slice is semantically equivalent to the previous&[account_info.clone()], avoids an extraAccountInfoclone, and keeps lifetimes straightforward. No correctness or lifetime issues here, and this aligns well with clippy’s preferences for single‑element slices.test-integration/test-runner/src/env_config.rs (2)
52-57: Good refactoring to useis_none_or.The change from
map_or(true, |setup| setup.devnet())tois_none_or(|setup| setup.devnet())is functionally equivalent and more idiomatic for boolean predicates.
59-64: Good refactoring to useis_none_or.The change is functionally equivalent and more idiomatic.
Option::is_none_orhas been available since Rust 1.82.0, so it's safe to use with any recent Rust toolchain.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
1-1: Crate-levelresult_large_errallow is acceptable for this test crateUsing
#![allow(clippy::result_large_err)]at the crate level is consistent with treating these integration tests as non‑perf‑critical and avoids churn boxing test-only error types. The only trade‑off is that future large errorResults in this file will also be silently allowed, so if you ever add non‑test helpers here, it’s worth revisiting and tightening the scope of the lint suppression.test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
286-300: Explicit lifetime onfieldsreturn type is correct and clarifies borrowingChanging
fields(&self)to returnScheduleCommitTestContextFields<'_>aligns the method with the lifetime parameter onScheduleCommitTestContextFieldsand correctly ties all returned references to&selfwithout altering semantics. This looks sound and should satisfy the newer toolchain/clippy requirements.test-integration/programs/flexi-counter/src/processor.rs (1)
492-492: CPI account slice construction looks good and avoids unnecessary clone/allocationUsing
invoke(&ix, std::slice::from_ref(payer_info))is semantically equivalent to the previous single‑element account slice ofpayer_info.clone(), while avoiding both a clone and an intermediate allocation. Lifetimes line up with theaccountsslice, so this should be a no‑op behaviorally and satisfies stricter clippy/toolchain lints.Please confirm the existing integration tests that exercise
process_cancel_task(especially CancelTask CPI paths) still pass under the updated Rust toolchain to validate no behavioral drift.programs/guinea/src/lib.rs (1)
163-163: Single‑account CPI viafrom_refis correct and consistentSwitching
invoketo usestd::slice::from_ref(payer_info)correctly produces the required single‑element&[AccountInfo]without cloning or heap allocation. Given the instruction metas only reference the payer, this remains behaviorally identical to the prior implementation.Please rerun the guinea program tests or scenarios that call
GuineaInstruction::CancelTaskto confirm the CPI still behaves as expected with the new toolchain.magicblock-config/src/lib.rs (1)
68-95: Boxed figment error return type is type-safe and causes no actual breakageThe change to
pub fn try_new(...) -> Result<Self, Box<figment::error::Error>>withfigment.extract().map_err(Box::new)is type-correct and preserves all behavior. While the return type signature has changed, all three call sites in the codebase (main.rs, tests.rs, and README.md) use generic error handling (Display trait viaeprintln!,.expect(), or?operator) which Box transparently forwards, so no caller updates are needed. The config layering and precedence remain unchanged.Likely an incorrect or invalid review comment.
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
363-370: Import cleanup in tests is correct and minimalSwitching the test
stdimports to only bring incollections::HashSetmatches actual usage intest_non_blocking_messagesand removes now‑unused imports. This is a safe clippy‑style cleanup with no behavioral impact on tests or production code.magicblock-ledger/src/blockstore_processor/mod.rs (1)
59-59: LGTM!The
is_multiple_ofmethod is the idiomatic replacement forslot % PROGRESS_REPORT_INTERVAL == 0introduced in Rust 1.91. SincePROGRESS_REPORT_INTERVALis a non-zero constant (100), this is semantically equivalent and cleaner.magicblock-ledger/src/store/api.rs (2)
210-210: LGTM - Explicit lifetime annotations.Adding
'_toRwLockReadGuardreturn types makes the elided lifetime explicit, which is a clippy lint fix for the new toolchain. This improves readability without changing behavior.Also applies to: 232-232
421-429: Documentation formatting adjustments.Minor whitespace changes in doc comments. No semantic changes.
Also applies to: 855-856, 1049-1050
magicblock-accounts-db/src/index.rs (1)
360-365: LGTM - Explicit lifetime annotation.The
'_lifetime onAccountOffsetFinder<'_>explicitly ties the returned iterator to the borrow ofself, which correctly captures the dependency on the read-only transaction created fromself.env. This aligns with the broader lifetime threading pattern in this PR.magicblock-chainlink/src/remote_account_provider/errors.rs (1)
10-12: LGTM - Boxing inner error for stack efficiency.Boxing
PubsubClientErrorreduces the size ofRemoteAccountProviderErroron the stack. The manualFromimpl correctly handles the boxing, replacing the previous#[from]attribute. This pattern is consistent with similar changes across the codebase (e.g.,magicblock-api/src/errors.rs,magicblock-task-scheduler/src/errors.rs).Also applies to: 91-98
magicblock-accounts-db/src/lib.rs (2)
185-205: LGTM - Lifetime threading forAccountsScanner.The explicit lifetime parameter
'dbonAccountsScannerand the return typeAccountsScanner<'_, F>correctly captures the borrow relationship with&self. The struct already held'db-lifetime references, so this makes the signature consistent with the actual constraints.Also applies to: 380-384
249-251: LGTM -is_multiple_offor snapshot frequency check.The
is_multiple_ofmethod replaces the modulo check. Sincesnapshot_frequencyis asserted non-zero at line 66 (assert_ne!(snapshot_frequency, 0, ...)), this is safe and semantically equivalent.magicblock-rpc-client/src/lib.rs (2)
50-85: LGTM - Consistent error boxing pattern.Boxing
solana_rpc_client_api::client_error::Errorin error variants reduces stack size forMagicBlockRpcClientResult. TheFromimpl correctly boxes the error, providing ergonomic conversion via.into().
247-260: Error propagation sites updated correctly.All error mapping locations properly construct boxed variants:
get_latest_blockhash:GetLatestBlockhash(Box::new(e))get_slot:GetSlot(Box::new(e))get_account:RpcClientError(Box::new(err))get_multiple_accounts_with_commitment:.into()(usesFromimpl)request_airdrop:RpcClientError(Box::new(e))send_transaction:SendTransaction(Box::new(e))get_transaction:RpcClientError(Box::new(e))Also applies to: 280-280, 318-318, 360-360, 407-409, 587-587
magicblock-validator-admin/src/claim_fees.rs (1)
81-84: LGTM - Error boxing aligned withMagicBlockRpcClientErrorchanges.The error mappings correctly use the boxed variants (
GetLatestBlockhash(Box::new(e))andSendTransaction(Box::new(e))) to match the updatedMagicBlockRpcClientErrorenum definition inmagicblock-rpc-client/src/lib.rs.Also applies to: 93-96
magicblock-rpc-client/src/utils.rs (2)
114-125: LGTM - Correct dereference of boxed errors.Since
RpcClientErrorandSendTransactionvariants now holdBox<Error>, the dereference*errcorrectly extracts the inner error for mapping viatry_map_client_error. The result is re-boxed when reconstructing the error variant.
143-174: LGTM -try_map_client_errorreturns boxed errors.The return type change to
Result<ExecErr, Box<...>>aligns with the boxed error variants. All error construction paths correctly wrap inBox::new(...), ensuring consistent boxing throughout the error handling chain.magicblock-committor-service/src/intent_executor/mod.rs (1)
676-698: Boxed RPC error mapping looks correctDereferencing the boxed
MagicBlockRpcClientErrorhere to pass the owned inner error intomap_magicblock_client_erroris the right adjustment after boxingInternalError::MagicBlockRpcClientError; it preserves previous behavior without double-boxing or cloning.magicblock-task-scheduler/src/errors.rs (1)
11-16: Pubsub boxing and explicitFromimpl are consistent and safeStoring
PubsubClientErrorin aBoxand adding an explicitFrom<...>keeps the enum size in check while preserving?ergonomics and transparent display behavior.Also applies to: 49-56
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
16-16: Safe NonZeroUsize initializationSwitching to
NonZeroUsize::new(...).unwrap()in consts removes the unsafe usage while remaining provably safe with positive literals (5, 1000). No behavioral change.Also applies to: 52-58
268-282: Boxed RPC client error inTaskInfoFetcherErroris wired correctlyWrapping
MagicBlockRpcClientErrorin aBoxplus the newFrom<MagicBlockRpcClientError>keeps?-based propagation working (e.g., infetch_metadata) andsignature()correctly delegates to the inner error via deref.magicblock-committor-service/src/service_ext.rs (1)
221-235: BoxingCommittorServiceErrorin the Ext error type is consistentThe boxed
CommittorServiceErrorvariant plus the explicitFromkeeps theawait??pattern inschedule_base_intents_waitingworking while aligning with the boxed-error pattern used elsewhere.magicblock-committor-service/src/error.rs (1)
42-51: Boxing init changeset RPC client errors is fineWrapping the init changeset
client_error::Errorvalues in aBoxmatches the general boxing strategy and doesn’t affect existing helpers (likesignature()), since those never inspected these variants.magicblock-committor-service/src/intent_executor/error.rs (1)
20-32: Internal RPC error boxing and conversion chain look correctMaking
InternalError::MagicBlockRpcClientErrorboxed and funnelingFrom<MagicBlockRpcClientError>forTransactionStrategyExecutionErrorthroughInternalError::fromis a clean centralization of the boxing logic. Thesignature()helper and existing?paths remain valid.Also applies to: 151-154
magicblock-committor-service/src/transaction_preparator/error.rs (1)
9-23: BoxedDeliveryPreparatorErrorintegration is soundStoring
DeliveryPreparatorErrorin aBoxand adding the explicitFromkeeps conversions ergonomic while allowingsignature()to keep delegating cleanly to the inner error.Also applies to: 25-31
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
346-354: LGTM - Correct dereference of boxed error.The dereference
*erris necessary here sincemap_magicblock_client_errorexpects the unboxedMagicBlockRpcClientError, while the variant now storesBox<MagicBlockRpcClientError>.
508-516: LGTM - Consistent boxing pattern for error conversion.The explicit
Fromimplementation correctly boxes the error, aligning with the project-wide pattern forMagicBlockRpcClientErrorconversions seen in other modules (e.g.,intent_executor/error.rs,task_info_fetcher.rs).
547-553: LGTM - Proper error chaining with boxing.The
Fromimplementation correctly boxes theMagicBlockRpcClientErrorwhen wrapping it inTransactionSendError, maintaining consistent error conversion semantics.
565-577: LGTM - Boxing reduces enum size.The boxed
MagicBlockRpcClientErrorvariant and explicitFromimplementation follow the established pattern, helping reduce the stack size ofInternalError.magicblock-api/src/errors.rs (3)
15-25: LGTM - Selective boxing of larger error types.Boxing
AccountsError,AccountClonerError,LedgerError, andChainlinkErrorwhile leaving smaller types unboxed is a reasonable approach to reduce enum size without over-engineering.
33-36: LGTM - Boxing applied consistently.The boxed service errors align with the project-wide pattern.
Also applies to: 92-95
103-141: LGTM - AllFromimplementations correctly box errors.The explicit
Fromimplementations consistently box each error type before wrapping, enabling ergonomic?operator usage while maintaining the boxed representation. This aligns with the pattern used across the codebase (e.g.,magicblock_accounts::errors,magicblock_task_scheduler::errors).magicblock-ledger/src/database/db.rs (1)
104-107: Lifetime‑bound raw iterator wrapper looks correctThe explicit
DBRawIterator<'_>return type cleanly ties the iterator’s lifetime to&selfand simply forwards toself.backend.raw_iterator_cf(cf). This is consistent with theRocks::raw_iterator_cfsignature and should satisfy the updated RocksDB API without changing behavior.magicblock-ledger/src/database/rocks_db.rs (5)
83-90: Pinned‑slice lifetime annotation is consistent with&selfChanging
get_pinned_cfto returnLedgerResult<Option<DBPinnableSlice<'_>>>matches the RocksDB API shape where the pinnable slice’s lifetime is tied to the DB reference. The body simply forwardsself.db.get_pinned_cf(...)and propagates the result, so behavior is unchanged.
102-117:multi_get_cfreturn type now correctly exposes lifetimes of pinned slicesThe new signature
pub fn multi_get_cf( &self, cf: &ColumnFamily, keys: Vec<&[u8]>, ) -> Vec<LedgerResult<Option<DBPinnableSlice<'_>>>>accurately reflects that each
DBPinnableSliceis borrowed from&self. The mapping from RocksDB’sbatched_multi_get_cfintoLedgerResultis unchanged, so this is purely a type‑level adjustment.
172-190: Iterator wrapper now explicitly lifetime‑bound; logic unchangedReturning
DBIterator<'_>fromiterator_cfis consistent with the iterator being tied to&self. The match that converts your higher‑levelIteratorMode<C::Index>intoRocksIteratorModeis unchanged, and you still just forward intoself.db.iterator_cf(cf, iterator_mode), so this is a safe, signature‑only update.
192-207: Raw‑key iterator lifetime annotation mirrors typed iterator semantics
iterator_cf_raw_keynow returningDBIterator<'_>aligns its signature withiterator_cfand the underlying RocksDB API. Construction ofRocksIteratorModefrom yourIteratorMode<Vec<u8>>is identical to before, so there’s no functional change beyond exposing the proper lifetime.
209-211:raw_iterator_cflifetime is now explicit and matches RocksDBSwitching to
pub fn raw_iterator_cf(&self, cf: &ColumnFamily) -> DBRawIterator<'_>cleanly ties the raw iterator’s lifetime to&selfwhile still just delegating toself.db.raw_iterator_cf(cf). This keeps the wrapper in sync with RocksDB’s type signatures.
| with: | ||
| build_cache_key_name: "magicblock-validator-ci-test-integration-${{ github.ref_name }}-${{ hashFiles('magicblock-validator/Cargo.lock') }}" | ||
| rust_toolchain_release: "1.84.1" | ||
| rust_toolchain_release: "1.91.1" |
There was a problem hiding this comment.
Inconsistent toolchain versions across CI jobs.
The build job uses 1.91.1 (line 27), but the run_integration_tests job still uses 1.84.1 (line 68). This creates a mismatch: code compiled with one toolchain is tested with another, which can mask toolchain-specific bugs or inconsistencies.
Update line 68 to 1.91.1 to maintain consistency across all CI jobs.
Apply this diff to fix the inconsistency:
- uses: ./magicblock-validator/.github/actions/setup-build-env
with:
build_cache_key_name: "magicblock-validator-ci-test-integration-${{ github.ref_name }}-${{ hashFiles('magicblock-validator/Cargo.lock') }}"
- rust_toolchain_release: "1.84.1"
+ rust_toolchain_release: "1.91.1"
github_access_token: ${{ secrets.GH_PERSONAL_ACCESS_TOKEN }}
github_token: ${{ secrets.GITHUB_TOKEN }}Also applies to: 68-68
🤖 Prompt for AI Agents
.github/workflows/ci-test-integration.yml lines 27 and 68: the Rust toolchain
versions are inconsistent across CI jobs (line 27 shows 1.91.1 while line 68
uses 1.84.1); update the rust_toolchain_release value at line 68 from 1.84.1 to
1.91.1 so both build and run_integration_tests jobs use the same 1.91.1
toolchain.
| #![allow(clippy::manual_is_multiple_of)] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the toolchain constraint with an inline comment.
The crate-level allow is justified given Solana's older toolchain constraint (as explained in the discussion). However, add an inline comment explaining why this exists and when it can be removed to help future maintainers.
Apply this diff to add documentation:
+// TODO: Remove once Solana upgrades to toolchain 1.87+
+// Needed because Solana build uses older toolchain but clippy runs with 1.91.1
#![allow(clippy::manual_is_multiple_of)]📝 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.
| #![allow(clippy::manual_is_multiple_of)] | |
| // TODO: Remove once Solana upgrades to toolchain 1.87+ | |
| // Needed because Solana build uses older toolchain but clippy runs with 1.91.1 | |
| #![allow(clippy::manual_is_multiple_of)] | |
🤖 Prompt for AI Agents
In magicblock-committor-program/src/lib.rs around lines 1 to 2, add an inline
comment next to the crate-level allow attribute explaining that
clippy::manual_is_multiple_of is allowed because the crate must maintain
compatibility with an older Solana toolchain that lacks the newer is_multiple_of
intrinsic; state when it can be removed (e.g., after upgrading the Solana
toolchain to a version that provides the intrinsic or when the minimum supported
Rust/toolchain is bumped), and include a short reference to the issue/discussion
or PR that justifies the exception.
| if b.is_empty() { | ||
| "None".to_string() | ||
| } else { | ||
| { | ||
| b.iter() | ||
| .map(|b| b.ui_token_amount.amount.to_string()) | ||
| .collect::<Vec<_>>() | ||
| .join(" | ") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing unnecessary inner braces.
The else block contains redundant braces around the iterator chain (lines 60-65). Removing them would improve readability and match idiomatic Rust style.
Apply this diff to simplify:
|b| {
if b.is_empty() {
"None".to_string()
} else {
- {
- b.iter()
- .map(|b| b.ui_token_amount.amount.to_string())
- .collect::<Vec<_>>()
- .join(" | ")
- }
+ b.iter()
+ .map(|b| b.ui_token_amount.amount.to_string())
+ .collect::<Vec<_>>()
+ .join(" | ")
}
},📝 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.
| if b.is_empty() { | |
| "None".to_string() | |
| } else { | |
| { | |
| b.iter() | |
| .map(|b| b.ui_token_amount.amount.to_string()) | |
| .collect::<Vec<_>>() | |
| .join(" | ") | |
| } | |
| } | |
| if b.is_empty() { | |
| "None".to_string() | |
| } else { | |
| b.iter() | |
| .map(|b| b.ui_token_amount.amount.to_string()) | |
| .collect::<Vec<_>>() | |
| .join(" | ") | |
| } |
🤖 Prompt for AI Agents
In tools/ledger-stats/src/transaction_details.rs around lines 57 to 66, the else
branch contains redundant inner braces around the iterator chain; remove the
extra braces so the branch simply returns b.iter().map(|b|
b.ui_token_amount.amount.to_string()).collect::<Vec<_>>().join(" | ") (keeping
the surrounding if/else structure intact) to simplify and match idiomatic Rust.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.