Conversation
WalkthroughAdds a new CallHandlerV2 instruction variant, a deprecated legacy builder, a new instruction builder and size-budget helper, a processor implementation performing CPI-style delegation with escrow PDA validation, module re-exports, error constants, and extensive Rust/JS tests exercising finalize and undelegate flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/discriminator.rs`:
- Around line 46-47: The doc comment for the enum variant CallHandlerV2 in
discriminator.rs is missing a trailing period; update the comment text "See
[crate::processor::process_call_handler_v2] for docs" to end with a period so it
matches the style of other variant comments (i.e., "See
[crate::processor::process_call_handler_v2] for docs.").
In `@src/processor/call_handler_v2.rs`:
- Around line 80-87: The log message in the executable check for source_program
prints destination_program.key by mistake; update the msg! call inside the
non-executable branch (the block that returns
Err(ProgramError::InvalidAccountData)) to reference source_program.key instead
of destination_program.key so the error correctly identifies the source program.
- Around line 19-31: The doc comment misnumbers accounts (duplicate index 3) and
makes the PDA origin ambiguous; update the account index list so indices are
sequential and match the named accounts: 0 = `[signer]` validator, 1 = `[]`
validator fee vault, 2 = `[]` destination program, 3 = `[]` source program, 4 =
`[]` escrow_authority, 5 = `[writable]` non-delegated escrow PDA created from
escrow_authority (index 4), 6 = `[readonly/writable]` other accounts needed for
action, 7 = `[readonly/writable]` other accounts needed for action, etc.; also
replace the ambiguous phrase “escrow pda created from 3” with “escrow PDA
created from escrow_authority (index 4)” to keep the documentation consistent
with function semantics in call_handler_v2.
In `@tests/test_call_handler_v2.rs`:
- Around line 186-202: The delegation metadata is being created with the wrong
seed index (using ephemeral_balance_seeds_from_payer!(payer.pubkey(), 0)) while
the actual ephemeral balance PDA (ephemeral_balance_pda) was derived with index
1; update the seeds passed to create_delegation_metadata_data and the
program_test.add_account call so they use
ephemeral_balance_seeds_from_payer!(payer.pubkey(), 1) (or otherwise match the
index used to derive ephemeral_balance_pda) so
delegation_metadata_pda_from_delegated_account(&ephemeral_balance_pda) and
create_delegation_metadata_data(...) use the same seed index.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/integration/programs/test-delegation/src/lib.rs`:
- Around line 120-161: Both v2 handlers duplicate v1 logic; extract the shared
transfer logic into a private helper (e.g.,
perform_escrow_transfer(ctx_accounts: &T, amount: u64) or similar) and call it
from commit_base_action_handler_v2 and undelegate_base_action_handler_v2 to
remove verbatim duplication while preserving the extra counter update in
undelegate_base_action_handler_v2; ensure the helper accepts the context types
or account infos (used in Transfer creation) and that undelegate still
increments the Counter after calling the helper.
In `@tests/integration/tests/test-delegation.ts`:
- Around line 660-696: createCallHandlerV2Instruction relies on Buffer.alloc
zero-fill for bytes 1–7 of the 8-byte discriminator (only byte 0 is set), so
explicitly write the full 8-byte discriminator to the data buffer (e.g., use a
64-bit write like writeBigUInt64LE(BigInt(20), 0) or explicitly zero bytes 1–7
and set byte 0) before writing escrowIndex and lengths; also review the top-up
helper (the function that takes amount at line ~633) and consider using BigInt
for its amount parameter or clearly document/guard against
Number.MAX_SAFE_INTEGER to avoid silent precision loss.
- Around line 630-658: The discriminator for TopUpEphemeralBalance in
createTopUpEphemeralBalanceInstruction only writes the first byte and relies on
Buffer.alloc zero-fill; change it to the same explicit Buffer.from([...])
pattern used elsewhere by encoding all 8 discriminator bytes explicitly (instead
of data.writeUint8(9,0)), then append the u64 amount and u8 index bytes in order
so the final data buffer matches other instruction builders (keep references to
ephemeralBalancePdaFromPayer and DELEGATION_PROGRAM_ID intact).
src/processor/call_handler_v2.rs
Outdated
| use solana_program::pubkey::Pubkey; | ||
| use solana_program::{msg, system_program}; | ||
|
|
||
| pub const INVALID_ESCROW_PDA: &str = "invalid escrow pda in CallHandler"; |
There was a problem hiding this comment.
Errors should be defined in error.rs
src/processor/call_handler_v2.rs
Outdated
| const OTHER_ACCOUNTS_OFFSET: usize = 6; | ||
|
|
||
| if accounts.len() < OTHER_ACCOUNTS_OFFSET { | ||
| return Err(ProgramError::NotEnoughAccountKeys); |
There was a problem hiding this comment.
Seems an unnecessary check since it would fail in the below code
src/processor/call_handler_v2.rs
Outdated
| // verify signer is a registered validator | ||
| load_initialized_validator_fees_vault(validator, validator_fees_vault, true)?; | ||
| // Check if destination program is executable | ||
| if !destination_program.executable { |
There was a problem hiding this comment.
unnecessary since the CPI would fail anyways
There was a problem hiding this comment.
Removed from both call_handler and call_handler_v2
| return Err(ProgramError::InvalidAccountData); | ||
| } | ||
| // Check if source program is executable | ||
| if !source_program.executable { |
There was a problem hiding this comment.
this should be impossible per construction, also seems uneccesary
There was a problem hiding this comment.
We can, but here construction happens elsewhere, while in dlp v2 case the impossiblity by construction indeed works since it is all constructed in one place.
This will work as long as we're the only one who constructing it, which is true at the moment, but in future it could be constructed by any other validator.
| // deduce necessary accounts for CPI | ||
| let (accounts_meta, handler_accounts): (Vec<AccountMeta>, Vec<AccountInfo>) = other_accounts | ||
| .iter() | ||
| .chain([source_program, escrow_authority_account, escrow_account]) |
There was a problem hiding this comment.
source_program, in this instruction and also in the handler, could just be a pubkey passed trough the args. We don't need to pass the account. Is there a reason to pass the actual account?
There was a problem hiding this comment.
The reason source_program is passed as an account rather than a pubkey in the args is to preserve the integrator's instruction data (args.data) as an opaque, passthrough blob.
Currently, the integrator (e.g. a game program) schedules an action like:
let commit_action = FlexiCounterInstruction::CommitActionHandler { amount: PRIZE };
let data = to_vec(&commit_action);The dlp handler then forwards args.data verbatim as the instruction data to the destination program's. The destination handler deserializes it directly into its own expected format.
If we moved source_program into the args we'd have a problem: the DLP would need to inject the source_program pubkey into the user's instruction data before the CPI call. But DLP doesn't know the user's serialization format - it could be Borsh, it could be custom, it could be anything. We'd have 2 options here:
- Know the user's serialization layout - impossible since it's arbitrary per-program.
- Force the user into a wrapper struct like
struct MagicCallHandlerArgs { source_program: Pubkey, data: Vec<u8> }which means the destination handler would first have to deserialize the wrapper, then deserialize data again into its actual args. This adds complexity IMO for the User.
By passing source_program as an account the user's args.data stays untouched and opaque, dlp never needs to parse or modify it. The integrator just adds a source_program field to their Anchor accounts struct and then can utilize it or ignore it.
There was a problem hiding this comment.
Couldn't it just be a new field in ActionArgs ? Instruction data would still be opaque.
There was a problem hiding this comment.
Wouldn't you have to insert/serialize it into user supplied data? We don't call with ActionArgs really we call only with ActionArgs::data.
let handler_instruction = Instruction {
program_id: *destination_program.key,
data: args.data, // I think we would have to insert pubkey here
accounts: accounts_meta,
};What is benefit of passing source as argument?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/processor/call_handler_v2.rs`:
- Around line 66-73: Add the same explicit executability check for
destination_program as is done for source_program: verify
destination_program.executable, emit a clear msg! referencing
destination_program.key (e.g., "{} program is not executable: destination
program"), and return Err(ProgramError::InvalidAccountData) on failure so
callers get a specific early error instead of a generic CPI failure; locate this
change near the existing source_program.executable check in call_handler_v2.rs.
2 participants
Problem
What problem are you trying to solve?
The users must be able to get source program from where the action was scheduled
Solution
How did you solve the problem?
Before & After Screenshots
Insert screenshots of example code output
BEFORE:
[insert screenshot here]
AFTER:
[insert screenshot here]
Other changes (e.g. bug fixes, small refactors)
Deploy Notes
Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.
New scripts:
script: script detailsNew dependencies:
dependency: dependency detailsSummary by CodeRabbit