feat: check risk strategies - #1429
Conversation
|
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:
📝 WalkthroughWalkthroughAdds configurable AML risk checking with Assessment against linked issues:
Assessment against linked issues: Out-of-scope changes:
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 |
65d1e07 to
5104c59
Compare
7f9ba49 to
8a4f601
Compare
5104c59 to
b7addc0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 1577-1582: The signer collection in lines 1584-1602 currently
includes signers from all delegation_actions regardless of the check_strategy
result, which defeats the risk-check optimization. After the
delegation_actions_require_risk_check guard, add conditional logic based on
risk_service.check_strategy(): when the strategy indicates RelevantPrograms,
filter the actions with instruction_involves_risk_relevant_program before
collecting signers; when the strategy indicates AllSigners, preserve the current
behavior of collecting from all actions. Also add a test case that validates the
filtering behavior by including a mixed batch with both relevant and unrelated
actions to ensure only relevant signers are sent to check_addresses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 37aea70b-9b7d-4458-a5a8-acf7df61fc2d
📒 Files selected for processing (7)
config.example.tomlmagicblock-aml/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/tests/10_aml_undelegation.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/config/mod.rs
39529ab to
fc4f0d6
Compare
b7addc0 to
4de6e54
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4de6e54 to
f269963
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
f269963 to
15ec769
Compare
835518e to
ec9b7ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/context/crates/magicblock-task-scheduler.md:
- Around line 75-78: Rewrite the entire guide to match the current stateless
faucet-funded Hydra crank implementation. Remove references to the nonexistent
db module, SchedulerDatabase, DbTask, SQLite, migrate_persisted_tasks,
DelayQueue, retry state, failed-record cleanup, and the obsolete
TaskSchedulerService::new(path, ...) API; instead document only the exported
crank, errors, and service modules and the behavior implemented by
TaskSchedulerService and its current runtime flow.
- Around line 75-78: Update the relevant change log or task note in
.agents/AGENTS.md to record the addition or modification of
magicblock-task-scheduler.md, preserving the existing weekly documentation
maintenance path and surrounding entries.
In `@bins/magicblock-validator/src/crank_faucet.rs`:
- Around line 63-76: Update the faucet-account handling around faucet_account to
log faucet_account.lamports at info level, and emit a warning when the balance
is below the minimum required to fund one crank. Preserve the existing skip
behavior for a missing account and continue delegation for balances that meet
the threshold.
- Around line 56-79: Replace direct indexing of the get_multiple_accounts result
in the faucet delegation flow with slice destructuring that safely handles
truncated responses and returns FailedToDelegateFaucet for malformed lengths.
Use faucet_account_opt.as_ref() and delegation_record_opt.as_ref() in the
existing account checks, preserving the current delegation behavior for valid
responses.
In `@bins/magicblock-validator/src/leader.rs`:
- Around line 539-567: The scheduler currently starts before the detached faucet
setup completes, allowing requests to reach TaskSchedulerService prematurely.
Update the startup flow around the faucet setup block and scheduler start so
scheduler processing begins only after ensure_faucet_delegated_on_chain
succeeds, or otherwise gate TaskSchedulerService request handling until faucet
delegation is confirmed with retry behavior.
In `@magicblock-aml/src/lib.rs`:
- Around line 132-146: Update assess_address to retry transient transport errors
and HTTP 5xx responses using a bounded backoff before returning failure, while
preserving the existing request and deserialization behavior. Do not retry
definitive responses or successful RiskAssessment values; continue propagating
final errors so the existing fail-closed handling remains unchanged.
In `@magicblock-config/src/tests.rs`:
- Around line 108-131: Update the risk_configuration_loads_from_toml test
fixture to use the HTTPS risk-server-url expected by
RiskService::try_from_config, while preserving the enabled flag and existing
timeout assertions.
In `@magicblock-task-scheduler/src/errors.rs`:
- Around line 25-30: Remove the unused FaucetNotReady and legacy
TaskSchedulerError variants from errors.rs, retaining only variants constructed
by the Hydra task-scheduler path. Update the TaskSchedulerError definition
without changing the existing Engine error conversion.
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 137-142: Update the invalid-schedule and non-positive-iterations
early-return paths in the task scheduling flow to emit a warn! containing the
task ID before returning Ok(()). Remove the redundant clamp around
task.execution_interval_millis and use the validated interval directly,
preserving the existing validation behavior.
- Around line 265-282: Update the submit/process_request flow so failed task
submissions are not silently discarded: add bounded retries with backoff around
transaction creation and scheduling, or persist failed requests for later retry.
Preserve the existing TaskSchedulerError propagation after retries are
exhausted, and document the behavior in the crate documentation if task dropping
is intentionally accepted.
- Around line 285-321: Extend the tests module around test_service with coverage
for the new crank paths: assert send_create builds the expected instruction list
for both a new crank and an existing crank, including the prepended cancel
instruction on reschedule. Add assertions for interval-to-slot conversion and
deterministic PDA derivation, and verify funding equals iterations multiplied by
CRANKER_REWARD plus crank_rent_floor(instructions).
- Around line 157-169: Update process_cancel_request to call the existing
crank_exists check for the derived crank before invoking send_cancel. If the
crank is absent, return Ok(()) without submitting a transaction or logging
cancellation; otherwise preserve the current send_cancel and success behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef494625-57db-4d98-85aa-c736afb02c3d
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/programs/hydra/hydra.sois excluded by!**/*.so
📒 Files selected for processing (48)
.agents/context/crates/magicblock-task-scheduler.mdCargo.tomlbins/magicblock-validator/Cargo.tomlbins/magicblock-validator/src/crank_faucet.rsbins/magicblock-validator/src/errors.rsbins/magicblock-validator/src/leader.rsbins/magicblock-validator/src/main.rsconfig.example.tomlmagicblock-aml/Cargo.tomlmagicblock-aml/src/lib.rsmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/tests/10_aml_undelegation.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/config/mod.rsmagicblock-config/src/config/scheduler.rsmagicblock-config/src/consts.rsmagicblock-config/src/lib.rsmagicblock-config/src/tests.rsmagicblock-config/src/types/crypto.rsmagicblock-config/src/types/mod.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-magic-program-api/src/lib.rsmagicblock-magic-program-api/src/pda.rsmagicblock-runtime/src/lib.rsmagicblock-task-scheduler/Cargo.tomlmagicblock-task-scheduler/src/crank.rsmagicblock-task-scheduler/src/db.rsmagicblock-task-scheduler/src/errors.rsmagicblock-task-scheduler/src/lib.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/ephemeral_accounts/mod.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_task/mod.rsprograms/magicblock/src/schedule_task/process_execute_task.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/Cargo.tomltest-integration/configs/schedule-task.devnet.tomltest-integration/test-aml/src/lib.rstest-integration/test-aml/tests/range_mock.rstest-integration/test-task-scheduler/Cargo.tomltest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rstest-integration/test-task-scheduler/tests/test_schedule_task.rstest-integration/test-task-scheduler/tests/test_undrained_faucet.rs
💤 Files with no reviewable changes (5)
- magicblock-magic-program-api/src/pda.rs
- magicblock-magic-program-api/src/instruction.rs
- programs/magicblock/src/schedule_task/process_execute_task.rs
- test-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rs
- magicblock-task-scheduler/src/db.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/context/crates/magicblock-task-scheduler.md:
- Around line 75-78: Rewrite the entire guide to match the current stateless
faucet-funded Hydra crank implementation. Remove references to the nonexistent
db module, SchedulerDatabase, DbTask, SQLite, migrate_persisted_tasks,
DelayQueue, retry state, failed-record cleanup, and the obsolete
TaskSchedulerService::new(path, ...) API; instead document only the exported
crank, errors, and service modules and the behavior implemented by
TaskSchedulerService and its current runtime flow.
- Around line 75-78: Update the relevant change log or task note in
.agents/AGENTS.md to record the addition or modification of
magicblock-task-scheduler.md, preserving the existing weekly documentation
maintenance path and surrounding entries.
In `@bins/magicblock-validator/src/crank_faucet.rs`:
- Around line 63-76: Update the faucet-account handling around faucet_account to
log faucet_account.lamports at info level, and emit a warning when the balance
is below the minimum required to fund one crank. Preserve the existing skip
behavior for a missing account and continue delegation for balances that meet
the threshold.
- Around line 56-79: Replace direct indexing of the get_multiple_accounts result
in the faucet delegation flow with slice destructuring that safely handles
truncated responses and returns FailedToDelegateFaucet for malformed lengths.
Use faucet_account_opt.as_ref() and delegation_record_opt.as_ref() in the
existing account checks, preserving the current delegation behavior for valid
responses.
In `@bins/magicblock-validator/src/leader.rs`:
- Around line 539-567: The scheduler currently starts before the detached faucet
setup completes, allowing requests to reach TaskSchedulerService prematurely.
Update the startup flow around the faucet setup block and scheduler start so
scheduler processing begins only after ensure_faucet_delegated_on_chain
succeeds, or otherwise gate TaskSchedulerService request handling until faucet
delegation is confirmed with retry behavior.
In `@magicblock-aml/src/lib.rs`:
- Around line 132-146: Update assess_address to retry transient transport errors
and HTTP 5xx responses using a bounded backoff before returning failure, while
preserving the existing request and deserialization behavior. Do not retry
definitive responses or successful RiskAssessment values; continue propagating
final errors so the existing fail-closed handling remains unchanged.
In `@magicblock-config/src/tests.rs`:
- Around line 108-131: Update the risk_configuration_loads_from_toml test
fixture to use the HTTPS risk-server-url expected by
RiskService::try_from_config, while preserving the enabled flag and existing
timeout assertions.
In `@magicblock-task-scheduler/src/errors.rs`:
- Around line 25-30: Remove the unused FaucetNotReady and legacy
TaskSchedulerError variants from errors.rs, retaining only variants constructed
by the Hydra task-scheduler path. Update the TaskSchedulerError definition
without changing the existing Engine error conversion.
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 137-142: Update the invalid-schedule and non-positive-iterations
early-return paths in the task scheduling flow to emit a warn! containing the
task ID before returning Ok(()). Remove the redundant clamp around
task.execution_interval_millis and use the validated interval directly,
preserving the existing validation behavior.
- Around line 265-282: Update the submit/process_request flow so failed task
submissions are not silently discarded: add bounded retries with backoff around
transaction creation and scheduling, or persist failed requests for later retry.
Preserve the existing TaskSchedulerError propagation after retries are
exhausted, and document the behavior in the crate documentation if task dropping
is intentionally accepted.
- Around line 285-321: Extend the tests module around test_service with coverage
for the new crank paths: assert send_create builds the expected instruction list
for both a new crank and an existing crank, including the prepended cancel
instruction on reschedule. Add assertions for interval-to-slot conversion and
deterministic PDA derivation, and verify funding equals iterations multiplied by
CRANKER_REWARD plus crank_rent_floor(instructions).
- Around line 157-169: Update process_cancel_request to call the existing
crank_exists check for the derived crank before invoking send_cancel. If the
crank is absent, return Ok(()) without submitting a transaction or logging
cancellation; otherwise preserve the current send_cancel and success behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef494625-57db-4d98-85aa-c736afb02c3d
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/programs/hydra/hydra.sois excluded by!**/*.so
📒 Files selected for processing (48)
.agents/context/crates/magicblock-task-scheduler.mdCargo.tomlbins/magicblock-validator/Cargo.tomlbins/magicblock-validator/src/crank_faucet.rsbins/magicblock-validator/src/errors.rsbins/magicblock-validator/src/leader.rsbins/magicblock-validator/src/main.rsconfig.example.tomlmagicblock-aml/Cargo.tomlmagicblock-aml/src/lib.rsmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/tests/10_aml_undelegation.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/config/mod.rsmagicblock-config/src/config/scheduler.rsmagicblock-config/src/consts.rsmagicblock-config/src/lib.rsmagicblock-config/src/tests.rsmagicblock-config/src/types/crypto.rsmagicblock-config/src/types/mod.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-magic-program-api/src/lib.rsmagicblock-magic-program-api/src/pda.rsmagicblock-runtime/src/lib.rsmagicblock-task-scheduler/Cargo.tomlmagicblock-task-scheduler/src/crank.rsmagicblock-task-scheduler/src/db.rsmagicblock-task-scheduler/src/errors.rsmagicblock-task-scheduler/src/lib.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/ephemeral_accounts/mod.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_task/mod.rsprograms/magicblock/src/schedule_task/process_execute_task.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/Cargo.tomltest-integration/configs/schedule-task.devnet.tomltest-integration/test-aml/src/lib.rstest-integration/test-aml/tests/range_mock.rstest-integration/test-task-scheduler/Cargo.tomltest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rstest-integration/test-task-scheduler/tests/test_schedule_task.rstest-integration/test-task-scheduler/tests/test_undrained_faucet.rs
💤 Files with no reviewable changes (5)
- magicblock-magic-program-api/src/pda.rs
- magicblock-magic-program-api/src/instruction.rs
- programs/magicblock/src/schedule_task/process_execute_task.rs
- test-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rs
- magicblock-task-scheduler/src/db.rs
🛑 Comments failed to post (11)
.agents/context/crates/magicblock-task-scheduler.md (1)
75-78: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The documented database and migration API no longer exists in the crate.
magicblock-task-scheduler/src/lib.rsin this same change exports onlycrank,errors, andservice. Thedbmodule,SchedulerDatabase, andDbTaskare gone, andmagicblock-task-scheduler/Cargo.tomlno longer lists a SQLite dependency.magicblock-task-scheduler/src/service.rscontains nomigrate_persisted_tasks, noDelayQueue, no retry state, and no failed-record cleanup.These changed lines describe SQLite migration APIs as present. Line 117 describes a startup migration flow that no code implements. The surrounding unchanged sections — the crate exports at Lines 63-66, the
TaskSchedulerService::new(path, config, rpc_url, ...)signature at Line 82, the errors section at Line 101, and the runtime-flow sections through Line 171 — are also stale.This guide is the document the coding guidelines require an agent to read before changing this crate. Rewrite it to describe the stateless faucet-funded Hydra crank service, or the next change based on it will be wrong.
As per coding guidelines: "Before changing code, consult the matching
.agentsdocumentation so the change does not violate validator goals, invariants, performance requirements, or specifications."Also applies to: 117-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/context/crates/magicblock-task-scheduler.md around lines 75 - 78, Rewrite the entire guide to match the current stateless faucet-funded Hydra crank implementation. Remove references to the nonexistent db module, SchedulerDatabase, DbTask, SQLite, migrate_persisted_tasks, DelayQueue, retry state, failed-record cleanup, and the obsolete TaskSchedulerService::new(path, ...) API; instead document only the exported crank, errors, and service modules and the behavior implemented by TaskSchedulerService and its current runtime flow.Source: Coding guidelines
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd -t f 'AGENTS.md' -H echo "--- references to the task scheduler guide ---" rg -n 'magicblock-task-scheduler' AGENTS.md 2>/dev/null echo "--- changed files in this PR ---" git diff --name-only origin/HEAD...HEAD 2>/dev/null | rg 'AGENTS|\.agents' || echo "no AGENTS.md/.agents entries found in range"Repository: magicblock-labs/magicblock-validator
Length of output: 1734
🏁 Script executed:
#!/bin/bash set -u echo "--- AGENTS.md file existence and references ---" test -f AGENTS.md && echo "AGENTS.md exists" || echo "AGENTS.md missing" rg -n 'AGENTS|magicblock-task-scheduler|task scheduler|\.agents|context/crates/magicblock-task-scheduler' AGENTS.md .agents 2>/dev/null || true echo echo "--- git diff names across all refs if origin/HEAD unavailable ---" git diff --name-only HEAD 2>/dev/null | rg '(^AGENTS\.md$|\.agents/)' || true git diff --name-only --cached 2>/dev/null | rg '(^AGENTS\.md$|\.agents/)' || true git diff --name-only HEAD~1..HEAD 2>/dev/null | rg '(^AGENTS\.md$|\.agents/)' || true echo echo "--- AGENTS.md size and first 120 lines if present ---" wc -l AGENTS.md 2>/dev/null || true sed -n '1,120p' AGENTS.md 2>/dev/null || true echo echo "--- task scheduler changed diff stat and first 120 relevant lines ---" git diff --stat -- .agents/context/crates/magicblock-task-scheduler.md 2>/dev/null || true git diff -- .agents/context/crates/magicblock-task-scheduler.md 2>/dev/null | sed -n '1,180p' || trueRepository: magicblock-labs/magicblock-validator
Length of output: 41920
Update
AGENTS.mdin the same.agents/change.
magicblock-task-scheduler.mdis modified in this change, butAGENTS.mdis not updated to document this.agents/change. The existing entry already points agents to the weekly documentation maintenance path; update the change log/task note to include this guide update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/context/crates/magicblock-task-scheduler.md around lines 75 - 78, Update the relevant change log or task note in .agents/AGENTS.md to record the addition or modification of magicblock-task-scheduler.md, preserving the existing weekly documentation maintenance path and surrounding entries.Source: Coding guidelines
bins/magicblock-validator/src/crank_faucet.rs (2)
56-79: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not index the
get_multiple_accountsresult directly.
accounts[0]at Line 69 andaccounts[1]at Line 79 assume the RPC returned exactly two entries.solana_rpc_clientdoes not verify that the response length matches the request length, so a truncated or malformed response panics this task. The task runs insidespawn_primary_onchain_setup, so a panic there aborts validator startup setup without the intendedFailedToDelegateFaucetdiagnostic.Destructure the slice instead.
🛠️ Proposed fix
- let accounts = rpc + let accounts = rpc .get_multiple_accounts(&[faucet_pubkey, delegation_record_pubkey]) .await .map_err(|err| { ApiError::FailedToDelegateFaucet(faucet_pubkey, err.to_string()) })?; + let [faucet_account_opt, delegation_record_opt] = accounts.as_slice() + else { + return Err(ApiError::FailedToDelegateFaucet( + faucet_pubkey, + format!( + "expected 2 accounts from get_multiple_accounts, got {}", + accounts.len() + ), + )); + };Then use
faucet_account_opt.as_ref()at Line 69 anddelegation_record_opt.as_ref()at Line 79.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bins/magicblock-validator/src/crank_faucet.rs` around lines 56 - 79, Replace direct indexing of the get_multiple_accounts result in the faucet delegation flow with slice destructuring that safely handles truncated responses and returns FailedToDelegateFaucet for malformed lengths. Use faucet_account_opt.as_ref() and delegation_record_opt.as_ref() in the existing account checks, preserving the current delegation behavior for valid responses.
63-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Report the faucet balance so operators can diagnose unfunded faucets.
The function only checks that the faucet account exists. A faucet that exists but holds too few lamports passes delegation and then fails at every crank creation.
magicblock-task-scheduler/src/service.rsonly logs those failures, so the root cause stays invisible. Logfaucet_account.lamportsatinfolevel here, and warn when it cannot cover at least one crank.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bins/magicblock-validator/src/crank_faucet.rs` around lines 63 - 76, Update the faucet-account handling around faucet_account to log faucet_account.lamports at info level, and emit a warning when the balance is below the minimum required to fund one crank. Preserve the existing skip behavior for a missing account and continue delegation for balances that meet the threshold.bins/magicblock-validator/src/leader.rs (1)
539-567: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Gate scheduler processing on faucet readiness.
The detached setup task delegates the faucet after
start()can launch the scheduler. A schedule request can therefore reachTaskSchedulerServicebefore the faucet exists in the ephemeral rollup. The service logs a failed request and does not retry it.Complete faucet setup before starting the scheduler, or make scheduler request processing wait and retry until the faucet is delegated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bins/magicblock-validator/src/leader.rs` around lines 539 - 567, The scheduler currently starts before the detached faucet setup completes, allowing requests to reach TaskSchedulerService prematurely. Update the startup flow around the faucet setup block and scheduler start so scheduler processing begins only after ensure_faucet_delegated_on_chain succeeds, or otherwise gate TaskSchedulerService request handling until faucet delegation is confirmed with retry behavior.magicblock-aml/src/lib.rs (1)
132-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider a retry for transient risk-server failures.
assess_addresspropagates every request error, including a connect error, a timeout, and a 5xx status. Inmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs(lines 895-898) that error is treated the same as a high-risk signer, so the delegated account is cloned and then scheduled for undelegation. The previous implementation absorbed some of these failures through its local cache, which this change removes.A single transient failure of the local risk server now undelegates a legitimate account. Add a bounded retry with backoff for transport-level and 5xx failures, and keep the fail-closed behavior for a definitive risky verdict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-aml/src/lib.rs` around lines 132 - 146, Update assess_address to retry transient transport errors and HTTP 5xx responses using a bounded backoff before returning failure, while preserving the existing request and deserialization behavior. Do not retry definitive responses or successful RiskAssessment values; continue propagating final errors so the existing fail-closed handling remains unchanged.magicblock-config/src/tests.rs (1)
108-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a URL accepted by the risk client.
Line 115 configures enabled risk checks with remote plaintext HTTP.
RiskService::try_from_configrejects this URL, so this fixture does not represent a valid enabled-risk configuration. Usehttps://risk.example:3001instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-config/src/tests.rs` around lines 108 - 131, Update the risk_configuration_loads_from_toml test fixture to use the HTTPS risk-server-url expected by RiskService::try_from_config, while preserving the enabled flag and existing timeout assertions.magicblock-task-scheduler/src/errors.rs (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash for v in FaucetNotReady InvalidConfiguration TransactionExecution UnauthorizedReplacing SizeMismatch Wincode; do echo "== $v" rg -n "TaskSchedulerError::$v|$v\(" --type=rust -g '!**/errors.rs' doneRepository: magicblock-labs/magicblock-validator
Length of output: 292
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Repository files of interest ==" git ls-files | rg '(^|/)(errors|service)\.rs$|magicblock-task-scheduler|engine|errors' | sed -n '1,200p' echo echo "== errors.rs outline ==" ast-grep outline magicblock-task-scheduler/src/errors.rs || true echo echo "== errors.rs content ==" cat -n magicblock-task-scheduler/src/errors.rs echo echo "== search TaskSchedulerError variant names outside errors.rs ==" for v in FaucetNotReady InvalidConfiguration TransactionExecution UnauthorizedReplacing SizeMismatch Wincode; do echo "== $v ==" rg -n "TaskSchedulerError::$v|\b$v\b|\b(v|enum TaskSchedulerError|TaskSchedulerError)" --type=rust -g '!**/errors.rs' | sed -n '1,200p' || true done echo echo "== inspect service.rs relevant sections ==" wc -l magicblock-task-scheduler/src/service.rs sed -n '1,260p' magicblock-task-scheduler/src/service.rsRepository: magicblock-labs/magicblock-validator
Length of output: 2444
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== task scheduler files ==" wc -l magicblock-task-scheduler/src/*.rs echo for f in magicblock-task-scheduler/src/lib.rs magicblock-task-scheduler/src/crank.rs magicblock-task-scheduler/src/service.rs; do echo "== $f ==" sed -n '1,260p' "$f" done echo echo "== exact TaskSchedulerError variant constructor occurrences outside errors.rs ==" python3 - <<'PY' import re, subprocess patterns = ["TaskSchedulerError::FaucetNotReady", "InvalidConfiguration(", "TransactionExecution(", "UnauthorizedReplacing(", "SizeMismatch(", "TaskSchedulerError::Wincode", "Engine("] for pat in patterns: p = subprocess.run(["rg", "-n", re.escape(pat), "--type=rust", "-g", "!**/errors.rs"], text=True, capture_output=True) print(f"-- {pat} --") print(p.stdout or "") print("exit", p.returncode) PYRepository: magicblock-labs/magicblock-validator
Length of output: 16344
Remove unused
TaskSchedulerErrorvariants.
FaucetNotReadyand the legacy variants are only declared; no task-scheduler code outsideerrors.rsconstructs them. Keep only the errors the Hydra path returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/errors.rs` around lines 25 - 30, Remove the unused FaucetNotReady and legacy TaskSchedulerError variants from errors.rs, retaining only variants constructed by the Hydra task-scheduler path. Update the TaskSchedulerError definition without changing the existing Engine error conversion.magicblock-task-scheduler/src/service.rs (4)
137-142: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the silently discarded schedule requests.
Two paths return
Ok(())without any record. Line 137 drops the request when the interval is invalid. Line 182 drops it wheniterations <= 0. A later cancel for the same task then fails, and the operator has no log line explaining why the task never ran. Emit awarn!with the task ID in both paths.Note also that
clamp(1, u32::MAX as i64)at Line 141 cannot change the value:is_valid_task_intervalalready requires0 < interval < u32::MAX.🛠️ Proposed logging
if !is_valid_task_interval(task.execution_interval_millis) { - // Too large or zero: ignore. + warn!( + "Ignoring task {}: interval {}ms is out of range", + task.id, task.execution_interval_millis + ); return Ok(()); } - let interval_millis = - task.execution_interval_millis.clamp(1, u32::MAX as i64); + let interval_millis = task.execution_interval_millis;if iterations <= 0 { + warn!("Ignoring task {task_id}: iterations is {iterations}"); return Ok(()); }Also applies to: 182-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 137 - 142, Update the invalid-schedule and non-positive-iterations early-return paths in the task scheduling flow to emit a warn! containing the task ID before returning Ok(()). Remove the redundant clamp around task.execution_interval_millis and use the validated interval directly, preserving the existing validation behavior.
157-169: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the cancel path with
crank_exists.
process_cancel_requestalways submits a cancel instruction. The file already containscrank_existsat Line 197. A cancel for a task whose crank was never created — for example a schedule request thatprocess_schedule_requestskipped becauseiterations <= 0or the interval was invalid — produces a failed transaction and an error log for a request the program treated as valid.Check existence first and return
Ok(())when no crank is present.🛠️ Proposed guard
let crank = crank_pubkey(&cancel_request.authority, cancel_request.task_id); - // Does not check if the crank exists, so it will fail if it does not exist + if !self.crank_exists(&crank) { + debug!( + "No hydra crank for task {}, nothing to cancel", + cancel_request.task_id + ); + return Ok(()); + } self.send_cancel(crank).await?;📝 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.async fn process_cancel_request( &self, cancel_request: &CancelTaskRequest, ) -> TaskSchedulerResult<()> { let crank = crank_pubkey(&cancel_request.authority, cancel_request.task_id); if !self.crank_exists(&crank) { debug!( "No hydra crank for task {}, nothing to cancel", cancel_request.task_id ); return Ok(()); } self.send_cancel(crank).await?; debug!("Cancelled hydra crank for task {}", cancel_request.task_id); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 157 - 169, Update process_cancel_request to call the existing crank_exists check for the derived crank before invoking send_cancel. If the crank is absent, return Ok(()) without submitting a transaction or logging cancellation; otherwise preserve the current send_cancel and success behavior.
265-282: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A failed submission drops the task permanently.
submitreturns the scheduling error,process_requestlogs it at Line 127, and nothing else happens. The service holds no state, so a transient failure — a stale blockhash fromself.engine.blockhash(), a temporarily unfunded faucet, or a lost race with hydra closing the crank — silently discards a task the user already paid to schedule. The removed database previously provided the retry path.Add bounded retry with backoff for submission failures, or record the failed request so it can be retried. State explicitly in the crate documentation if dropping the task is the accepted behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 265 - 282, Update the submit/process_request flow so failed task submissions are not silently discarded: add bounded retries with backoff around transaction creation and scheduling, or persist failed requests for later retry. Preserve the existing TaskSchedulerError propagation after retries are exhausted, and document the behavior in the crate documentation if task dropping is intentionally accepted.
285-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add tests for the new crank paths.
The only remaining test asserts shutdown on cancellation. The rewrite introduced the funding calculation, the reschedule path that prepends a cancel instruction, the interval-to-slot conversion, and the deterministic PDA derivation. None of them are covered. Add tests that assert the instruction list
send_createbuilds for a new crank and for an existing crank, and thatfundingequalsiterations * CRANKER_REWARD + crank_rent_floor(instructions).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 285 - 321, Extend the tests module around test_service with coverage for the new crank paths: assert send_create builds the expected instruction list for both a new crank and an existing crank, including the prepended cancel instruction on reschedule. Add assertions for interval-to-slot conversion and deterministic PDA derivation, and verify funding equals iterations multiplied by CRANKER_REWARD plus crank_rent_floor(instructions).
15ec769 to
6863017
Compare
87d5e7c to
1e477a3
Compare
6863017 to
ea76e6a
Compare
405bd42 to
c4a54d5
Compare
c4a54d5 to
b40b2b1
Compare
ea76e6a to
dee4d61
Compare
c40cc22 to
dd316c3
Compare
dee4d61 to
9ae900f
Compare
c40cc22 to
836e5f9
Compare
836e5f9 to
2e64b83
Compare
9ae900f to
27e8c37
Compare
4761dba to
dcebe2c
Compare
27e8c37 to
f23a497
Compare
dcebe2c to
7368b12
Compare
f23a497 to
845638d
Compare
7368b12 to
c0554ec
Compare
845638d to
3288f82
Compare
c0554ec to
0b69900
Compare
3288f82 to
25ec8f8
Compare
2 participants
Summary
Introduce a configurable risk checking strategy: aggressively check all delegation actions, or only those involving programs relevant to money laundering: token programs and the magic program. Closes #1435
Breaking Changes
Summary by CodeRabbit
New Features
Documentation