Feat: native SPL support #738
Conversation
📝 WalkthroughWalkthroughThis PR updates ephemeral-validator package references to magicblock-validator and introduces comprehensive support for Associated Token Accounts (ATA) and Enhanced ATA (eATA) handling. It adds new dependencies (solana-program, spl-token, spl-token-2022), extends fetch_cloner.rs with ATA detection and eATA derivation utilities, introduces new data structures and trait implementations, updates schedule_commit.rs to remap delegated ATAs to eATAs, creates testing utilities for token account creation, and includes integration tests verifying ATA/eATA replacement behavior during cloning operations. Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/packages/npm-package/package.json(1 hunks).github/packages/npm-package/package.json.tmpl(1 hunks)magicblock-chainlink/Cargo.toml(2 hunks)magicblock-chainlink/src/chainlink/fetch_cloner.rs(10 hunks)magicblock-chainlink/src/testing/eatas.rs(1 hunks)magicblock-chainlink/src/testing/mod.rs(1 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs(2 hunks)test-integration/Cargo.toml(1 hunks)test-integration/test-chainlink/Cargo.toml(1 hunks)test-integration/test-chainlink/tests/ix_ata_eata_replace.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/Cargo.tomltest-integration/test-chainlink/Cargo.toml.github/packages/npm-package/package.json.tmpl.github/packages/npm-package/package.jsonmagicblock-chainlink/Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
test-integration/test-chainlink/Cargo.tomlmagicblock-chainlink/Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
.github/packages/npm-package/package.json.tmpl.github/packages/npm-package/package.json
📚 Learning: 2025-11-24T08:45:11.164Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 675
File: Cargo.toml:145-145
Timestamp: 2025-11-24T08:45:11.164Z
Learning: The reqwest dependency version 0.11 is intentionally used in the magicblock-validator project to maintain compatibility with the Solana client dependencies.
Applied to files:
.github/packages/npm-package/package.json.tmpl.github/packages/npm-package/package.json
📚 Learning: 2025-10-27T09:55:17.808Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:296-298
Timestamp: 2025-10-27T09:55:17.808Z
Learning: In the magicblock-validator codebase, it is acceptable to log API keys in debug/info statements because logs are private and the team considers API keys not that sensitive in their security model.
Applied to files:
.github/packages/npm-package/package.json.tmpl
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
programs/magicblock/src/schedule_transactions/process_schedule_commit.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
programs/magicblock/src/schedule_transactions/process_schedule_commit.rstest-integration/test-chainlink/tests/ix_ata_eata_replace.rsmagicblock-chainlink/src/testing/eatas.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
📚 Learning: 2025-11-25T11:07:20.001Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/programs/flexi-counter/src/processor.rs:643-680
Timestamp: 2025-11-25T11:07:20.001Z
Learning: In the compressed delegation architecture, ownership reassignment during undelegation is handled by the delegation program itself, not by the owner program's external undelegate handler. The external undelegate handler (e.g., `process_external_undelegate_compressed` in flexi-counter) is a callback for program-specific cleanup (restoring lamports and data) after the delegation program has already reassigned ownership back to the original owner stored in the CompressedDelegationRecord.
Applied to files:
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-chainlink/tests/ix_ata_eata_replace.rsmagicblock-chainlink/src/testing/mod.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-chainlink/tests/ix_ata_eata_replace.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-chainlink/tests/ix_ata_eata_replace.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-chainlink/tests/ix_ata_eata_replace.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
📚 Learning: 2025-11-19T11:31:24.218Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
Applied to files:
test-integration/test-chainlink/tests/ix_ata_eata_replace.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner.rs
🧬 Code graph analysis (2)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (4)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (3)
owner(54-60)owner(260-262)data(71-77)magicblock-magic-program-api/src/args.rs (1)
data(26-28)programs/magicblock/src/mutate_accounts/account_mod_data.rs (1)
data(137-144)magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(142-145)
test-integration/test-chainlink/tests/ix_ata_eata_replace.rs (2)
magicblock-chainlink/src/testing/eatas.rs (4)
create_ata_account(34-57)create_eata_account(59-79)derive_ata(18-24)derive_eata(26-32)magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
delegated(95-101)
🔇 Additional comments (20)
test-integration/test-chainlink/Cargo.toml (1)
25-25: LGTM!The workspace dependency addition for
spl-tokenaligns with the broader ATA/eATA test support introduced in this PR..github/packages/npm-package/package.json.tmpl (1)
7-10: LGTM!Repository and bugs URLs correctly updated to reflect the rename from
ephemeral-validatortomagicblock-validator.magicblock-chainlink/Cargo.toml (1)
24-35: LGTM!The new dependencies (
solana-program,spl-token,spl-token-2022) appropriately enable the ATA/eATA support functionality introduced in this PR. Using workspace versions ensures consistency across the codebase..github/packages/npm-package/package.json (1)
5-11: LGTM!All metadata URLs consistently updated to reference
magicblock-validatorrepository.magicblock-chainlink/src/testing/mod.rs (1)
9-12: LGTM!The new
eatasmodule is properly gated with#[cfg(any(test, feature = "dev-context"))], consistent with other testing modules in this file.programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
39-64: LGTM!The program ID constants and PDA derivation functions are correctly defined. The
derive_atauses the standard ATA seeds ([owner, SPL_TOKEN_PROGRAM_ID, mint]withASSOCIATED_TOKEN_PROGRAM_ID), andderive_eatauses the eATA program's seeds ([owner, mint]withEATA_PROGRAM_ID).
244-248: LGTM!The
target_pubkey(potentially remapped to eATA) is correctly used when constructing theCommittedAccount, ensuring that commits/undelegates target the appropriate address.test-integration/Cargo.toml (1)
92-92: Version compatibility confirmed; no action required.The exact version pin
spl-token = "=7.0"is compatible withsolana-sdk 2.2and aligns with the Solana 2.x ecosystem. This pinning strategy ensures reproducible builds across the workspace.test-integration/test-chainlink/tests/ix_ata_eata_replace.rs (4)
1-12: LGTM!Imports are appropriate for the test scenarios. The use of
ReadableAccountandPacktraits are correctly imported for SPL token account handling.
15-72: LGTM!Test correctly verifies that when an eATA is delegated to the validator, the cloned ATA account is populated with the eATA's data. The assertions cover all relevant token account fields including mint, amount, owner, close_authority, state, delegated_amount, is_native, and the delegated flag.
74-108: LGTM!Good negative test case. Correctly verifies that without a delegation record, the cloned ATA retains its original data and is not marked as delegated.
110-161: LGTM!Good edge case test. Correctly verifies that when the eATA is delegated to a different validator (not ours), the cloned ATA retains its original data and is not marked as delegated.
magicblock-chainlink/src/testing/eatas.rs (4)
18-24: LGTM!The ATA derivation uses the correct seeds
[owner, SPL_TOKEN_PROGRAM_ID, mint]per the SPL Associated Token Program specification.
26-32: LGTM!The eATA derivation correctly uses seeds
[owner, mint]with the EATA program ID, matching the derivation infetch_cloner.rs.
34-57: LGTM!The function correctly creates a mock SPL Token account with proper initialization using
SplAccount::packand calculates minimum rent-exempt lamports.
59-79: LGTM!The eATA account creation correctly matches the expected data layout (owner 32B + mint 32B + amount 8B) that
fetch_cloner.rs::maybe_into_ataparses. The conditional owner selection based ondelegateflag correctly distinguishes delegated vs. non-delegated eATAs.magicblock-chainlink/src/chainlink/fetch_cloner.rs (4)
16-20: LGTM!New imports for SPL token handling are appropriate:
Account,COption,Pack,Rent, andSplAccount/AccountStatefor constructing token accounts.
669-763: LGTM!The account categorization logic cleanly integrates ATA detection. ATAs are properly separated into their own category for special handling downstream, following the existing pattern for delegated accounts and programs.
3331-3394: LGTM!The
is_atafunction correctly:
- Validates account ownership (SPL Token or Token-2022)
- Checks minimum data length before parsing
- Extracts mint and owner from the standard SPL token account layout
- Verifies the account pubkey matches the derived ATA address
This ensures only valid ATAs are detected.
3396-3409: LGTM!The eATA derivation correctly uses
try_find_program_addresswith seeds[owner, mint]and returnsOptionto safely handle potential derivation failures.
| // We will compute subscription cancellations after ATA handling, once accounts_to_clone is finalized | ||
|
|
||
| // Handle ATAs: for each detected ATA, we derive the eATA PDA, subscribe to both, | ||
| // and, if the ATA is delegated to us and the eATA exists, we clone the eATA data | ||
| // into the ATA in the bank. | ||
| if !atas.is_empty() { | ||
| let mut ata_join_set = JoinSet::new(); | ||
|
|
||
| // Subscribe first so subsequent fetches are kept up-to-date | ||
| for (ata_pubkey, _, ata_info, slot_for_ata) in &atas { | ||
| let _ = self.subscribe_to_account(ata_pubkey).await; | ||
| if let Some((eata, _)) = try_derive_eata_address_and_bump( | ||
| &ata_info.owner, | ||
| &ata_info.mint, | ||
| ) { | ||
| let _ = self.subscribe_to_account(&eata).await; | ||
|
|
||
| let effective_slot = | ||
| if let Some(min_slot) = min_context_slot { | ||
| min_slot.max(*slot_for_ata) | ||
| } else { | ||
| *slot_for_ata | ||
| }; | ||
| ata_join_set.spawn(self.task_to_fetch_with_companion( | ||
| *ata_pubkey, | ||
| eata, | ||
| effective_slot, | ||
| fetch_origin, | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| let ata_results = ata_join_set.join_all().await; | ||
| for res in ata_results.into_iter() { | ||
| match res { | ||
| Ok(Ok(AccountWithCompanion { | ||
| pubkey: ata_pubkey, | ||
| account: ata_account, | ||
| companion_pubkey: eata_pubkey, | ||
| companion_account: maybe_eata_account, | ||
| })) => { | ||
| // Convert to AccountSharedData using the bank snapshot | ||
| let mut account_to_clone = | ||
| ata_account.account_shared_data_cloned(); | ||
| let mut commit_frequency_ms = None; | ||
| if let Some(eata_acc) = maybe_eata_account { | ||
| let eata_shared = | ||
| eata_acc.account_shared_data_cloned(); | ||
| if let Some(deleg) = self | ||
| .fetch_and_parse_delegation_record( | ||
| eata_pubkey, | ||
| self.remote_account_provider.chain_slot(), | ||
| fetch_origin, | ||
| ) | ||
| .await | ||
| { | ||
| let is_delegated_to_us = deleg | ||
| .authority | ||
| .eq(&self.validator_pubkey) | ||
| || deleg.authority.eq(&Pubkey::default()); | ||
| if is_delegated_to_us { | ||
| if let Some(projected_ata) = | ||
| eata_shared.maybe_into_ata(deleg) | ||
| { | ||
| account_to_clone = projected_ata; | ||
| account_to_clone.set_delegated(true); | ||
| commit_frequency_ms = | ||
| Some(deleg.commit_frequency_ms); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| accounts_to_clone.push(AccountCloneRequest { | ||
| pubkey: ata_pubkey, | ||
| account: account_to_clone, | ||
| commit_frequency_ms, | ||
| }); | ||
| } | ||
| Ok(Err(err)) => { | ||
| warn!("Failed to resolve ATA/eATA companion: {err}"); | ||
| } | ||
| Err(join_err) => { | ||
| warn!("Failed to join ATA/eATA fetch task: {join_err}"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for cancel_subs and delegated_acc_subs usage in fetch_cloner.rs
rg -n "cancel_subs|delegated_acc_subs" magicblock-chainlink/src/chainlink/fetch_cloner.rs | head -50Repository: magicblock-labs/magicblock-validator
Length of output: 445
🏁 Script executed:
# View the section where delegated_acc_subs is populated and cancel_subs is called
sed -n '1200,1230p' magicblock-chainlink/src/chainlink/fetch_cloner.rsRepository: magicblock-labs/magicblock-validator
Length of output: 1219
🏁 Script executed:
# View the cancel_subs function implementation
sed -n '1803,1900p' magicblock-chainlink/src/chainlink/fetch_cloner.rsRepository: magicblock-labs/magicblock-validator
Length of output: 3183
🏁 Script executed:
# Search for maybe_into_ata implementation
rg -n "fn maybe_into_ata" magicblock-chainlink/src/chainlink/fetch_cloner.rs -A 20Repository: magicblock-labs/magicblock-validator
Length of output: 1560
🏁 Script executed:
# Search for patterns of subscription cleanup and how delegated accounts are handled
rg -n "set_delegated.*true" magicblock-chainlink/src/chainlink/fetch_cloner.rs -B 5 -A 5 | head -60Repository: magicblock-labs/magicblock-validator
Length of output: 760
🏁 Script executed:
# Check how delegation records are handled - look for record_subs usage
rg -n "record_subs" magicblock-chainlink/src/chainlink/fetch_cloner.rs -B 3 -A 3 | head -80Repository: magicblock-labs/magicblock-validator
Length of output: 2743
🏁 Script executed:
# View the complete ATA handling section with comments
sed -n '1107,1200p' magicblock-chainlink/src/chainlink/fetch_cloner.rsRepository: magicblock-labs/magicblock-validator
Length of output: 4513
🏁 Script executed:
# Search for tests related to ATA delegation to understand expected behavior
rg -n "maybe_into_ata|ata.*deleg|eata" magicblock-chainlink/src/chainlink/fetch_cloner.rs | grep -i testRepository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
# Search for test files that mention ATA delegation or fetch_cloner tests
find magicblock-chainlink -name "*.rs" -type f | xargs grep -l "fetch_cloner\|test.*ata\|test.*eata" | grep testRepository: magicblock-labs/magicblock-validator
Length of output: 170
🏁 Script executed:
# View the eatas test file to understand expected behavior
head -100 magicblock-chainlink/src/testing/eatas.rsRepository: magicblock-labs/magicblock-validator
Length of output: 2650
🏁 Script executed:
# Search for any TODOs, FIXMEs, or issue references related to ATA/eATA subscriptions
rg -n "TODO|FIXME|XXX|HACK|BUG" magicblock-chainlink/src/chainlink/fetch_cloner.rs | grep -i "ata\|sub\|deleg" | head -20Repository: magicblock-labs/magicblock-validator
Length of output: 280
🏁 Script executed:
# Let's check what accounts are actually tracked for subscription cancellation
# Look at how acc_subs is built and used
sed -n '1195,1230p' magicblock-chainlink/src/chainlink/fetch_cloner.rsRepository: magicblock-labs/magicblock-validator
Length of output: 1424
🏁 Script executed:
# Check if eATA pubkeys are ever added to any subscription tracking structures
rg -n "eata|eATA" magicblock-chainlink/src/chainlink/fetch_cloner.rs | grep -i "subs\|record\|track\|cancel" | head -20Repository: magicblock-labs/magicblock-validator
Length of output: 247
Unsubscribe from eATA accounts when delegation conversion fails.
eATA accounts are subscribed to at line 1122 but are never tracked for subscription cancellation—they're not added to delegated_acc_subs or any other cleanup structure. When maybe_into_ata returns None (invalid data format), the fallback uses the original ATA data, leaving the eATA subscription active unnecessarily. Consider tracking eATA pubkeys separately and cancelling them when conversion fails or the delegation check doesn't pass.
The ATA subscription itself is correctly handled: delegated ATAs are included in delegated_acc_subs and cancelled, while non-delegated ATAs are retained (intentional fallback behavior).
🤖 Prompt for AI Agents
In magicblock-chainlink/src/chainlink/fetch_cloner.rs around lines 1107-1194:
eATA accounts are subscribed to (line ~1122) but their pubkeys are not recorded
for cleanup, so when delegation conversion fails or the eATA is not delegated to
us we leave the eATA subscription active; fix by collecting eATA pubkeys (e.g.,
push eata into a local Vec or add to delegated_acc_subs-like structure) when you
subscribe, and if fetch_and_parse_delegation_record returns None or delegation
isn’t to us (or maybe_into_ata returns None) then call the same
unsubscribe/cancel routine used for other temporary subscriptions for that eATA
pubkey (or remove from the collection of kept subscriptions), otherwise, when
you accept the converted ATA mark it as retained as you already do; add the
collection and unsubscribe calls just after subscription and in the branches
where conversion/delegation check fails.
| impl Into<AccountSharedData> for EphemeralAta { | ||
| fn into(self) -> AccountSharedData { | ||
| let token_account = SplAccount { | ||
| mint: self.mint, | ||
| owner: self.owner, | ||
| amount: self.amount, | ||
| delegate: COption::None, | ||
| state: AccountState::Initialized, | ||
| is_native: COption::None, | ||
| delegated_amount: 0, | ||
| close_authority: COption::None, | ||
| }; | ||
|
|
||
| let mut data = vec![0u8; SplAccount::LEN]; | ||
| SplAccount::pack(token_account, &mut data) | ||
| .expect("pack spl token account"); | ||
| let lamports = Rent::default().minimum_balance(data.len()); | ||
|
|
||
| let account = Account { | ||
| owner: spl_token::id(), | ||
| data, | ||
| lamports, | ||
| executable: false, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| AccountSharedData::from(account) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer impl From<EphemeralAta> for AccountSharedData over impl Into.
Clippy warns against implementing Into directly (clippy::from_over_into). The idiomatic pattern is to implement From, which automatically provides Into.
-impl Into<AccountSharedData> for EphemeralAta {
- fn into(self) -> AccountSharedData {
+impl From<EphemeralAta> for AccountSharedData {
+ fn from(eata: EphemeralAta) -> Self {
let token_account = SplAccount {
- mint: self.mint,
- owner: self.owner,
- amount: self.amount,
+ mint: eata.mint,
+ owner: eata.owner,
+ amount: eata.amount,
delegate: COption::None,
state: AccountState::Initialized,
is_native: COption::None,
delegated_amount: 0,
close_authority: COption::None,
};
// ... rest unchanged
}
}📝 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.
| impl Into<AccountSharedData> for EphemeralAta { | |
| fn into(self) -> AccountSharedData { | |
| let token_account = SplAccount { | |
| mint: self.mint, | |
| owner: self.owner, | |
| amount: self.amount, | |
| delegate: COption::None, | |
| state: AccountState::Initialized, | |
| is_native: COption::None, | |
| delegated_amount: 0, | |
| close_authority: COption::None, | |
| }; | |
| let mut data = vec![0u8; SplAccount::LEN]; | |
| SplAccount::pack(token_account, &mut data) | |
| .expect("pack spl token account"); | |
| let lamports = Rent::default().minimum_balance(data.len()); | |
| let account = Account { | |
| owner: spl_token::id(), | |
| data, | |
| lamports, | |
| executable: false, | |
| ..Default::default() | |
| }; | |
| AccountSharedData::from(account) | |
| } | |
| } | |
| impl From<EphemeralAta> for AccountSharedData { | |
| fn from(eata: EphemeralAta) -> Self { | |
| let token_account = SplAccount { | |
| mint: eata.mint, | |
| owner: eata.owner, | |
| amount: eata.amount, | |
| delegate: COption::None, | |
| state: AccountState::Initialized, | |
| is_native: COption::None, | |
| delegated_amount: 0, | |
| close_authority: COption::None, | |
| }; | |
| let mut data = vec![0u8; SplAccount::LEN]; | |
| SplAccount::pack(token_account, &mut data) | |
| .expect("pack spl token account"); | |
| let lamports = Rent::default().minimum_balance(data.len()); | |
| let account = Account { | |
| owner: spl_token::id(), | |
| data, | |
| lamports, | |
| executable: false, | |
| ..Default::default() | |
| }; | |
| AccountSharedData::from(account) | |
| } | |
| } |
🤖 Prompt for AI Agents
In magicblock-chainlink/src/chainlink/fetch_cloner.rs around lines 3426 to 3454,
the code implements Into<AccountSharedData> for EphemeralAta which triggers
clippy::from_over_into; replace that impl with impl From<EphemeralAta> for
AccountSharedData keeping the exact conversion body but change the function
signature to fn from(src: EphemeralAta) -> AccountSharedData, remove the Into
impl, and rely on the auto-generated Into impl for callers.
| impl MaybeIntoAta<AccountSharedData> for AccountSharedData { | ||
| fn maybe_into_ata( | ||
| &self, | ||
| record: DelegationRecord, | ||
| ) -> Option<AccountSharedData> { | ||
| if !record.owner.ne(&EATA_PROGRAM_ID) { | ||
| return None; | ||
| } | ||
| let data = self.data(); | ||
| if data.len() < 40 { | ||
| return None; | ||
| } | ||
| let owner = Pubkey::new_from_array(data[0..32].try_into().ok()?); | ||
| let mint = Pubkey::new_from_array(data[32..64].try_into().ok()?); | ||
| let amount = u64::from_le_bytes(data[64..72].try_into().ok()?); | ||
| let eata = EphemeralAta { | ||
| owner, | ||
| mint, | ||
| amount, | ||
| }; | ||
| Some(eata.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Logic error with confusing double negation.
Line 3461: if !record.owner.ne(&EATA_PROGRAM_ID) simplifies to if record.owner == EATA_PROGRAM_ID, returning None when the delegation record's owner equals EATA_PROGRAM_ID.
This logic seems inverted. The delegation record's owner field typically stores the original account owner (e.g., the wallet owner), not the program ID. Based on the test utility add_delegation_record_for, the record owner is set to the wallet owner.
If the intent is to validate that this is actually an eATA delegation, the check should likely be verifying something else (perhaps self.owner() checking the account's program owner?).
Consider clarifying the intended logic. If you want to check that the account is NOT owned by EATA_PROGRAM_ID:
- if !record.owner.ne(&EATA_PROGRAM_ID) {
+ if record.owner == EATA_PROGRAM_ID {
return None;
}Or, if the check is meant to validate something else entirely, please clarify the intent.
🤖 Prompt for AI Agents
In magicblock-chainlink/src/chainlink/fetch_cloner.rs around lines 3456–3478,
the conditional uses a confusing double-negation `if
!record.owner.ne(&EATA_PROGRAM_ID)` which is equivalent to `if record.owner ==
EATA_PROGRAM_ID` and therefore returns None in the inverted case; change the
check to the intended intent: either replace with `if record.owner !=
EATA_PROGRAM_ID { return None; }` to only proceed for eATA delegation records,
or if the intent was to validate the account's program owner instead, call the
account owner accessor (e.g., self.owner() or the appropriate AccountSharedData
method) and compare that to EATA_PROGRAM_ID; update the condition accordingly
and add a short comment clarifying which owner is being validated.
| let data = self.data(); | ||
| if data.len() < 40 { | ||
| return None; | ||
| } | ||
| let owner = Pubkey::new_from_array(data[0..32].try_into().ok()?); | ||
| let mint = Pubkey::new_from_array(data[32..64].try_into().ok()?); | ||
| let amount = u64::from_le_bytes(data[64..72].try_into().ok()?); |
There was a problem hiding this comment.
Buffer length check is insufficient - potential panic.
Line 3465 checks data.len() < 40, but the code reads data[64..72] for the amount (line 3470). If data length is between 40 and 71 bytes, this will panic with an index out of bounds error.
- if data.len() < 40 {
+ if data.len() < 72 {
return None;
}📝 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.
| let data = self.data(); | |
| if data.len() < 40 { | |
| return None; | |
| } | |
| let owner = Pubkey::new_from_array(data[0..32].try_into().ok()?); | |
| let mint = Pubkey::new_from_array(data[32..64].try_into().ok()?); | |
| let amount = u64::from_le_bytes(data[64..72].try_into().ok()?); | |
| let data = self.data(); | |
| if data.len() < 72 { | |
| return None; | |
| } | |
| let owner = Pubkey::new_from_array(data[0..32].try_into().ok()?); | |
| let mint = Pubkey::new_from_array(data[32..64].try_into().ok()?); | |
| let amount = u64::from_le_bytes(data[64..72].try_into().ok()?); |
🤖 Prompt for AI Agents
In magicblock-chainlink/src/chainlink/fetch_cloner.rs around lines 3464 to 3470
the length guard is incorrect: it only checks for data.len() < 40 but later
slices up to 72, which can panic; change the guard to ensure the buffer is at
least 72 bytes (e.g. if data.len() < 72 { return None; }) or replace the slices
with safe .get() calls and propagate None on short buffers so owner, mint and
amount reads cannot index out of bounds.
| const SPL_TOKEN_PROGRAM_ID: Pubkey = | ||
| pubkey!("TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA"); | ||
|
|
||
| // Associated Token Program id | ||
| const ASSOCIATED_TOKEN_PROGRAM_ID: Pubkey = | ||
| pubkey!("ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL"); | ||
|
|
||
| // eATA PDA derivation seed copied from fetch_cloner.rs | ||
| const EATA_PROGRAM_ID: Pubkey = | ||
| pubkey!("5iC4wKZizyxrKh271Xzx3W4Vn2xUyYvSGHeoB2mdw5HA"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duplicated constants.
These program ID constants (SPL_TOKEN_PROGRAM_ID, ASSOCIATED_TOKEN_PROGRAM_ID, EATA_PROGRAM_ID) are duplicated from fetch_cloner.rs. Consider extracting them to a shared module to avoid maintenance drift.
🤖 Prompt for AI Agents
In magicblock-chainlink/src/testing/eatas.rs around lines 7 to 16, the three
program ID constants are duplicated from fetch_cloner.rs; extract these
constants into a new shared module (e.g., magicblock-chainlink/src/constants.rs
or src/program_ids.rs) as pub consts, update both eatas.rs and fetch_cloner.rs
to import them (use crate::constants::{SPL_TOKEN_PROGRAM_ID,
ASSOCIATED_TOKEN_PROGRAM_ID, EATA_PROGRAM_ID}), remove the duplicated
declarations here, and run cargo check to ensure visibility and namespaces are
correct; add a short doc comment on the shared constants and keep the same
Pubkey initializers so behavior remains unchanged.
| /// Internal representation of a token account data. | ||
| #[repr(C)] | ||
| pub struct EphemeralAta { | ||
| /// The owner of the eata | ||
| pub owner: Pubkey, | ||
| /// The mint associated with this account | ||
| pub mint: Pubkey, | ||
| /// The amount of tokens this account holds. | ||
| pub amount: u64, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate struct definition.
EphemeralAta is also defined in fetch_cloner.rs (lines 3416-3424). Consider consolidating this into a single definition that can be shared between production code and tests.
🤖 Prompt for AI Agents
In magicblock-chainlink/src/testing/eatas.rs around lines 81 to 90, EphemeralAta
is duplicated (also defined in fetch_cloner.rs lines 3416-3424); remove the
duplicate and consolidate into a single shared definition: create a single pub
struct EphemeralAta in a common module (e.g., src/common/ephemeral_ata.rs or
src/types.rs), export it (pub use) and update both testing and production files
to import that definition, or move the shared definition into lib.rs and
reference it from both places, ensuring visibility and removing the redundant
struct declaration.
| // ---------- Helpers for ATA/eATA remapping tests ---------- | ||
| // SPL Token and ATA/eATA program ids | ||
| const SPL_TOKEN_PROGRAM_ID: Pubkey = | ||
| solana_sdk::pubkey!("TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA"); | ||
| const ASSOCIATED_TOKEN_PROGRAM_ID: Pubkey = | ||
| solana_sdk::pubkey!("ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL"); | ||
| const EATA_PROGRAM_ID: Pubkey = | ||
| solana_sdk::pubkey!("5iC4wKZizyxrKh271Xzx3W4Vn2xUyYvSGHeoB2mdw5HA"); | ||
|
|
||
| fn derive_ata(owner: &Pubkey, mint: &Pubkey) -> Pubkey { | ||
| Pubkey::find_program_address( | ||
| &[owner.as_ref(), SPL_TOKEN_PROGRAM_ID.as_ref(), mint.as_ref()], | ||
| &ASSOCIATED_TOKEN_PROGRAM_ID, | ||
| ) | ||
| .0 | ||
| } | ||
|
|
||
| fn derive_eata(owner: &Pubkey, mint: &Pubkey) -> Pubkey { | ||
| Pubkey::find_program_address( | ||
| &[owner.as_ref(), mint.as_ref()], | ||
| &EATA_PROGRAM_ID, | ||
| ) | ||
| .0 | ||
| } | ||
|
|
||
| fn make_delegated_spl_ata_account( | ||
| owner: &Pubkey, | ||
| mint: &Pubkey, | ||
| ) -> AccountSharedData { | ||
| // Minimal SPL token account data: first 32 bytes mint, next 32 owner | ||
| let mut data = vec![0u8; 72]; | ||
| data[0..32].copy_from_slice(mint.as_ref()); | ||
| data[32..64].copy_from_slice(owner.as_ref()); | ||
|
|
||
| let mut acc = | ||
| AccountSharedData::new(0, data.len(), &SPL_TOKEN_PROGRAM_ID); | ||
| acc.set_data_from_slice(&data); | ||
| acc.set_delegated(true); | ||
| acc | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ATA/eATA helper derivations look correct; minor nit on SPL account data length
The ATA/eATA derivation helpers and the delegated SPL ATA fabricator look consistent with how ATAs are derived (wallet, SPL token program, mint) and with your new remapping tests. One small readability nit: make_delegated_spl_ata_account allocates 72 bytes of data, while the comment says the “minimal” layout only needs the first 32 (mint) + 32 (owner). Either adjust the buffer size to 64 or clarify in the comment why 72 is used so future readers don’t assume the extra 8 bytes are significant.
🤖 Prompt for AI Agents
In
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
around lines 263 to 303, the helper make_delegated_spl_ata_account allocates a
72‑byte data buffer but the comment states only 32 bytes for mint + 32 bytes for
owner are needed; either change the allocation to vec![0u8; 64] to match the
minimal layout or update the comment to explain why 8 extra bytes are
intentionally reserved (and what they represent), and keep the rest of the
function logic unchanged.
| #[test] | ||
| fn test_schedule_commit_remaps_delegated_ata_to_eata() { | ||
| init_logger!(); | ||
|
|
||
| let payer = | ||
| Keypair::from_seed(b"schedule_commit_remap_ata_to_eata").unwrap(); | ||
| let wallet_owner = Pubkey::new_unique(); | ||
| let mint = Pubkey::new_unique(); | ||
| let ata_pubkey = derive_ata(&wallet_owner, &mint); | ||
| let eata_pubkey = derive_eata(&wallet_owner, &mint); | ||
|
|
||
| // 1) Prepare transaction with our ATA as the only committee | ||
| let (mut account_data, mut transaction_accounts) = | ||
| prepare_transaction_with_single_committee( | ||
| &payer, | ||
| Pubkey::new_unique(), | ||
| ata_pubkey, | ||
| ); | ||
|
|
||
| // Replace the committee account with a delegated SPL-Token ATA layout | ||
| account_data.insert( | ||
| ata_pubkey, | ||
| make_delegated_spl_ata_account(&wallet_owner, &mint), | ||
| ); | ||
|
|
||
| // Build ScheduleCommit instruction using the ATA pubkey | ||
| let ix = InstructionUtils::schedule_commit_instruction( | ||
| &payer.pubkey(), | ||
| vec![ata_pubkey], | ||
| ); | ||
| extend_transaction_accounts_from_ix( | ||
| &ix, | ||
| &mut account_data, | ||
| &mut transaction_accounts, | ||
| ); | ||
|
|
||
| // Execute scheduling | ||
| let processed_scheduled = process_instruction( | ||
| ix.data.as_slice(), | ||
| transaction_accounts.clone(), | ||
| ix.accounts.clone(), | ||
| Ok(()), | ||
| ); | ||
|
|
||
| // Extract magic context and then accept scheduled commits | ||
| let magic_context_acc = assert_non_accepted_actions( | ||
| &processed_scheduled, | ||
| &payer.pubkey(), | ||
| 1, | ||
| ); | ||
|
|
||
| let ix_accept = | ||
| InstructionUtils::accept_scheduled_commits_instruction(); | ||
| let (mut account_data2, mut transaction_accounts2) = | ||
| prepare_transaction_with_single_committee( | ||
| &payer, | ||
| Pubkey::new_unique(), | ||
| ata_pubkey, | ||
| ); | ||
| extend_transaction_accounts_from_ix_adding_magic_context( | ||
| &ix_accept, | ||
| magic_context_acc, | ||
| &mut account_data2, | ||
| &mut transaction_accounts2, | ||
| ); | ||
| let processed_accepted = process_instruction( | ||
| ix_accept.data.as_slice(), | ||
| transaction_accounts2, | ||
| ix_accept.accounts, | ||
| Ok(()), | ||
| ); | ||
|
|
||
| let scheduled = | ||
| assert_accepted_actions(&processed_accepted, &payer.pubkey(), 1); | ||
| // Verify the committed pubkey remapped to eATA | ||
| assert_eq!( | ||
| scheduled[0].base_intent.get_committed_pubkeys().unwrap(), | ||
| vec![eata_pubkey] | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_schedule_commit_and_undelegate_remaps_delegated_ata_to_eata() { | ||
| init_logger!(); | ||
|
|
||
| let payer = | ||
| Keypair::from_seed(b"schedule_commit_undelegate_remap_ata_eata") | ||
| .unwrap(); | ||
| let wallet_owner = Pubkey::new_unique(); | ||
| let mint = Pubkey::new_unique(); | ||
| let ata_pubkey = derive_ata(&wallet_owner, &mint); | ||
| let eata_pubkey = derive_eata(&wallet_owner, &mint); | ||
|
|
||
| // 1) Prepare transaction with our ATA as the only committee | ||
| let (mut account_data, mut transaction_accounts) = | ||
| prepare_transaction_with_single_committee( | ||
| &payer, | ||
| Pubkey::new_unique(), | ||
| ata_pubkey, | ||
| ); | ||
|
|
||
| // Replace the committee account with a delegated SPL-Token ATA layout | ||
| account_data.insert( | ||
| ata_pubkey, | ||
| make_delegated_spl_ata_account(&wallet_owner, &mint), | ||
| ); | ||
|
|
||
| // Build ScheduleCommitAndUndelegate instruction using the ATA pubkey (writable) | ||
| let ix = InstructionUtils::schedule_commit_and_undelegate_instruction( | ||
| &payer.pubkey(), | ||
| vec![ata_pubkey], | ||
| ); | ||
| extend_transaction_accounts_from_ix( | ||
| &ix, | ||
| &mut account_data, | ||
| &mut transaction_accounts, | ||
| ); | ||
|
|
||
| // Execute scheduling | ||
| let processed_scheduled = process_instruction( | ||
| ix.data.as_slice(), | ||
| transaction_accounts.clone(), | ||
| ix.accounts.clone(), | ||
| Ok(()), | ||
| ); | ||
|
|
||
| // Extract magic context and then accept scheduled commits | ||
| let magic_context_acc = assert_non_accepted_actions( | ||
| &processed_scheduled, | ||
| &payer.pubkey(), | ||
| 1, | ||
| ); | ||
|
|
||
| let ix_accept = | ||
| InstructionUtils::accept_scheduled_commits_instruction(); | ||
| let (mut account_data2, mut transaction_accounts2) = | ||
| prepare_transaction_with_single_committee( | ||
| &payer, | ||
| Pubkey::new_unique(), | ||
| ata_pubkey, | ||
| ); | ||
| extend_transaction_accounts_from_ix_adding_magic_context( | ||
| &ix_accept, | ||
| magic_context_acc, | ||
| &mut account_data2, | ||
| &mut transaction_accounts2, | ||
| ); | ||
| let processed_accepted = process_instruction( | ||
| ix_accept.data.as_slice(), | ||
| transaction_accounts2, | ||
| ix_accept.accounts, | ||
| Ok(()), | ||
| ); | ||
|
|
||
| let scheduled = | ||
| assert_accepted_actions(&processed_accepted, &payer.pubkey(), 1); | ||
| // Verify the committed pubkey remapped to eATA | ||
| assert_eq!( | ||
| scheduled[0].base_intent.get_committed_pubkeys().unwrap(), | ||
| vec![eata_pubkey] | ||
| ); | ||
| // And the intent contains undelegation | ||
| assert!(scheduled[0].base_intent.is_undelegate()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ATA→eATA remapping tests are solid; consider tiny consistency/DRY tweaks
Both new tests correctly:
- Synthesize a delegated SPL ATA account for the committee,
- Run the full schedule + accept flows, and
- Assert that the committed pubkey is the derived eATA (and
is_undelegate()in the second test).
Behaviorally this is spot‑on and matches the intended MIMD-0013 semantics. As optional polish:
- In the accept phase you call
prepare_transaction_with_single_committeewith a freshPubkey::new_unique()forprograminstead of reusing a localprogramfrom the schedule phase (unlike earlier tests); reusing aprogramvariable would make control flow slightly easier to follow. - The two tests share a lot of setup; extracting a small helper for “schedule and accept with delegated ATA, expecting optional undelegate” would reduce duplication.
These are style-only; functionally the tests look good.
🤖 Prompt for AI Agents
In
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
around lines 475 to 638, the two new tests duplicate setup and pass a fresh
Pubkey::new_unique() as the `program` argument in the accept-phase call to
prepare_transaction_with_single_committee instead of reusing the `program` used
in the schedule phase; refactor by (1) capturing the `program` Pubkey used in
the schedule preparation into a local variable and passing that same `program`
into the accept-phase prepare_transaction_with_single_committee calls, and (2)
optionally extract a small helper function that performs “schedule with
delegated ATA, accept, and assert expected remap (and optional undelegate)” to
reduce duplicated setup and assertions across the two tests.
| // If this is a delegated SPL Token ATA that was cloned from an eATA, | ||
| // we should commit/undelegate the corresponding eATA instead. | ||
| let mut target_pubkey = *acc_pubkey; | ||
| let acc_borrow = acc.borrow(); | ||
| if acc_borrow.delegated() | ||
| && acc_borrow.owner() == &SPL_TOKEN_PROGRAM_ID | ||
| { | ||
| let data = acc_borrow.data(); | ||
| if data.len() >= 64 { | ||
| // spl-token Account layout: [0..32]=mint, [32..64]=owner | ||
| let mint = | ||
| Pubkey::new_from_array(match data[0..32].try_into() { | ||
| Ok(a) => a, | ||
| Err(_) => [0u8; 32], | ||
| }); | ||
| let wallet_owner = | ||
| Pubkey::new_from_array(match data[32..64].try_into() { | ||
| Ok(a) => a, | ||
| Err(_) => [0u8; 32], | ||
| }); | ||
|
|
||
| // Verify that the current pubkey matches the derived ATA | ||
| let ata_addr = derive_ata(&wallet_owner, &mint); | ||
| if ata_addr == *acc_pubkey { | ||
| // Remap to eATA PDA | ||
| target_pubkey = derive_eata(&wallet_owner, &mint); | ||
| ic_msg!( | ||
| invoke_context, | ||
| "ScheduleCommit: remapping ATA {} -> eATA {} for commit/undelegate", | ||
| acc_pubkey, | ||
| target_pubkey | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using expect instead of fallback to zeroed arrays for clearer intent.
The try_into() calls on lines 219-222 and 224-227 will never fail since the slice lengths are guaranteed by the data.len() >= 64 check. The fallback to [0u8; 32] is effectively dead code.
While the subsequent ata_addr == *acc_pubkey check (line 231) provides a safety net, using expect or unwrap would better communicate that failure is impossible here:
- let mint =
- Pubkey::new_from_array(match data[0..32].try_into() {
- Ok(a) => a,
- Err(_) => [0u8; 32],
- });
- let wallet_owner =
- Pubkey::new_from_array(match data[32..64].try_into() {
- Ok(a) => a,
- Err(_) => [0u8; 32],
- });
+ let mint = Pubkey::new_from_array(
+ data[0..32].try_into().expect("slice is 32 bytes"),
+ );
+ let wallet_owner = Pubkey::new_from_array(
+ data[32..64].try_into().expect("slice is 32 bytes"),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If this is a delegated SPL Token ATA that was cloned from an eATA, | |
| // we should commit/undelegate the corresponding eATA instead. | |
| let mut target_pubkey = *acc_pubkey; | |
| let acc_borrow = acc.borrow(); | |
| if acc_borrow.delegated() | |
| && acc_borrow.owner() == &SPL_TOKEN_PROGRAM_ID | |
| { | |
| let data = acc_borrow.data(); | |
| if data.len() >= 64 { | |
| // spl-token Account layout: [0..32]=mint, [32..64]=owner | |
| let mint = | |
| Pubkey::new_from_array(match data[0..32].try_into() { | |
| Ok(a) => a, | |
| Err(_) => [0u8; 32], | |
| }); | |
| let wallet_owner = | |
| Pubkey::new_from_array(match data[32..64].try_into() { | |
| Ok(a) => a, | |
| Err(_) => [0u8; 32], | |
| }); | |
| // Verify that the current pubkey matches the derived ATA | |
| let ata_addr = derive_ata(&wallet_owner, &mint); | |
| if ata_addr == *acc_pubkey { | |
| // Remap to eATA PDA | |
| target_pubkey = derive_eata(&wallet_owner, &mint); | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduleCommit: remapping ATA {} -> eATA {} for commit/undelegate", | |
| acc_pubkey, | |
| target_pubkey | |
| ); | |
| } | |
| } | |
| } | |
| // If this is a delegated SPL Token ATA that was cloned from an eATA, | |
| // we should commit/undelegate the corresponding eATA instead. | |
| let mut target_pubkey = *acc_pubkey; | |
| let acc_borrow = acc.borrow(); | |
| if acc_borrow.delegated() | |
| && acc_borrow.owner() == &SPL_TOKEN_PROGRAM_ID | |
| { | |
| let data = acc_borrow.data(); | |
| if data.len() >= 64 { | |
| // spl-token Account layout: [0..32]=mint, [32..64]=owner | |
| let mint = Pubkey::new_from_array( | |
| data[0..32].try_into().expect("slice is 32 bytes"), | |
| ); | |
| let wallet_owner = Pubkey::new_from_array( | |
| data[32..64].try_into().expect("slice is 32 bytes"), | |
| ); | |
| // Verify that the current pubkey matches the derived ATA | |
| let ata_addr = derive_ata(&wallet_owner, &mint); | |
| if ata_addr == *acc_pubkey { | |
| // Remap to eATA PDA | |
| target_pubkey = derive_eata(&wallet_owner, &mint); | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduleCommit: remapping ATA {} -> eATA {} for commit/undelegate", | |
| acc_pubkey, | |
| target_pubkey | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs around
lines 208 to 242: the current code falls back to zeroed [0u8;32] on failed
try_into() even though data.len() >= 64 guarantees the slices are length 32;
replace the match+Err fallback with a direct .try_into().expect("slice length
32") (or .unwrap()) for both the mint and wallet_owner conversions to make the
impossibility explicit and remove dead fallback code, preserving the existing
logic and safety check on data.len().
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Needs similiar implementation for ScheduleBaseIntent case as it also contains committed accounts.
Also an interesting discussion:
Shall we implement such mappings on smart contract level or in "prepocessing" in ScheduledCommitsProcessorImpl?
| let mut account: Account = acc.borrow().to_owned().into(); | ||
| account.owner = parent_program_id.cloned().unwrap_or(account.owner); | ||
|
|
||
| // If this is a delegated SPL Token ATA that was cloned from an eATA, |
There was a problem hiding this comment.
I guess this also shall be implemented for ScheduleBaseIntent/ process_schedule_base_intent
There was a problem hiding this comment.
I'd suggest extracting this whole block in separate function
fn handle_spl_bla_bla(commted_account: &mut CommittedAccount) {
// in case this is a delegated SPL Token ATA
committed_account.pubkey = ata_addr
}
Same logic then could be reused both in process_schedule_commit /process_schedule_base_intent
| acc_pubkey, | ||
| target_pubkey | ||
| ); | ||
| } |
There was a problem hiding this comment.
What would else case mean here? Is that an error?
Description
Implements MIMD 0013
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.