feat: schedule automatic undelegation on AML check failure#1300
feat: schedule automatic undelegation on AML check failure#1300Dodecahedr0x wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds an AML-triggered undelegation scheduling path. A new 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-api/src/magic_validator.rs`:
- Around line 115-120: Wrap the await on self.0.schedule_undelegation(pubkey,
request.account).await in a tokio timeout (e.g.,
tokio::time::timeout(Duration::from_secs(...), ...)) so the call cannot hang
indefinitely; after awaiting the timeout, map a timeout error to the same
chainlink/committor error variant currently produced (the "committor response
channel closed" mapping) so both channel-closure and timeout produce the same
error path, and then continue to .and_then(|result| ...) as before; update
imports if necessary to include tokio::time::timeout and Duration and ensure the
timeout mapping converts into a String consistent with the existing
.map_err(|message| ...) flow.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs`:
- Around line 251-255: The mock server's blocking accept loop inside the
tokio::task::spawn_blocking worker (which calls listener.accept() in a for
0..expected_calls loop) can hang CI if fewer connections arrive; modify the
worker to enforce a hard timeout (e.g., 5s) for each accept or for the entire
loop: set the TcpListener to non-blocking (listener.set_nonblocking(true)) or
use Instant::now()/elapsed to track time, repeatedly try accept() with a short
sleep, and if the deadline is exceeded before receiving all expected_calls,
break and return an error (or panic) so the test fails fast instead of hanging
while the test awaits the worker join. Ensure the change is applied to the same
worker block that spawns the mock server and to the other similar instance
mentioned (around the other accept loop).
In `@magicblock-chainlink/tests/10_aml_undelegation.rs`:
- Around line 70-74: The mock risk-server worker can block forever on
listener.accept() inside the spawned closure (the worker) causing join() to
hang; change the worker loop to avoid blocking by calling
listener.set_nonblocking(true) (or use try_accept if available) and perform a
bounded retry loop with a short sleep, counting successful accepts until
expected_calls or until a configurable timeout elapses, then break and return an
error/result so the task completes; update the code that awaits the worker (the
join() site) to handle the early-return error case. Ensure you reference the
spawned closure (worker), listener.accept, expected_calls and the join()
awaiting the worker when making the change.
In `@magicblock-committor-service/src/committor_processor.rs`:
- Around line 302-309: The current intent_id() uses wall-clock nanoseconds which
can collide; replace it with a collision-free generator such as a monotonic
atomic counter or a UUID-based ID instead of SystemTime::now(). Specifically,
update the intent_id() function to read from a shared AtomicU64 (e.g.,
NEXT_INTENT_ID.fetch_add(1, Ordering::SeqCst)) or generate a UUID and
convert/encode it for ScheduledIntentBundle.id so each undelegation gets a
unique, monotonic or globally-unique ID; ensure the chosen generator is
initialized and accessible where intent_id() is called and that persisted
keys/status lookups use the same ID format.
In `@magicblock-committor-service/src/service.rs`:
- Around line 68-72: The ScheduleUndelegation variant embeds AccountSharedData
which causes CommittorService::try_send() to log full account bytes on
TrySendError::{Full,Closed}; change the logging to avoid dumping the account
payload by either redacting ScheduleUndelegation's Debug output or logging only
the pubkey and message type. Concretely, update the CommittorService::try_send()
error paths that currently log the CommittorMessage to instead pattern-match the
message and log a concise string for ScheduleUndelegation (e.g.,
"ScheduleUndelegation { pubkey: <pubkey> }") or implement a custom Debug/Display
for that enum variant that omits AccountSharedData; ensure references to
ScheduleUndelegation, CommittorMessage, AccountSharedData, Pubkey, and
TrySendError::Full/Closed are used to locate and change the relevant code.
🪄 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: dd15f542-18ab-4785-a422-c9f5e2e1d1a1
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
magicblock-api/Cargo.tomlmagicblock-api/src/magic_validator.rsmagicblock-chainlink/Cargo.tomlmagicblock-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-chainlink/tests/utils/test_context.rsmagicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/service.rsmagicblock-committor-service/src/service_ext.rsmagicblock-committor-service/src/stubs/changeset_committor_stub.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/src/test_context.rs
|
@Dodecahedr0x : can you please add some description as to when/why undelegation should be scheduled and automated. |
There was a problem hiding this comment.
I think this PR digresses from current committor flow, and imo there's no need in this deviation.
Delegation is an account state property, therefore it has to go through builtin program and modify delegated flags.
This makes flow similar to regular Intent scheduling one, in fact you do schedule an Intent youself.
- Validator detect account is malicious
- Validator sends TX in magic-program(new entrypoint, or an old one with commit)
- magic-program mutates account states correctly and schedules undelegation(
mark_account_as_undelegated) - CommittorService picks it up and executes as in current flow
This gets rid of new schedule_undelegation functionality completely as it will seamlessly integrates into current Committor logic. All is needed is new entrypoint in magic-program and TX sender
Current state bypasses all of that flow to which it belongs. This additionally lead to an amigious state where in AccountsDB account is delegated but on Base it is not. Maybe cloner will fix this(which I'm not sure about) after some updates, but this leaves window for an attack where in meantime it could be used by user.
| struct CommittorUndelegationScheduler(Arc<CommittorService>); | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl UndelegationScheduler for CommittorUndelegationScheduler { |
There was a problem hiding this comment.
magic_validator.rs is already massive, let's extract this into separate file
| } | ||
| } | ||
|
|
||
| pub(crate) fn undelegation_intent_bundle( |
There was a problem hiding this comment.
This has to be created by magic-program which additionally would call mark_account_as_undelegated
| } | ||
| } | ||
|
|
||
| fn intent_id() -> u64 { |
There was a problem hiding this comment.
Intent ids are assigned by magic-program
snawaz
changed the title
|
@taco-paco I completely reworked the flow to use the regular commit path. However, it now clones and undelegates the account in a single transaction. |
There was a problem hiding this comment.
Actionable comments posted: 1
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/utils/instruction_utils.rs (1)
82-84:⚠️ Potential issue | 🟠 MajorMark accounts at index 2..n as non-signers, not signers.
Line 83 marks each PDA as writable and signer with
AccountMeta::new(*pubkey, true), but the instruction spec (magicblock-magic-program-api/src/instruction.rs, line 57) documents accounts 2..n as[](read-only, non-signer). Since the builder signs with only the payer, non-payer accounts marked as signers will fail transaction sanitization in production.Suggested fix
for pubkey in &pdas { - account_metas.push(AccountMeta::new(*pubkey, true)); + account_metas.push(AccountMeta::new_readonly(*pubkey, false)); }🤖 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 `@programs/magicblock/src/utils/instruction_utils.rs` around lines 82 - 84, The for loop in instruction_utils.rs that processes PDAs (line 82-84) marks each account as both writable and signer using AccountMeta::new with true as the second parameter. However, according to the instruction specification in magicblock-magic-program-api/src/instruction.rs at line 57, accounts 2..n should be read-only, non-signers. Change the AccountMeta::new call to use an appropriate constructor method that marks these PDA accounts as read-only and non-signers instead, since only the payer is expected to sign the transaction and marking non-payer accounts as signers will cause transaction sanitization to fail in production.
🤖 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 `@test-integration/Makefile`:
- Around line 57-64: The test-aml target bypasses required program preparation
because the shared test target only runs chainlink-prep-programs when RUN_TESTS
is empty or contains chainlink or cloning. Since aml.devnet.toml requires
artifacts from ../target/deploy/miniv3/program_mini.so, ensure
chainlink-prep-programs is invoked before running aml tests. Either add aml to
the condition that triggers chainlink-prep-programs in the shared test target,
or explicitly invoke chainlink-prep-programs as a prerequisite step in the
test-aml target (similarly to how other test configurations handle program
preparation).
---
Outside diff comments:
In `@programs/magicblock/src/utils/instruction_utils.rs`:
- Around line 82-84: The for loop in instruction_utils.rs that processes PDAs
(line 82-84) marks each account as both writable and signer using
AccountMeta::new with true as the second parameter. However, according to the
instruction specification in magicblock-magic-program-api/src/instruction.rs at
line 57, accounts 2..n should be read-only, non-signers. Change the
AccountMeta::new call to use an appropriate constructor method that marks these
PDA accounts as read-only and non-signers instead, since only the payer is
expected to sign the transaction and marking non-payer accounts as signers will
cause transaction sanitization to fail in production.
🪄 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: b5b1a290-f756-4c8a-a7f2-c08af943c5a5
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/cloner/errors.rsmagicblock-chainlink/src/cloner/mod.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-magic-program-api/src/instruction.rsprograms/magicblock/src/clone_account/process_post_delegation_actions.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_cloned_undelegation.rsprograms/magicblock/src/utils/instruction_utils.rsprograms/magicblock/src/utils/mod.rsprograms/magicblock/src/utils/validation.rstest-integration/Cargo.tomltest-integration/Makefiletest-integration/configs/aml.devnet.tomltest-integration/test-aml/Cargo.tomltest-integration/test-aml/src/lib.rstest-integration/test-aml/tests/range_mock.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_aml_undelegation.rstest-integration/test-runner/Cargo.tomltest-integration/test-runner/bin/run_tests.rs
| test-aml: | ||
| RUN_TESTS=aml \ | ||
| $(MAKE) test | ||
| setup-aml-devnet: | ||
| RUST_LOG_STYLE=none \ | ||
| RUN_TESTS=aml \ | ||
| SETUP_ONLY=devnet \ | ||
| $(MAKE) test |
There was a problem hiding this comment.
test-aml currently bypasses required program prep and can fail on clean runs
test-aml sets RUN_TESTS=aml, but the shared test target only invokes chainlink-prep-programs for empty RUN_TESTS or chainlink|cloning. Since aml.devnet.toml loads artifacts like ../target/deploy/miniv3/program_mini.so, make test-aml can fail before running tests when those artifacts are absent.
Suggested fix
test: $(PROGRAMS_SO)
- if [ -z "$(RUN_TESTS)" ] || printf ',%s,' "$(RUN_TESTS)" | grep -Eq ',(chainlink|cloning),'; then \
+ if [ -z "$(RUN_TESTS)" ] || printf ',%s,' "$(RUN_TESTS)" | grep -Eq ',(chainlink|cloning|aml),'; then \
$(MAKE) chainlink-prep-programs -C ./test-chainlink; \
fi && \🤖 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 `@test-integration/Makefile` around lines 57 - 64, The test-aml target bypasses
required program preparation because the shared test target only runs
chainlink-prep-programs when RUN_TESTS is empty or contains chainlink or
cloning. Since aml.devnet.toml requires artifacts from
../target/deploy/miniv3/program_mini.so, ensure chainlink-prep-programs is
invoked before running aml tests. Either add aml to the condition that triggers
chainlink-prep-programs in the shared test target, or explicitly invoke
chainlink-prep-programs as a prerequisite step in the test-aml target (similarly
to how other test configurations handle program preparation).
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-account-cloner/src/lib.rs`:
- Around line 873-878: The panic messages in the match arms for
PostDelegationActionExecutorInstruction::ScheduleUndelegation are inverted and
misleading. These arms trigger when ScheduleUndelegation is unexpectedly
received, but the panic message says "expected schedule undelegation
instruction" which is backwards. Locate both occurrences of the
ScheduleUndelegation match arm and update each panic message to accurately
reflect that ScheduleUndelegation was received unexpectedly, and state what
instruction was actually expected instead (which should be Execute based on the
test expectations).
🪄 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: a7fc8003-2a37-466a-96e1-35961d05bf41
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rs
| PostDelegationActionExecutorInstruction::ScheduleUndelegation { | ||
| .. | ||
| } => { | ||
| panic!("expected schedule undelegation instruction") | ||
| } | ||
| } |
There was a problem hiding this comment.
Panic messages are inverted and misleading.
These match arms trigger when ScheduleUndelegation is unexpectedly received, but the message says "expected schedule undelegation instruction". The tests actually expect Execute, not ScheduleUndelegation.
🐛 Proposed fix
PostDelegationActionExecutorInstruction::ScheduleUndelegation {
..
} => {
- panic!("expected schedule undelegation instruction")
+ panic!("unexpected ScheduleUndelegation; expected Execute")
}Apply the same fix at both locations (lines 876 and 927).
Also applies to: 924-929
🤖 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-account-cloner/src/lib.rs` around lines 873 - 878, The panic
messages in the match arms for
PostDelegationActionExecutorInstruction::ScheduleUndelegation are inverted and
misleading. These arms trigger when ScheduleUndelegation is unexpectedly
received, but the panic message says "expected schedule undelegation
instruction" which is backwards. Locate both occurrences of the
ScheduleUndelegation match arm and update each panic message to accurately
reflect that ScheduleUndelegation was received unexpectedly, and state what
instruction was actually expected instead (which should be Execute based on the
test expectations).
3 participants
Summary
Automatically schedules the undelegation of AML-flagged accounts. Currently, if an account fails the AML check, it is just not cloned, meaning it stays owned by the DLP on mainnet but can't be used in the ER. This PR automatically undelegates it.
Breaking Changes
Summary by CodeRabbit