Open
Conversation
# Conflicts: # e-token/src/processor/process_transfer_queue_tick.rs
# Conflicts: # e-token-api/src/lib.rs # e-token/src/entrypoint.rs # e-token/src/processor/process_transfer_queue_tick.rs
# Conflicts: # e-token/src/processor/deposit_and_queue_transfer.rs # e-token/src/processor/process_transfer_queue_tick.rs
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds group-receipt state and end-to-end processing for transfer callbacks: new on-chain structs, controllers, CPI helpers to the magic program, processors to initialize and execute group receipts, transfer-queue wiring to create/schedule receipts, and extensive tests and test mocks. Also updates workspace deps and small formatting/visibility tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e-token-api/src/state/group_receipt.rs`:
- Around line 41-49: The function from_data_mut currently trusts fields parsed
from GroupReceiptHeader (like transfers_completed and splits) and returns a view
that can cause panics when items() or record_transfer() slice past items_data;
validate header-derived bounds before exposing the view by computing expected
items byte offsets/lengths from header.transfers_completed and header.splits and
ensuring they do not exceed items_data.len(), returning
ProgramError::InvalidAccountData on failure; apply the same bounds checks and
error handling to the other constructor(s)/parser(s) mentioned (the analogous
code around lines 57-93) so any malformed receipt yields InvalidAccountData
instead of panicking.
- Around line 176-177: The two size() helper functions (pub const fn size() ->
usize) call size_of::<Self>() but do not qualify or import it; fix by either
adding an import like use core::mem::size_of; (or use std::mem::size_of;) at the
top of the module or by fully qualifying the call as
core::mem::size_of::<Self>() (or std::mem::size_of::<Self>()), ensuring both
size() helpers reference the qualified/imported symbol so the file compiles.
In `@e-token/src/entrypoint.rs`:
- Line 1: Re-export process_initialize_group_receipt from the processor root so
imports are consistent: add a pub use for process_initialize_group_receipt
(matching how process_execute_transfer_callback is re-exported) in
processor/mod.rs, then remove the explicit use
crate::processor::initialize_group_receipt::process_initialize_group_receipt;
line from entrypoint.rs so entrypoint can rely on use crate::processor::* for
both processors.
In
`@e-token/src/processor/deposit_and_delegate_shuttle_ephemeral_ata_with_merge_and_private_transfer.rs`:
- Line 300: Update the incorrect inline comment on the
MaybeEncryptedAccountMeta::ClearText(compact::AccountMeta::new_readonly(10,
false)) entry to reflect that index 10 is the MAGIC_PROGRAM_ID (magic_program)
rather than shuttle_wallet_ata_info; locate this occurrence in
deposit_and_delegate_shuttle_ephemeral_ata_with_merge_and_private_transfer.rs
and replace the stale comment with one stating "magic_program" or
"MAGIC_PROGRAM_ID" so the account meta annotation matches the actual account
usage.
In `@e-token/src/processor/execute_transfer_callback.rs`:
- Around line 112-114: Replace the manual owner check on queue_info that returns
ProgramError::IllegalOwner with the crate-standard assertion: use the
assert_owner! macro to validate queue_info is owned by crate::ID (i.e., call
assert_owner!(queue_info, &crate::ID) or the crate's expected form) so that
owner mismatches consistently return ProgramError::InvalidAccountOwner; update
the block containing queue_info.owned_by(&crate::ID) accordingly.
- Around line 76-80: The file contains unguarded pinocchio_log::log! calls
(inside the execute_transfer_callback function and elsewhere in this module);
wrap every pinocchio_log::log! invocation and any multi-statement logging blocks
with the crate-wide logging feature guard (use #[cfg(feature = "logging")]
before single-line logs or #[cfg(feature = "logging")] { ... } for blocks) so
logging only compiles when the "logging" feature is enabled; update all
occurrences of the pinocchio_log::log! macro in this file to follow that pattern
and leave other logic unchanged.
In `@e-token/src/processor/utils/ephemeral_account.rs`:
- Around line 55-60: Update the doc comment for the function in
ephemeral_account.rs that currently begins "Creates an ephemeral account via the
magic program." to correctly describe that it "Resizes an ephemeral account via
the magic program" (this is a copy-paste from create_ephemeral_account); also
adjust the summary text and any brief description lines to say "Resizes" instead
of "Creates" and ensure the account references (sponsor, account, vault) and
their roles still match the resize behavior in the function (e.g., account must
be existing and have 0 lamports if applicable).
- Around line 15-16: Update the misleading comment above the
RESIZE_EPHEMERAL_VARIANT constant: it currently refers to
"CreateEphemeralAccount" but should state "ResizeEphemeralAccount" to match the
constant name and variant index; edit the comment text for
RESIZE_EPHEMERAL_VARIANT to correctly describe that it is the bincode variant
index for MagicBlockInstruction::ResizeEphemeralAccount (variant 13).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3aa62dc-67a2-4f41-bca6-deac342a3328
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcompare-metrics/src/main.rse-token-api/Cargo.tomle-token-api/src/lib.rse-token-api/src/state/group_receipt.rse-token-api/src/state/mod.rse-token/Cargo.tomle-token/src/entrypoint.rse-token/src/processor/deposit_and_delegate_shuttle_ephemeral_ata_with_merge_and_private_transfer.rse-token/src/processor/deposit_and_queue_transfer.rse-token/src/processor/ensure_transfer_queue_crank.rse-token/src/processor/execute_transfer_callback.rse-token/src/processor/initialize_group_receipt.rse-token/src/processor/lamports_pda.rse-token/src/processor/mod.rse-token/src/processor/process_transfer_queue_tick.rse-token/src/processor/shuttle_close_schedule.rse-token/src/processor/utils/ephemeral_account.rse-token/src/processor/utils/group_receipt_controller.rse-token/src/processor/utils/mod.rse-token/tests/rent_pda.rs
Comment on lines
+76
to
+80
| if !response.ok { | ||
| if let Ok(value) = core::str::from_utf8(response.error) { | ||
| pinocchio_log::log!("Action failed: {}", value); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== Cargo feature/dependency declarations =="
rg -n 'pinocchio-log|logging' --glob 'Cargo.toml' .
echo
echo "== Logging usage patterns in e-token/src/processor =="
rg -n -C2 'cfg\(feature = "logging"\)|pinocchio_log::log!' e-token/src/processorRepository: magicblock-labs/ephemeral-spl-token
Length of output: 18055
Standardize logging guards in this file to match the codebase pattern.
execute_transfer_callback.rs has unguarded pinocchio_log::log! calls at lines 76–80, 95–96, 147, and 177–198, while most other processors in e-token/src/processor wrap logging behind #[cfg(feature = "logging")]. Wrap all logging statements in this file with the feature guard for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e-token/src/processor/execute_transfer_callback.rs` around lines 76 - 80, The
file contains unguarded pinocchio_log::log! calls (inside the
execute_transfer_callback function and elsewhere in this module); wrap every
pinocchio_log::log! invocation and any multi-statement logging blocks with the
crate-wide logging feature guard (use #[cfg(feature = "logging")] before
single-line logs or #[cfg(feature = "logging")] { ... } for blocks) so logging
only compiles when the "logging" feature is enabled; update all occurrences of
the pinocchio_log::log! macro in this file to follow that pattern and leave
other logic unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e-token/tests/transfer_queue_automation.rs (1)
983-997: 🧹 Nitpick | 🔵 TrivialMatch the queue-crank schedule by content, not by
captured[1].This now passes only while exactly one unrelated
ScheduleTaskprecedes the recurring queue crank. The PR already shifted the index once; another scheduled task will break these tests again even if the queue-crank flow is still correct. Find the task whose instruction targetsinternal::PROCESS_TRANSFER_QUEUE_TICK(or the expected account tuple) before buildingscheduled_ix.Also applies to: 1154-1170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e-token/tests/transfer_queue_automation.rs` around lines 983 - 997, The test currently assumes the queue-crank ScheduleTask is at captured[1], which is brittle; instead locate the correct scheduled entry by scanning captured for the instruction that targets internal::PROCESS_TRANSFER_QUEUE_TICK (or matches the expected account tuple) and then build scheduled_ix from that found entry; update both occurrences (around scheduled_ix and the similar block at lines ~1154-1170) to use the discovered index rather than hardcoding captured[1], referencing the captured vector, Instruction construction, and internal::PROCESS_TRANSFER_QUEUE_TICK when locating the correct entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e-token/tests/execute_transfer_callback.rs`:
- Around line 158-167: The test fixture always preloads receipt with owner:
PROGRAM so handle_group_receipt never executes the code path guarded by
!group_receipt_info.owned_by(&crate::ID); update tests in
execute_transfer_callback.rs to add at least one case where the receipt account
is created/added with a non-PROGRAM owner (e.g., a different Pubkey) before
calling setup_context()/the test flow so the code path that partially
initializes the receipt when the callback wins the race is exercised; then
assert the resulting receipt header and items (the fields checked elsewhere in
the file after callback handling) match the expected partially-initialized
values to validate the callback-before-crank behavior.
---
Outside diff comments:
In `@e-token/tests/transfer_queue_automation.rs`:
- Around line 983-997: The test currently assumes the queue-crank ScheduleTask
is at captured[1], which is brittle; instead locate the correct scheduled entry
by scanning captured for the instruction that targets
internal::PROCESS_TRANSFER_QUEUE_TICK (or matches the expected account tuple)
and then build scheduled_ix from that found entry; update both occurrences
(around scheduled_ix and the similar block at lines ~1154-1170) to use the
discovered index rather than hardcoding captured[1], referencing the captured
vector, Instruction construction, and internal::PROCESS_TRANSFER_QUEUE_TICK when
locating the correct entry.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fce6639-70b6-44b4-9adb-a2dad4f72033
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
e-token/Cargo.tomle-token/tests/common/magic_mock.rse-token/tests/common/mod.rse-token/tests/deposit_and_queue_transfer.rse-token/tests/execute_transfer_callback.rse-token/tests/transfer_queue_automation.rs
Comment on lines
+67
to
+106
| fn captured_schedules() -> &'static Mutex<HashMap<Pubkey, Vec<CapturedScheduleTask>>> { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedScheduleTask>>>> = OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn captured_cancels() -> &'static Mutex<HashMap<Pubkey, Vec<CapturedCancelTask>>> { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedCancelTask>>>> = OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn captured_intent_bundles() -> &'static Mutex<HashMap<Pubkey, Vec<CapturedIntentBundle>>> { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedIntentBundle>>>> = OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn captured_action_callbacks() -> &'static Mutex<HashMap<Pubkey, Vec<CapturedActionCallback>>> { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedActionCallback>>>> = OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn captured_ephemeral_creates( | ||
| ) -> &'static Mutex<HashMap<Pubkey, Vec<CapturedCreateEphemeralAccount>>> { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedCreateEphemeralAccount>>>> = | ||
| OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn captured_ephemeral_resizes( | ||
| ) -> &'static Mutex<HashMap<Pubkey, Vec<CapturedResizeEphemeralAccount>>> { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedResizeEphemeralAccount>>>> = | ||
| OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn captured_ephemeral_closes() -> &'static Mutex<HashMap<Pubkey, Vec<CapturedCloseEphemeralAccount>>> | ||
| { | ||
| static S: OnceLock<Mutex<HashMap<Pubkey, Vec<CapturedCloseEphemeralAccount>>>> = | ||
| OnceLock::new(); | ||
| S.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } |
Contributor
There was a problem hiding this comment.
Captured Magic calls survive caller failure.
These stores are process-global, and process() pushes into them eagerly. If the outer instruction later returns an error, the recorded CPI still remains even though real CPI effects would roll back. That makes take_*/peek_* unreliable for negative-path assertions and can leak stale captures into later test steps unless every failure manually clears them. Please either snapshot/restore the stores around failed transactions or constrain this helper to success-path assertions only.
Also applies to: 223-341
Comment on lines
+158
to
+167
| pt.add_account( | ||
| receipt, | ||
| SolanaAccount { | ||
| lamports: rent.minimum_balance(receipt_data.len()), | ||
| data: receipt_data, | ||
| owner: PROGRAM, | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The callback-before-crank branch still isn't exercised.
setup_context() always preloads receipt with owner: PROGRAM, and every new test reuses that fixture. That means handle_group_receipt never hits the !group_receipt_info.owned_by(&crate::ID) path that creates/partially initializes the receipt when the callback wins the race. Please add one case with a non-PROGRAM receipt account and verify the resulting header/items after the callback.
Also applies to: 243-244, 298-299, 365-366, 429-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e-token/tests/execute_transfer_callback.rs` around lines 158 - 167, The test
fixture always preloads receipt with owner: PROGRAM so handle_group_receipt
never executes the code path guarded by
!group_receipt_info.owned_by(&crate::ID); update tests in
execute_transfer_callback.rs to add at least one case where the receipt account
is created/added with a non-PROGRAM owner (e.g., a different Pubkey) before
calling setup_context()/the test flow so the code path that partially
initializes the receipt when the callback wins the race is exercised; then
assert the resulting receipt header and items (the fields checked elsewhere in
the file after callback handling) match the expected partially-initialized
values to validate the callback-before-crank behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds feature of
GroupReceiptswhich accumulateTransferReceiptsconsisting of data like: amount, signature.GroupReceiptimplemented using crank & action callbacks. Crank - schedules receipt initialization, callback adds creates and addsTransferReceiptImplementation details
Due to race-condition between crank and callbacks receipt can be initialized in both of them. Since callback doesn't have information on number of
splitsit just appends data to existing Receipt, sort of like Vec allocating more memory once it runs out of capacity. Arriving crank fully initializesGroupReceiptfrom there it will be able to eventually be closed assplitsare now knownHandling of allocation, deserialization and so on is abstracted via
GroupReceiptControllerStructure of Receipts
Summary by CodeRabbit
New Features
Tests
Chores