feat: compression#1171
Open
Dodecahedr0x wants to merge 78 commits intomasterfrom
Open
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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces end-to-end compressed-delegation support: two new crates (compressed-delegation-api, compressed-delegation-client) with instruction/state types, builders, and CPI helpers; integrates a Photon indexer and PhotonClient implementation, threading a Photon generic through RemoteAccountProvider, FetchCloner, and Chainlink; adds compression-aware paths and a compressed: bool flag across committor, task building, and MagicBlock flows; adds compression configuration and validator toggle; updates many tests, configs, and workspace dependencies. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
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/config.rs (1)
19-33:⚠️ Potential issue | 🟠 MajorDo not default devnet/mainnet Photon URI to localhost.
Line 22 and Line 32 make
photon_urieffectively always enabled for non-local configs, buthttp://localhost:8784is environment-specific and likely unavailable outside local setups.Suggested fix
pub fn devnet(compute_budget_config: ComputeBudgetConfig) -> Self { Self { rpc_uri: "https://api.devnet.solana.com".to_string(), - photon_uri: Some("http://localhost:8784".to_string()), + photon_uri: None, commitment: CommitmentConfig::confirmed(), compute_budget_config, actions_timeout: DEFAULT_ACTIONS_TIMEOUT, } } pub fn mainnet(compute_budget_config: ComputeBudgetConfig) -> Self { Self { rpc_uri: "https://api.mainnet-beta.solana.com".to_string(), - photon_uri: Some("http://localhost:8784".to_string()), + photon_uri: None, commitment: CommitmentConfig::confirmed(), compute_budget_config, actions_timeout: DEFAULT_ACTIONS_TIMEOUT, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/config.rs` around lines 19 - 33, The devnet and mainnet constructors (pub fn devnet and pub fn mainnet) currently set photon_uri to Some("http://localhost:8784"), which wrongly enables a local-only Photon endpoint for non-local environments; change these to either None or to read from an environment/config value instead of hardcoding localhost so photon_uri is not enabled by default for non-local configs—update the photon_uri assignment in both devnet and mainnet to None (or to pull from a config/env var) and ensure any code that uses photon_uri handles the None case appropriately.programs/magicblock/src/magic_scheduled_base_intent.rs (1)
224-236:⚠️ Potential issue | 🟠 MajorExtend the bundle walkers for the new compressed finalize intents.
These new variants can carry
WithBaseActions, butMagicIntentBundle::has_callbacks()andMagicIntentBundle::get_action_mut()still skip them. A compressed finalize bundle can therefore contain callbacks/actions that are never detected or addressable.Also applies to: 296-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/magicblock/src/magic_scheduled_base_intent.rs` around lines 224 - 236, The new compressed finalize variants (CommitFinalizeCompressed, CommitFinalizeAndUndelegateCompressed) and the corresponding MagicIntentBundle fields (commit_finalize_compressed, commit_finalize_compressed_and_undelegate) are not considered by the bundle walkers, so callbacks/actions within these compressed intents are ignored; update MagicIntentBundle::has_callbacks() and MagicIntentBundle::get_action_mut() (and any other bundle-walker helpers around the same logic) to treat commit_finalize_compressed and commit_finalize_compressed_and_undelegate the same as commit_finalize and commit_finalize_and_undelegate respectively, i.e., check those Option<> fields for WithBaseActions and return/return a mutable reference to the inner action when present so compressed finalize intents’ callbacks are detected and addressable.magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
55-57: 🧹 Nitpick | 🔵 TrivialUpdate outdated assumption in inline comment
The comment says a delegation record must exist when delegated, but compressed flow now explicitly supports delegated accounts without classic DLP records.
Suggested fix
- // Since the account was found to be delegated we must have - // found a delegation record and thus have the delegation slot. + // For classic DLP, delegation slot comes from delegation record. + // For compressed flow (no classic DLP), use compressed delegation slot fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs` around lines 55 - 57, The inline comment above the use of deleg_record (the line assigning delegation_slot = deleg_record) wrongly assumes a delegation record always exists for delegated accounts; update the comment to reflect that compressed flow can have delegated accounts without classic DLP records and describe the actual behavior (e.g., "If a delegation record exists use its slot; otherwise handle compressed delegation flow where no DLP record is present"). Keep the code path unchanged but make the comment accurate and reference deleg_record and delegation_slot so future readers know both cases are possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compressed-delegation-api/Cargo.toml`:
- Line 5: The Cargo.toml entry for the package uses an incorrect license-file
path; update the license-file key value from "../../LICENSE" to point at the
repository root LICENSE.md by changing the license-file setting in Cargo.toml
(look for the license-file = "..."/license-file key) so it reads the correct
relative path "../LICENSE.md".
In `@compressed-delegation-api/src/instruction.rs`:
- Around line 192-199: The current impl From<CdpValidityProof> for ValidityProof
swallows CompressedProof deserialization errors by using .ok(), making corrupted
proof bytes indistinguishable from absent proofs; change this to an
infallible-to-fallible conversion by implementing TryFrom<CdpValidityProof> for
ValidityProof (or otherwise returning Result<ValidityProof, InvalidData>) and
call CompressedProof::try_from(&bytes[..])?, mapping the underlying error into
an appropriate InvalidData error variant; update any callers that relied on the
infallible From to handle the Result and propagate or log the error accordingly
so invalid proofs are surfaced instead of dropped.
In `@compressed-delegation-api/src/state.rs`:
- Around line 35-40: The implementation of DataHasher for
CompressedDelegationRecord is ignoring the generic hasher type H and always uses
Sha256::hash; replace that hardcoded call with the caller-selected hasher by
invoking H's hashing API (e.g., H::hash or the appropriate method on H) on the
Borsh-serialized bytes inside fn hash<H: Hasher>(&self) -> Result<[u8; 32],
HasherError>, handle/map any hasher errors to HasherError (like the existing
BorshError mapping), then set hash[0] = 0 and return the resulting [u8; 32];
keep references to CompressedDelegationRecord, DataHasher, and the generic hash
function in your change and remove the Sha256::hash usage.
In `@compressed-delegation-client/Cargo.toml`:
- Around line 3-8: Update the package metadata in Cargo.toml: replace the
description field to accurately describe this crate (not "Metaplex Project
Template"), set the repository field to the correct MagicBlock repository URL,
and change license-file to point to an existing LICENSE location within the
workspace (or use license = "..." and remove license-file if appropriate);
ensure you modify the description, repository, and license-file entries in
compressed-delegation-client/Cargo.toml so the values match the project
workspace and resolve to an actual file.
In `@compressed-delegation-client/src/builders/commit_finalize.rs`:
- Line 6: The module-level doc comment currently labels this as "Commit" but the
builder constructs CommitAndFinalize; update the doc title/comment to say
"CommitAndFinalize" (or otherwise match the builder name) so the documentation
correctly reflects the builder implemented in this file (referencing the
CommitAndFinalize builder/struct in commit_finalize.rs).
In `@magicblock-api/src/magic_validator.rs`:
- Around line 495-504: In the block that extracts photon_url from config (the if
let Some(photon_url) = config.compression.as_ref().and_then(|compression|
compression.photon_url.clone()) pattern), you already cloned the String into
photon_url, so remove the redundant photon_url.clone() when constructing
Endpoint::Compression and use photon_url directly; update the
Endpoint::Compression instantiation to use url: photon_url instead of url:
photon_url.clone().
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 230-237: The mutex lock in
mark_post_undelegation_photon_merge_pending currently uses .expect(), which can
panic on a poisoned lock; change the lock acquisition to handle poisoning (for
example use .lock().unwrap_or_else(|poisoned| poisoned.into_inner()) or
propagate a Result) and then insert the pubkey into
post_undelegation_photon_merge_pending; update the function signature to return
a Result if you choose to propagate errors instead of recovering silently.
- Around line 1855-1878: The code currently calls .expect() on the
post_undelegation_photon_merge_pending mutex in two places (the block that binds
pending and the block that checks .contains()), which will panic on a poisoned
lock; replace both .expect() usages with proper error handling: attempt to lock
with lock() (or try_lock()), match the Result, log an error (including the mutex
name and context) and recover by either treating the pending set as empty/skip
the remove/contains logic or returning early from the surrounding function as
appropriate, ensuring behavior remains safe when
account_in_bank/delegated/compressed checks run; keep references to
post_undelegation_photon_merge_pending, pending.remove(*pubkey), and the boolean
expression using self.is_watching(pubkey) and account_in_bank for locating the
changes.
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Around line 1446-1457: The code currently returns FetchedRemoteAccounts::Rpc
filled with RemoteAccount::NotFound when photon_client is None, which
masquerades a no-Photon condition as an RPC result; instead add and return a
distinct variant (e.g., FetchedRemoteAccounts::PhotonUnavailable or ::NoPhoton)
from the branch that handles photon_client being None, populate it with the
appropriate counts (0 RPC matches, pubkeys.len() missing count or similar), and
update any callers that match on FetchedRemoteAccounts to handle the new
variant; reference the photon_client check and the FetchedRemoteAccounts::Rpc /
RemoteAccount::NotFound usage when making the change.
- Around line 1131-1153: The loop over results collects partial fetches into
remote_accounts_results and then calls
Self::consolidate_fetched_remote_accounts(&pubkeys, remote_accounts), but if
backends fail or consolidation returns fewer accounts some request receivers
remain unresolved; update the logic after consolidation to detect which pubkeys
are still missing and explicitly fail/complete their waiting responders so they
don't stay pending forever. Concretely: after calling
consolidate_fetched_remote_accounts, compute the set of pubkeys not present in
the consolidated result (using the original pubkeys vector and keys in
remote_accounts), then iterate the corresponding responder channels/entries and
send an error/None to complete them; also on Err(err) branches where both
backends fail ensure you mark those entries as failed immediately (e.g., push
failed placeholders or complete responders) so no receivers are left waiting.
In `@magicblock-chainlink/src/remote_account_provider/photon_client.rs`:
- Around line 31-33: The debug call in pub fn new_from_url(url: String) in
PhotonClient leaks secrets by logging the raw URL; update new_from_url (and its
call site to PhotonIndexer::new) to stop emitting the full url—either remove the
debug!(url = %url, ...) line or replace it with a sanitized value (e.g., mask
query/credential parts or log only the hostname) before logging, ensuring no API
keys or credentials are printed.
In `@magicblock-chainlink/src/testing/accounts.rs`:
- Around line 45-56: The serialization uses borsh::to_vec(...).unwrap() which
can panic; change the helper that builds delegation_record_bytes (the helper in
this file that constructs CompressedDelegationRecord and assigns
delegation_record_bytes) to return a Result and replace .unwrap() with the
fallible ? operator (e.g., let delegation_record_bytes =
borsh::to_vec(&CompressedDelegationRecord { ... })?;), updating the helper's
signature to propagate the borsh serialization error (or a suitable error type)
so callers can handle failures instead of panicking.
In `@magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs`:
- Around line 212-222: The redelegation setup currently constructs the
compressed account with compressed_account_shared_with_owner_and_slot(pubkey,
other_authority, slot, acc) (which sets the delegation record slot) but then
calls photon_client.add_account(pubkey, compressed_account.clone().into(), slot)
without setting the AccountSharedData slot; call set_remote_slot(slot) on the
AccountSharedData (the compressed_account before converting/into) so both the
delegation record and the shared-data slot are set consistently (i.e., invoke
set_remote_slot(slot) on compressed_account prior to photon_client.add_account).
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Around line 94-105: The default implementation of
get_compressed_data_for_accounts uses try_join_all and therefore returns
Ok(Some(...)) for every successful fetch and bubbles any individual error to
fail the whole call, which contradicts the Vec<Option<CompressedData>>
signature; update the implementation to call self.get_compressed_data for each
pubkey but convert per-call errors into None (e.g. use futures::future::join_all
over pubkeys.iter().map(|pubkey| async move { match
self.get_compressed_data(pubkey, min_context_slot).await { Ok(cd) => Ok::<_,
TaskInfoFetcherError>(Some(cd)), Err(_) => Ok(None) } }) and then collect the
Vec<Option<CompressedData>>), or if you decide that missing/failed fetches
should be treated as errors globally, remove the Option wrapper from the return
type and keep try_join_all—pick one and make the code and signature consistent
(refer to get_compressed_data_for_accounts, get_compressed_data, try_join_all,
Vec<Option<CompressedData>>).
- Around line 378-403: The current filter_map in the branch that builds `nonces`
silently drops failed deserializations and shifts nonces relative to `pubkeys`;
change the `filter_map(|compressed_account| { ... })` to a `map(...)` that
returns an Option for each input (e.g., map each compressed_account to
Some(last_update_nonce) or None on failure) so the resulting iterator preserves
position and can be zipped correctly with `pubkeys`; apply the same pattern used
in `fetch_next_commit_nonces` (keep `derive_cda_from_pda`,
`CompressedDelegationRecord::try_from_slice`, and return
Option<record.last_update_nonce>` so `pubkeys.iter().copied().zip(nonces)` pairs
the right items).
- Around line 330-355: The code zips pubkeys with a filtered `nonces` iterator
so missing or failed deserializations shift positions; change the logic to
preserve index alignment by mapping the response items back to the original
pubkeys order instead of using filter_map. Specifically, after calling
`self.photon_client()?.get_multiple_compressed_accounts(...)`, iterate the
returned `.value.items` with position (e.g., enumerate) or collect into a
Vec<Option<u64>> where each element is computed by trying to deserialize via
`CompressedDelegationRecord::try_from_slice` and mapping to
`record.last_update_nonce + 1` (or None on failure), then pair
`pubkeys.iter().copied()` with that aligned vector (or return an error/None per
pubkey as appropriate) so `derive_cda_from_pda`, `photon_client`, and
`CompressedDelegationRecord` usages preserve index alignment and do not silently
drop entries.
In `@magicblock-committor-service/src/tasks/commit_finalize_compressed_task.rs`:
- Around line 28-63: The instruction() function currently uses three .expect()
calls that can panic (deserializing CompressedDelegationRecord, borsh::to_vec
for new_record, and builders::CommitAndFinalizeBuilder::instruction()), so
change its signature to return Result<Instruction, E> (e.g., Result<Instruction,
CommitFinalizeError>), replace each .expect() with ? or map_err(...) to convert
the underlying error into that error type (handle the
CompressedDelegationRecord::try_from_slice error, borsh::to_vec error, and the
builder.instruction() error), and propagate errors instead of panicking; update
or add a small error enum/From impls to wrap/describable these error cases and
adjust callers to handle the Result.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 155-176: fetch_compressed_data currently silently drops accounts
whose compressed data is missing; instead, detect any missing entry in the
result of task_info_fetcher.get_compressed_data_for_accounts and return a
TaskBuilderError so the bundle fails/retries atomically. Update
fetch_compressed_data (and the analogous blocks referenced elsewhere) to: call
get_compressed_data_for_accounts(&pubkeys, Some(min_context_slot)).await?; check
that the returned collection contains an entry for every requested pubkey; if
any pubkey is absent, return Err(TaskBuilderError::MissingCompressedData {
pubkey, context_slot: min_context_slot }) (or the existing TaskBuilderError
variant used for missing data); otherwise construct and return the
HashMap<Pubkey, CompressedData> as before. Ensure you reference the
TaskInfoFetcher::get_compressed_data_for_accounts call and the TaskBuilderError
type when implementing the change.
In `@magicblock-core/src/compression.rs`:
- Around line 19-20: The call to
hashv_to_bn254_field_size_be_const_array::<3>(&[&pda.to_bytes()]).expect(...)
must not panic; change the containing function in compression.rs to return a
Result (or Option) and replace .expect(...) with the fallible call using the ?
operator (or map_err to convert to your error type), returning an error if
hashing fails; update callers of that function to propagate or handle the Result
accordingly. Ensure you reference the exact symbol
hashv_to_bn254_field_size_be_const_array::<3> and the use of pda.to_bytes() when
adjusting the function signature and error mapping so callers can compile and
handle the error path.
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 855-879: The new compressed-account helpers increment collectors
COMPRESSED_ACCOUNT_FETCHES_SUCCESS_COUNT,
COMPRESSED_ACCOUNT_FETCHES_FOUND_COUNT,
COMPRESSED_ACCOUNT_FETCHES_NOT_FOUND_COUNT, and
COMPRESSED_ACCOUNT_FETCHES_FAILED_COUNT but those collectors are never
registered with REGISTRY; update the register() function to register each of
those collectors so they are exported (ensure you register the FOUND/NOT_FOUND
collectors with the proper label for AccountFetchOrigin as created). Locate the
register() function and add the corresponding register calls for the four
COMPRESSED_* collectors so the metrics become visible to the metrics endpoint.
In
`@programs/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rs`:
- Around line 147-153: The code is always passing hardcoded false for
compression mode to fetch_current_commit_nonces and check_commit_limits even
when the bundle may include compressed finalize accounts; detect whether the
bundle contains compressed commit accounts (e.g., inspect
get_all_committed_accounts() or add a helper like has_compressed_commits() on
scheduled_intent similar to process_schedule_commit_finalize) and derive a
boolean compression_mode; then pass that boolean instead of false into
fetch_current_commit_nonces(&chargable_accounts, compression_mode) and
check_commit_limits(commit_accounts, compression_mode, invoke_context) so nonce
fetching and limit checks use the correct compressed vs uncompressed source.
In `@test-integration/programs/flexi-counter/src/processor.rs`:
- Around line 669-694: The DelegateCpi call is zeroing out the delegated
account's lamports (DelegateArgs.lamports = 0) which causes any surplus lamports
to be lost on undelegate; instead capture and pass through the current lamports
from counter_pda_info when constructing
compressed_delegation_client::cpi::DelegateCpi (use the existing account_data
and the actual lamports value read from counter_pda_info before you drain it) so
the delegation program can round-trip the full balance back on undelegate;
update the DelegateArgs.lamports field to that captured lamports value and
ensure you still transfer ownership/data cleanup via account_data and
signer_seeds (symbols: counter_pda_info, payer_info, DelegateCpi, DelegateArgs,
FlexiCounter::seeds_with_bump).
- Around line 713-741: The CPI currently uses the caller-supplied
magic_program.key as Instruction.program_id and omits the program AccountInfo
from invoke, which can call the wrong program or fail; change the
Instruction.program_id to the canonical MAGIC_PROGRAM_ID, add the program
account to the accounts metadata (e.g., an AccountMeta for magic_program as
readonly) and include magic_program.clone() in the account_refs passed to invoke
so the callee program account is provided to the runtime when invoking
MagicBlockInstruction::ScheduleCommitAndUndelegate.
In `@test-integration/programs/schedulecommit/src/lib.rs`:
- Around line 258-308: The compressed branches
(ScheduleCommitType::CommitFinalizeCompressed and
CommitFinalizeCompressedAndUndelegate) currently hardcode
magicblock_magic_program_api::ID; change both Instruction::new_with_bincode
calls to use the provided magic_program account's pubkey (use
*magic_program.key) as the program id, and ensure the magic_program account is
included in the all_accounts vector passed to invoke (alongside payer,
magic_context and the other accounts) so the CPI targets the caller-supplied
program; modify the two places where magicblock_magic_program_api::ID appears
and add magic_program.clone() into the all_accounts construction used before
invoke.
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`:
- Around line 394-401: The new ScheduleCommitType variants
CommitFinalizeCompressed and CommitFinalizeCompressedAndUndelegate are never
exercised by the tests; update the test invocations that call
run_test_for_commit_huge_order_book_account to also call it with
ScheduleCommitType::CommitFinalizeCompressed and
ScheduleCommitType::CommitFinalizeCompressedAndUndelegate (mirror how
CommitFinalize and CommitFinalizeAndUndelegate are used) so the match arms
handling compressed paths are executed and validated.
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 1286-1311: The current block skips validation for compressed
accounts; instead, when is_compressed is true fetch the delegated compressed
account via the Photon indexer/client (instead of using the existing RPC-only
get_account! path) and run the same validation logic (call validate_account with
the fetched Account, remaining_tries handling from Photon is unnecessary because
PhotonIndexer has built-in retries) to assert the expected_owner and data
transitions after commit/finalize/undelegate; replace or augment the
get_account!/rpc_client usage with a Photon fetch for account.pubkey and ensure
validate_account is invoked for compressed accounts as well (referencing
account.pubkey, account.account.lamports, validate_account, and expected_owner).
In `@test-integration/test-committor-service/tests/utils/transactions.rs`:
- Around line 417-437: The retry loop currently calls value.unwrap() on the
result of photon_indexer.get_compressed_account which panics on transient None
responses; change the logic in the loop around
photon_indexer.get_compressed_account(address, None).await to inspect the
response.value (and return early only when Some) and to treat missing value or a
failed CompressedDelegationRecord::try_from_slice decode as a retry condition:
decrement retries, sleep, and continue; only break with
compressed_delegation_record when response.value is Some and try_from_slice
succeeds, and keep using MAX_RETRIES and the existing sleep to terminate or
retry.
- Around line 340-344: The code is calling rpc_client.request_airdrop(...) which
can return before lamports are spendable; replace this with the safer helper
that waits for confirmation (use the existing airdrop_and_confirm helper or call
request_airdrop then explicitly confirm the transaction) so the init/delegate
transactions won't race the faucet; update the call that currently references
rpc_client.request_airdrop(&counter_auth.pubkey(), 777 *
LAMPORTS_PER_SOL).await.unwrap() to instead invoke
airdrop_and_confirm(&rpc_client, &counter_auth.pubkey(), 777 *
LAMPORTS_PER_SOL).await (or perform request_airdrop and then
rpc_client.confirm_transaction on the returned signature) and keep the debug log
after confirmation.
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 191-194: The tests use the start_devnet_validator closure which
calls start_validator with ValidatorCluster::Light but the teardown still calls
cleanup_devnet_only, so background light validators aren't stopped; update the
teardown logic in the affected test functions (the ones using
start_devnet_validator / start_validator with ValidatorCluster::Light) to
perform the light-specific stop command (equivalent to running `light
test-validator --stop`) or invoke the existing light teardown helper instead of
cleanup_devnet_only so the light validator process is properly terminated and
ports aren't leaked.
In `@test-integration/test-tools/src/validator.rs`:
- Around line 188-219: The code uses a fixed prover_port and a blind sleep,
causing port conflicts and racey readiness; change the prover_port assignment to
allocate a free ephemeral port (e.g., bind a TcpListener to "127.0.0.1:0" and
take its local_port), insert that port into light_args and the script, and pass
the dynamic port into wait_for_validator (or update wait_for_validator to accept
the allocated port); remove the fixed sleep(Duration::from_secs(10)) and instead
actively poll the validator RPC (e.g., attempt TCP connect or an HTTP/RPC health
call to the RPC port) until it responds or a timeout elapses before returning
child. Ensure any temporary listener is closed before spawning the process so
the validator can bind the port.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs`:
- Around line 55-57: The inline comment above the use of deleg_record (the line
assigning delegation_slot = deleg_record) wrongly assumes a delegation record
always exists for delegated accounts; update the comment to reflect that
compressed flow can have delegated accounts without classic DLP records and
describe the actual behavior (e.g., "If a delegation record exists use its slot;
otherwise handle compressed delegation flow where no DLP record is present").
Keep the code path unchanged but make the comment accurate and reference
deleg_record and delegation_slot so future readers know both cases are possible.
In `@magicblock-committor-service/src/config.rs`:
- Around line 19-33: The devnet and mainnet constructors (pub fn devnet and pub
fn mainnet) currently set photon_uri to Some("http://localhost:8784"), which
wrongly enables a local-only Photon endpoint for non-local environments; change
these to either None or to read from an environment/config value instead of
hardcoding localhost so photon_uri is not enabled by default for non-local
configs—update the photon_uri assignment in both devnet and mainnet to None (or
to pull from a config/env var) and ensure any code that uses photon_uri handles
the None case appropriately.
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 224-236: The new compressed finalize variants
(CommitFinalizeCompressed, CommitFinalizeAndUndelegateCompressed) and the
corresponding MagicIntentBundle fields (commit_finalize_compressed,
commit_finalize_compressed_and_undelegate) are not considered by the bundle
walkers, so callbacks/actions within these compressed intents are ignored;
update MagicIntentBundle::has_callbacks() and
MagicIntentBundle::get_action_mut() (and any other bundle-walker helpers around
the same logic) to treat commit_finalize_compressed and
commit_finalize_compressed_and_undelegate the same as commit_finalize and
commit_finalize_and_undelegate respectively, i.e., check those Option<> fields
for WithBaseActions and return/return a mutable reference to the inner action
when present so compressed finalize intents’ callbacks are detected and
addressable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97dcbc82-53fb-4ffd-893b-281795552c37
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/programs/compressed_delegation/compressed_delegation.sois excluded by!**/*.so
📒 Files selected for processing (130)
.github/workflows/ci-test-integration.ymlCargo.tomlcompressed-delegation-api/Cargo.tomlcompressed-delegation-api/src/instruction.rscompressed-delegation-api/src/lib.rscompressed-delegation-api/src/state.rscompressed-delegation-client/Cargo.tomlcompressed-delegation-client/src/builders/commit_finalize.rscompressed-delegation-client/src/builders/delegate.rscompressed-delegation-client/src/builders/init_delegation_record.rscompressed-delegation-client/src/builders/mod.rscompressed-delegation-client/src/builders/undelegate.rscompressed-delegation-client/src/cpi/commit_finalize.rscompressed-delegation-client/src/cpi/delegate.rscompressed-delegation-client/src/cpi/helpers.rscompressed-delegation-client/src/cpi/init_delegation_record.rscompressed-delegation-client/src/cpi/mod.rscompressed-delegation-client/src/cpi/undelegate.rscompressed-delegation-client/src/lib.rscompressed-delegation-client/src/mod.rscompressed-delegation-client/src/programs.rscompressed-delegation-client/src/shared.rsconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-aperture/src/state/mod.rsmagicblock-api/Cargo.tomlmagicblock-api/src/magic_sys_adapter.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/compression.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_updates_client.rsmagicblock-chainlink/src/remote_account_provider/endpoint.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/photon_client.rsmagicblock-chainlink/src/remote_account_provider/remote_account.rsmagicblock-chainlink/src/testing/accounts.rsmagicblock-chainlink/src/testing/mod.rsmagicblock-chainlink/src/testing/photon_client_mock.rsmagicblock-chainlink/src/testing/utils.rsmagicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-chainlink/tests/03_deleg_after_sub.rsmagicblock-chainlink/tests/04_redeleg_other_separate_slots.rsmagicblock-chainlink/tests/05_redeleg_other_same_slot.rsmagicblock-chainlink/tests/06_redeleg_us_separate_slots.rsmagicblock-chainlink/tests/07_redeleg_us_same_slot.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-committor-service/Cargo.tomlmagicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/config.rsmagicblock-committor-service/src/error.rsmagicblock-committor-service/src/intent_execution_manager/intent_scheduler.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/utils.rsmagicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/service.rsmagicblock-committor-service/src/service_ext.rsmagicblock-committor-service/src/stubs/changeset_committor_stub.rsmagicblock-committor-service/src/tasks/commit_finalize_compressed_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-config/README.mdmagicblock-config/src/config/compression.rsmagicblock-config/src/config/mod.rsmagicblock-config/src/lib.rsmagicblock-config/src/tests.rsmagicblock-core/Cargo.tomlmagicblock-core/src/compression.rsmagicblock-core/src/lib.rsmagicblock-core/src/traits.rsmagicblock-magic-program-api/src/args.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-metrics/src/metrics/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/magic_sys.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/mutate_accounts/process_mutate_accounts.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rsprograms/magicblock/src/test_utils/mod.rsprograms/magicblock/src/utils/instruction_utils.rsprograms/magicblock/src/validator.rstest-integration/Cargo.tomltest-integration/configs/chainlink-conf.devnet.tomltest-integration/configs/committor-conf.devnet.tomltest-integration/configs/schedulecommit-conf.devnet.tomltest-integration/programs/flexi-counter/Cargo.tomltest-integration/programs/flexi-counter/src/instruction.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/flexi-counter/src/processor/schedule_intent.rstest-integration/programs/flexi-counter/src/state.rstest-integration/programs/schedulecommit/Cargo.tomltest-integration/programs/schedulecommit/src/lib.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-chainlink/Cargo.tomltest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/src/test_context.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_03_deleg_after_sub.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rstest-integration/test-committor-service/Cargo.tomltest-integration/test-committor-service/tests/common.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/instructions.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-runner/bin/run_tests.rstest-integration/test-runner/src/cleanup.rstest-integration/test-runner/src/signal.rstest-integration/test-tools/Cargo.tomltest-integration/test-tools/src/validator.rstest-kit/Cargo.tomltest-kit/src/lib.rs
Dodecahedr0x
commented
Apr 29, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
224-236:⚠️ Potential issue | 🟠 MajorInclude the compressed intents in callback/action traversal.
These new variants can carry
WithBaseActions, but the bundle helpers still only traverse the pre-compressed paths. Any callbacks on compressed finalize bundles will be treated as absent, and later mutation/patching won't reach those actions.♻️ Suggested update
pub fn has_callbacks(&self) -> bool { let x = self .commit .as_ref() .map(|el| el.has_callbacks()) .unwrap_or(false); let y = self .commit_and_undelegate .as_ref() .map(|el| el.has_callbacks()) .unwrap_or(false); - let z = self + let z = self + .commit_finalize + .as_ref() + .map(|el| el.has_callbacks()) + .unwrap_or(false); + let w = self + .commit_finalize_and_undelegate + .as_ref() + .map(|el| el.has_callbacks()) + .unwrap_or(false); + let compressed = self + .commit_finalize_compressed + .as_ref() + .map(|el| el.has_callbacks()) + .unwrap_or(false); + let compressed_undelegate = self + .commit_finalize_compressed_and_undelegate + .as_ref() + .map(|el| el.has_callbacks()) + .unwrap_or(false); + let standalone = self .standalone_actions .iter() .any(|el| el.callback.is_some()); - x || y || z + x || y || z || w || compressed || compressed_undelegate || standalone }Please mirror the same intent set in
get_action_muttoo.Also applies to: 257-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/magicblock/src/magic_scheduled_base_intent.rs` around lines 224 - 236, The traversal helpers for MagicIntentBundle currently ignore the new compressed variants so callbacks/actions on CommitFinalizeCompressed and CommitFinalizeAndUndelegateCompressed never get visited; update the code paths that iterate/inspect actions (including the functions that build/collect callbacks and the get_action_mut method) to include the fields commit_finalize_compressed and commit_finalize_compressed_and_undelegate alongside their uncompressed counterparts (commit_finalize, commit_finalize_and_undelegate) so that WithBaseActions carried by those variants are traversed and mutable access via get_action_mut covers compressed intents as well.
♻️ Duplicate comments (2)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
94-106:⚠️ Potential issue | 🟠 MajorPropagate real compressed-fetch failures.
get_compressed_data_for_accounts()turns everyget_compressed_data()error intoNone, so callers loseCompressionNotConfigured,IndexerError, and any signature context and only see a generic “missing compressed data” later. ReserveNonefor genuine absence and bubble the rest.Proposed fix
async fn get_compressed_data_for_accounts( &self, pubkeys: &[Pubkey], min_context_slot: Option<u64>, ) -> TaskInfoFetcherResult<Vec<Option<CompressedData>>> { try_join_all(pubkeys.iter().map(|pubkey| async move { - Ok(self - .get_compressed_data(pubkey, min_context_slot) - .await - .ok()) + match self.get_compressed_data(pubkey, min_context_slot).await { + Ok(data) => Ok(Some(data)), + Err( + TaskInfoFetcherError::CompressedAccountNotFound(_) + | TaskInfoFetcherError::EmptyCompressedAccount + | TaskInfoFetcherError::MissingCompressedData, + ) => Ok(None), + Err(err) => Err(err), + } })) .await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs` around lines 94 - 106, get_compressed_data_for_accounts currently swallows all errors from get_compressed_data by calling .ok(), turning failures into None; change the inner async closure so errors are propagated instead of converted to None (i.e., return the Result<Option<CompressedData>, _> from get_compressed_data directly so try_join_all can short-circuit on Err). Concretely, remove the .ok() and any wrapping that converts Result to Option, using the await'ed Result from get_compressed_data(pubkey, min_context_slot).await (or the ? operator inside the closure) so get_compressed_data_for_accounts returns Err for real fetch failures and only yields None when get_compressed_data succeeds but returns None.magicblock-committor-service/src/tasks/task_builder.rs (1)
155-176:⚠️ Potential issue | 🟠 MajorFail on missing compressed accounts before persisting commit IDs.
This helper still builds a sparse map by dropping
Nones. The actual error is not raised until Lines 329-343, after Lines 303-310 have already persisted commit IDs, so one missing compressed fetch can leave orphaned persisted state behind.Proposed fix
async fn fetch_compressed_data<C: TaskInfoFetcher>( task_info_fetcher: &Arc<C>, accounts: &[(bool, bool, bool, CommittedAccount)], min_context_slot: u64, ) -> TaskInfoFetcherResult<HashMap<Pubkey, CompressedData>> { let pubkeys = accounts .iter() .filter(|(_, _, compressed, _)| *compressed) .map(|(_, _, _, account)| account.pubkey) .collect::<Vec<_>>(); - Ok(pubkeys - .iter() - .zip( - task_info_fetcher - .get_compressed_data_for_accounts( - &pubkeys, - Some(min_context_slot), - ) - .await?, - ) - .filter_map(|(pk, data)| data.map(|data| (*pk, data))) - .collect()) + let data = task_info_fetcher + .get_compressed_data_for_accounts(&pubkeys, Some(min_context_slot)) + .await?; + + pubkeys + .into_iter() + .zip(data) + .map(|(pubkey, maybe_data)| { + maybe_data + .map(|data| (pubkey, data)) + .ok_or(TaskInfoFetcherError::CompressedAccountNotFound( + pubkey, + )) + }) + .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/task_builder.rs` around lines 155 - 176, The helper fetch_compressed_data currently drops None results and returns a sparse HashMap, which delays the error until later and can leave persisted commit IDs orphaned; modify fetch_compressed_data (the async fn fetch_compressed_data<C: TaskInfoFetcher>) to call task_info_fetcher.get_compressed_data_for_accounts(&pubkeys, Some(min_context_slot)).await?, inspect the returned Vec<Option<CompressedData>> for any None entries, and if any are missing return an Err (with context including the missing Pubkeys) instead of filtering them out; only build and return the HashMap when all pubkeys have Some(CompressedData).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compressed-delegation-api/Cargo.toml`:
- Line 12: Remove the stale commented dependency line "# borsh10 = { workspace =
true }" from the Cargo.toml to clean up the manifest; locate the commented
reference to borsh10 and delete that line so the crate manifest no longer
contains leftover scaffolded dependency comments.
In `@compressed-delegation-api/src/instruction.rs`:
- Around line 36-37: The doc comment for the enum variant InitDelegationRecord
is incorrect; update the documentation above the InitDelegationRecord variant in
instruction.rs to accurately describe its purpose (e.g., "Initialize a
delegation record for a compressed account" or similar), so the comment matches
the variant name and behavior; ensure the new text references
InitDelegationRecord and mentions it initializes the delegation record rather
than "Delegate the compressed account to a validator."
In `@magicblock-api/src/magic_validator.rs`:
- Around line 291-297: The code only ever sets the process-wide compression flag
to true via validator::set_compression_enabled(true) which leaves compression
enabled across subsequent MagicValidator::try_from_config() runs; change the
logic to explicitly set the flag to the boolean presence of
config.compression.as_ref().and_then(|c| c.photon_url.as_ref()).is_some() (i.e.,
call validator::set_compression_enabled(...) with true when photon_url is
present and false otherwise) so the global state is reset correctly on every
startup and when config.compression or photon_url is absent.
In `@magicblock-chainlink/src/chainlink/errors.rs`:
- Around line 51-52: The new error variant
FailedToDeserializeCompressedDelegationRecord is never returned because the code
that calls CompressedDelegationRecord::try_from_slice currently logs and skips
deserialization failures; change that flow to propagate the error instead:
replace the silent log-and-continue in the fetch_cloner compression handling
with returning
Err(chainlink::errors::Error::FailedToDeserializeCompressedDelegationRecord(e))
(or use the ? operator where appropriate) so deserialization failures bubble up,
and adjust callers in the fetch/cloner path to handle/propagate this error
instead of ignoring it.
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Line 1121: The debug! call currently logs full rpc_accounts and
photon_accounts payloads; replace it with a smaller summary that logs counts and
relevant slot info instead. In the block where rpc_accounts and photon_accounts
are produced (the debug! invocation using rpc_accounts and photon_accounts in
mod.rs), change the log to include rpc_accounts.len() and photon_accounts.len()
and optional slot summaries (e.g., min/max or first/last slot values derived
from the account entries) rather than the full vectors so logs remain compact
and informative.
- Around line 1131-1148: The match arm that currently treats all Err(err) from
the results loop (producing logs like error!("Failed to fetch accounts:
{err:?}")) needs to special-case the CompressionNotEnabled error from
fetch_from_photon: detect if the error is CompressionNotEnabled and either
ignore it or log at debug/trace level instead of error, and only call error! for
genuine failures; update the same handling at the other occurrence around the
FetchedRemoteAccounts aggregation (the block handling results and the similar
block at lines ~1469-1471) so CompressionNotEnabled no longer floods logs.
- Around line 1123-1138: The RPC success metric
inc_account_fetches_success(pubkeys.len() as u64) is being called
unconditionally after merging results and thus records RPC successes even when
rpc_accounts failed; update the consolidation logic in the loop that processes
results (the results vec of rpc_accounts and photon_accounts and match arms for
FetchedRemoteAccounts::Rpc and ::Photon) so that you only call
inc_account_fetches_success(...) when the matched variant is
Ok((FetchedRemoteAccounts::Rpc(_), _, _)) and similarly only call
inc_account_fetches_not_found/other RPC-specific metric updates when the
rpc_accounts match arm produced data; in short, move/guard the metric increments
to the branches that confirm rpc_accounts succeeded (refer to
inc_account_fetches_success, inc_account_fetches_not_found,
FetchedRemoteAccounts::Rpc, rpc_accounts, photon_accounts, and the for result in
results loop).
In `@magicblock-chainlink/src/remote_account_provider/photon_client.rs`:
- Around line 102-114: The function account_from_compressed_account currently
uses compressed_acc.data.unwrap_or_default() which fabricates an empty payload
and returns an Account with zero-length data; instead, treat missing compressed
payloads as absent by returning None when compressed_acc.data is None. Modify
account_from_compressed_account to early-return None if
compressed_acc.data.is_none(), and only construct and return Some(Account { ...
data: compressed_acc.data.unwrap().data, ... }) when the payload exists
(preserve the existing owner/executable/rent_epoch/lamports logic). This ensures
CompressedAccount missing payloads do not become empty Accounts.
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Around line 443-521: In get_compressed_data validate the
compressed_delegation_record.data bytes before packaging by attempting the same
deserialization/parsing that commit_finalize_compressed_task.rs performs (the
code that currently calls .expect(...)) and map any deserialization failure to a
typed TaskInfoFetcherError (e.g. add TaskInfoFetcherError::InvalidCompressedData
if missing). Specifically, in get_compressed_data (and when constructing
CompressedData) try to parse compressed_delegation_record.data.ok_or(...)? .data
into the expected compressed-delegation struct used downstream; on error return
Err(TaskInfoFetcherError::InvalidCompressedData(...)) instead of returning raw
bytes.
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 550-561: The bundle accounting now includes compressed finalize
intents via get_commit_finalize_compressed_intent_accounts and
get_commit_finalize_compressed_and_undelegate_intent_accounts but calculate_fee
still only accounts for commit and commit_and_undelegate, letting compressed
bundles be underbilled; update calculate_fee (and any helper it uses) to treat
the compressed intent kinds returned by
get_commit_finalize_compressed_intent_accounts and
get_commit_finalize_compressed_and_undelegate_intent_accounts the same as
commit_finalize and commit_finalize_and_undelegate (i.e., map compressed
finalize paths to the same fee buckets or add matching match-arms/branches) so
compressed finalize bundles are charged the same fees as their uncompressed
equivalents.
In `@test-integration/programs/flexi-counter/src/processor.rs`:
- Around line 733-738: The CPI account metas currently mark magic_program
writable; change the account meta construction in account_metas so the program
account is readonly by using AccountMeta::new_readonly (or AccountMeta::new with
is_signer=false and is_writable=false) for the program entry and reference the
constant program id (MAGIC_PROGRAM_ID or the program_id constant) instead of
dereferencing magic_program.key; update the entry that currently uses
AccountMeta::new(*magic_program.key, false) to use
AccountMeta::new_readonly(MAGIC_PROGRAM_ID, false) (or equivalent) so the
program account is correctly readonly and uses the constant identity.
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 1156-1158: The Photon client is being pointed at the RPC port;
update the PhotonIndexer instantiation to use the Photon service port used by
the compressed setup helper (http://localhost:8784) instead of the RPC
URL—change the PhotonIndexer::new call so it constructs with
"http://localhost:8784" while leaving RpcClient::new("http://localhost:7799")
unchanged.
In `@test-integration/test-runner/src/cleanup.rs`:
- Around line 3-9: The helper cleanup_validators currently always calls
cleanup_light_validator which breaks chain-based runs; change
cleanup_validators(ephem_validator: &mut Child, light_validator: &mut Child) to
accept a third parameter uses_light_validator: bool, keep the existing call
cleanup_validator(ephem_validator, "ephemeral"), and then: if
uses_light_validator { call cleanup_light_validator(light_validator) } else {
call cleanup_validator(light_validator, "<appropriate non-light label>") } ;
preserve the final kill_validators() call and update all call sites to pass
uses_light_validator = true only when the caller’s cluster type is
ValidatorCluster::Light.
---
Outside diff comments:
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 224-236: The traversal helpers for MagicIntentBundle currently
ignore the new compressed variants so callbacks/actions on
CommitFinalizeCompressed and CommitFinalizeAndUndelegateCompressed never get
visited; update the code paths that iterate/inspect actions (including the
functions that build/collect callbacks and the get_action_mut method) to include
the fields commit_finalize_compressed and
commit_finalize_compressed_and_undelegate alongside their uncompressed
counterparts (commit_finalize, commit_finalize_and_undelegate) so that
WithBaseActions carried by those variants are traversed and mutable access via
get_action_mut covers compressed intents as well.
---
Duplicate comments:
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Around line 94-106: get_compressed_data_for_accounts currently swallows all
errors from get_compressed_data by calling .ok(), turning failures into None;
change the inner async closure so errors are propagated instead of converted to
None (i.e., return the Result<Option<CompressedData>, _> from
get_compressed_data directly so try_join_all can short-circuit on Err).
Concretely, remove the .ok() and any wrapping that converts Result to Option,
using the await'ed Result from get_compressed_data(pubkey,
min_context_slot).await (or the ? operator inside the closure) so
get_compressed_data_for_accounts returns Err for real fetch failures and only
yields None when get_compressed_data succeeds but returns None.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 155-176: The helper fetch_compressed_data currently drops None
results and returns a sparse HashMap, which delays the error until later and can
leave persisted commit IDs orphaned; modify fetch_compressed_data (the async fn
fetch_compressed_data<C: TaskInfoFetcher>) to call
task_info_fetcher.get_compressed_data_for_accounts(&pubkeys,
Some(min_context_slot)).await?, inspect the returned Vec<Option<CompressedData>>
for any None entries, and if any are missing return an Err (with context
including the missing Pubkeys) instead of filtering them out; only build and
return the HashMap when all pubkeys have Some(CompressedData).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e183c58f-c87c-48e8-b659-50331b02f0d0
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
compressed-delegation-api/Cargo.tomlcompressed-delegation-api/src/instruction.rscompressed-delegation-client/Cargo.tomlcompressed-delegation-client/src/builders/commit_finalize.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/photon_client.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/task_builder.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-runner/bin/run_tests.rstest-integration/test-runner/src/cleanup.rs
thlorenz
requested changes
May 5, 2026
Collaborator
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
Nice work on the compressed delegation flow!
I found multiple blocking issues which should be addressed before merging.
| ExecuteCrank { instructions } => { | ||
| process_execute_crank(signers, invoke_context, instructions) | ||
| } | ||
| ScheduleCommitCompressed => process_schedule_commit_finalize( |
Collaborator
There was a problem hiding this comment.
These compressed variants go straight into process_schedule_commit_finalize, but that path doesn't check is_compression_enabled().
The guard only exists in process_schedule_commit, so with compression disabled a validator can still accept ScheduleCommitCompressed and ScheduleCommitAndUndelegateCompressed and enqueue compressed intents.
That bypasses the new toggle.
Can we add the same opts.compressed && !is_compression_enabled() check in the finalize path,
or route these variants through code that does the guard before scheduling?
| Ok(self | ||
| .get_compressed_data(pubkey, min_context_slot) | ||
| .await | ||
| .ok()) |
Collaborator
There was a problem hiding this comment.
This helper is currently turning every get_compressed_data failure into None via .ok().
That hides retriable cases like MinContextSlotNotReached, Photon/indexer failures, or config errors, then TaskBuilderImpl::fetch_compressed_data drops the missing entry and later reports a generic compressed data absent IO error.
At that point we've lost the real cause and any signature/retry context the executor could use.
For compressed commit-finalize intents, can we preserve and propagate the original TaskInfoFetcherError here instead of collapsing it to Option?
| compressed_found_count += fc; | ||
| compressed_not_found_count += nfc; | ||
| } | ||
| Err(ChainlinkError::CompressionNotEnabled) => { |
Collaborator
There was a problem hiding this comment.
Disabled compression is modeled as an error path which is a bad pattern IMO:
fetch() always calls fetch_from_photon(...), even when self.photon_client is None, the
helper then returns Err(ChainlinkError::CompressionNotEnabled) immediately and the caller
treats that error as expected control flow.
IMHO if compression is disabled, we should not enter the
compressed fetch branch at all.
Branching before the tokio::join! would make the disabled
case explicit and keep it separate from real Photon/indexer failures.
| continue; | ||
| } | ||
| Err(err) => { | ||
| error!("Failed to fetch accounts: {err:?}"); |
Collaborator
There was a problem hiding this comment.
Real Photon failures are swallowed when compression is enabled here.
When Photon is configured but fetch_from_photon actually fails, this just logs
the error and continues with only the RPC results.
For compressed-only accounts, RPC can legitimately return NotFound, however after this branch
consolidate_fetched_remote_accounts returns the RPC NotFound,
and the fetch cloner treats the account as absent instead of surfacing the Photon issues or retrying.
| // We encountered error "Max instruction trace length exceeded" | ||
| // All the tasks a prepared to be executed at this point | ||
| // We attempt Two stages commit flow, need to split tasks up | ||
| let last_commit_ind = strategy.optimized_tasks.iter().rposition(|el| { |
Collaborator
There was a problem hiding this comment.
handle_cpi_limit_error() also needs to classify BaseTaskImpl::CommitFinalizeCompressed(_) as commit-stage work.
Right now a single-stage strategy with only compressed commit-finalize tasks gets
last_commit_ind == None, so the split puts every task into finalize_stage_tasks and leaves
commit_stage_tasks empty.
I'm pretty sure that breaks CPI-limit recovery for the new compressed flow, especially when the
transaction also has undelegation or follow-up action tasks.
Suggested change
| let last_commit_ind = strategy.optimized_tasks.iter().rposition(|el| { | |
| matches!( | |
| el, | |
| BaseTaskImpl::Commit(_) | |
| | BaseTaskImpl::CommitFinalize(_) | |
| | BaseTaskImpl::CommitFinalizeCompressed(_) | |
| ) | |
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Supersedes #843
Summary by CodeRabbit
New Features
Documentation
Chores