Allow using custom discriminator #105
Conversation
WalkthroughRemoved the shared Context enum and external discriminator; CallHandler now forwards raw args.data and no longer prepends a discriminator. Tests and the example delegation program were refactored to use explicit commit and undelegate handlers with discriminator-prefixed payloads at the call site. Changes
Sequence Diagram(s)sequenceDiagram
participant Tests
participant Processor as Call Handler (processor)
participant Program as Delegation Test Program
rect rgb(220,240,255)
Note over Tests,Processor: Old flow (before changes)
Tests->>Processor: instruction(data = EXTERNAL_CALL_HANDLER_DISCRIMINATOR + payload)
Processor->>Program: invoke(program, data = discriminator + payload)
Program->>Program: dispatch via shared Context enum
end
rect rgb(240,230,220)
Note over Tests,Program: New flow (after changes)
Tests->>Program: commit_base_action_handler(data = COMMIT_DISCRIMINATOR + payload)
Program->>Program: direct commit handler (no enum)
Tests->>Program: undelegate_base_action_handler(data = UNDELEGATE_DISCRIMINATOR + payload)
Program->>Program: direct undelegate handler (no enum)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor/call_handler.rs (1)
116-123: Avoid heap alloc for signer seedsYou can avoid the
concat()allocation by passing a two-level slice directly:- let bump_slice = &[escrow_bump]; - let escrow_signer_seeds = [escrow_seeds, &[bump_slice]].concat(); - invoke_signed(&handler_instruction, &handler_accounts, &[&escrow_signer_seeds]) + let bump_slice: &[u8] = &[escrow_bump]; + let signer_seeds: [&[u8]; 1] = [bump_slice]; + // pass two groups of seeds: the PDA seeds and the bump + invoke_signed(&handler_instruction, &handler_accounts, &[escrow_seeds, &signer_seeds])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktests/buffers/test_delegation.sois excluded by!**/*.so
📒 Files selected for processing (5)
src/args/call_handler.rs(1 hunks)src/consts.rs(0 hunks)src/processor/call_handler.rs(1 hunks)tests/integration/programs/test-delegation/src/lib.rs(2 hunks)tests/test_call_handler.rs(6 hunks)
💤 Files with no reviewable changes (1)
- src/consts.rs
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_call_handler.rs (1)
src/discriminator.rs (1)
to_vec(40-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: install
🔇 Additional comments (6)
src/processor/call_handler.rs (1)
111-115: Pass-through CPI data looks correctMoving
args.datadirectly intoInstruction.datais correct and aligns with the new custom-discriminator model. No ownership/lifetime issues here.tests/test_call_handler.rs (3)
306-311: LGTM: payload assembled with discriminator + args
[COMMIT_HANDLER_DISCRIMINATOR, borsh(u64)]matches the new pass-through contract.
358-363: LGTM: undelegate payload format is correct
[UNDELEGATE_HANDLER_DISCRIMINATOR, borsh(u64)]is consistent with handler expectations.
459-464: LGTM: final undelegate call uses full payloadDiscriminator + prize matches the handler signature.
tests/integration/programs/test-delegation/src/lib.rs (2)
70-92: Confirm framework support for custom 4‑byte discriminatorsAnchor defaults to an 8‑byte discriminator. Verify that your macros/runtime (e.g.,
#[ephemeral]) actually route on this 4‑byte#[instruction(discriminator = …)]; otherwise CPI will fail to dispatch.
185-193: Accounts layout matches the testsOrder and mutability align with how
call_handlerassembles metas. No issues.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/entrypoint.rs(1 hunks)src/lib.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib.rs (1)
src/entrypoint.rs (1)
entrypoint(19-37)
src/entrypoint.rs (15)
src/processor/call_handler.rs (1)
process_call_handler(46-124)src/processor/fast/delegate.rs (1)
process_delegate(50-193)src/processor/fast/commit_state.rs (1)
process_commit_state(55-89)src/processor/fast/commit_state_from_buffer.rs (1)
process_commit_state_from_buffer(10-46)src/processor/fast/finalize.rs (1)
process_finalize(51-147)src/processor/fast/undelegate.rs (1)
process_undelegate(77-225)src/processor/init_validator_fees_vault.rs (1)
process_init_validator_fees_vault(33-82)src/processor/init_protocol_fees_vault.rs (1)
process_init_protocol_fees_vault(26-59)src/processor/validator_claim_fees.rs (1)
process_validator_claim_fees(28-83)src/processor/whitelist_validator_for_program.rs (1)
process_whitelist_validator_for_program(38-99)src/processor/top_up_ephemeral_balance.rs (1)
process_top_up_ephemeral_balance(30-81)src/processor/delegate_ephemeral_balance.rs (1)
process_delegate_ephemeral_balance(34-99)src/processor/close_ephemeral_balance.rs (1)
process_close_ephemeral_balance(27-76)src/processor/protocol_claim_fees.rs (1)
process_protocol_claim_fees(25-70)src/processor/close_validator_fees_vault.rs (1)
process_close_validator_fees_vault(28-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (3)
src/lib.rs (2)
16-17: Good modularization of entrypoint behind feature gate.This avoids emitting the entrypoint when building as a library and aligns with best practices for Solana programs.
3-3: Narrow import looks good.Using solana_program::declare_id directly is clearer and avoids unnecessary imports.
src/entrypoint.rs (1)
19-37: Entrypoint with fast-path and safe fallback looks solid.Double-deserialization tradeoff is acceptable; the flow cleanly falls back to slow_entrypoint when fast path opts out.
src/entrypoint.rs
Outdated
| use crate::discriminator::DlpDiscriminator; | ||
| use crate::processor::process_call_handler; | ||
| use crate::{discriminator, processor}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: unify DlpDiscriminator import usage.
You import both DlpDiscriminator and discriminator; then reference both styles (DlpDiscriminator::… and discriminator::DlpDiscriminator::…). Pick one for consistency; e.g., drop the direct type import and always use discriminator::DlpDiscriminator.
🤖 Prompt for AI Agents
In src/entrypoint.rs around lines 1 to 3, the file imports DlpDiscriminator
directly and also the discriminator module, then uses both DlpDiscriminator::…
and discriminator::DlpDiscriminator::…. Remove the direct type import (drop
DlpDiscriminator from the use list) and update all references to consistently
use discriminator::DlpDiscriminator::… (or alternatively remove the module
import and use the direct type everywhere) so the code uses a single import
style throughout.
taco-paco
force-pushed
the
feat/base-layer-ix/custom-discriminator
branch
from
c99dd94 to
e3a785a
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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)
tests/integration/programs/test-delegation/src/lib.rs (1)
189-200: Also mark escrow_account as mutable here.Same issue applies to undelegate; source lamports change.
#[derive(Accounts)] pub struct UndelegateBaseActionHandler<'info> { /// CHECK: The authority that owns the escrow account pub escrow_authority: UncheckedAccount<'info>, - pub escrow_account: Signer<'info>, + #[account(mut)] + pub escrow_account: Signer<'info>, /// CHECK: The destination account to transfer lamports to #[account(mut)] pub destination_account: AccountInfo<'info>, /// CHECK: fails in finalize stage due to ownership by dlp pub counter: UncheckedAccount<'info>, pub system_program: Program<'info, System>, }
♻️ Duplicate comments (1)
tests/integration/programs/test-delegation/src/lib.rs (1)
102-105: Counter serialization ignores Anchor’s 8-byte discriminator; length mismatch will panic.
- Counter account data = [8-byte Anchor discriminator || Counter bytes]. You’re deserializing from offset 0 and writing back only Counter bytes, so deserialization reads the wrong value and copy_from_slice will mismatch lengths (16 vs 8), causing a panic.
- Add owner/size checks and respect the 8-byte discriminator when reading/writing.
- let counter_data = &mut ctx.accounts.counter.try_borrow_mut_data()?; - let mut counter = Counter::try_from_slice(&counter_data)?; - counter.count += 1; - counter_data.copy_from_slice(&counter.try_to_vec()?); + const DISC_LEN: usize = 8; + let mut data = ctx.accounts.counter.try_borrow_mut_data()?; + // Optional but recommended: crisp failure instead of finalize-time error. + if data.len() < DISC_LEN + 8 { + return Err(ProgramError::InvalidAccountData.into()); + } + // Deserialize after discriminator. + let mut counter = Counter::try_from_slice(&data[DISC_LEN..])?; + counter.count += 1; + // Serialize back after discriminator. + let serialized = counter.try_to_vec()?; + data[DISC_LEN..DISC_LEN + serialized.len()].copy_from_slice(&serialized);If you prefer earlier, explicit failures (instead of finalize-stage), also verify ownership before mutating:
- if ctx.accounts.counter.owner != &crate::id() { return Err(ProgramError::IllegalOwner.into()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/integration/programs/test-delegation/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
| pub struct CommitBaseActionHandler<'info> { | ||
| /// CHECK: The authority that owns the escrow account | ||
| pub escrow_authority: UncheckedAccount<'info>, | ||
| pub escrow_account: Signer<'info>, | ||
| /// CHECK: The destination account to transfer lamports to | ||
| #[account(mut)] | ||
| pub destination_account: AccountInfo<'info>, | ||
| pub system_program: Program<'info, System>, | ||
| } |
There was a problem hiding this comment.
escrow_account must be mutable (lamports change on transfer).
Without #[account(mut)], Anchor will error at finalize when lamports decrease on the source account.
pub struct CommitBaseActionHandler<'info> {
/// CHECK: The authority that owns the escrow account
pub escrow_authority: UncheckedAccount<'info>,
- pub escrow_account: Signer<'info>,
+ #[account(mut)]
+ pub escrow_account: Signer<'info>,
/// CHECK: The destination account to transfer lamports to
#[account(mut)]
pub destination_account: AccountInfo<'info>,
pub system_program: Program<'info, System>,
}📝 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.
| pub struct CommitBaseActionHandler<'info> { | |
| /// CHECK: The authority that owns the escrow account | |
| pub escrow_authority: UncheckedAccount<'info>, | |
| pub escrow_account: Signer<'info>, | |
| /// CHECK: The destination account to transfer lamports to | |
| #[account(mut)] | |
| pub destination_account: AccountInfo<'info>, | |
| pub system_program: Program<'info, System>, | |
| } | |
| pub struct CommitBaseActionHandler<'info> { | |
| /// CHECK: The authority that owns the escrow account | |
| pub escrow_authority: UncheckedAccount<'info>, | |
| #[account(mut)] | |
| pub escrow_account: Signer<'info>, | |
| /// CHECK: The destination account to transfer lamports to | |
| #[account(mut)] | |
| pub destination_account: AccountInfo<'info>, | |
| pub system_program: Program<'info, System>, | |
| } |
🤖 Prompt for AI Agents
In tests/integration/programs/test-delegation/src/lib.rs around lines 179 to
187, the CommitBaseActionHandler struct's escrow_account is missing the
#[account(mut)] attribute so Anchor will fail when its lamports change; add
#[account(mut)] directly above the pub escrow_account: Signer<'info> field
(keeping the existing CHECK comments and other attributes intact) so the account
is marked mutable for transfers.
Users have bad UX with hardcoded discriminator in dlp. Right now users have to define a fixed entrypoint with fixed discriminator. This is cumbersome and not flexible. This pr adapts changes made in dlp [PR](magicblock-labs/delegation-program#105) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Refactor - Reworked scheduling/handler flow: replaced redelegation-intent path with explicit commit and undelegate action handlers. - Removed legacy redelegation payload types and simplified call dispatch. - Eliminated unused context data from task payloads. - Tests - Updated tests to match new action shapes; redelegation intent test path disabled. - Chores - Updated workspace dependency revisions (no feature changes). <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Problem
What problem are you trying to solve?
Bad UX with hardcoded discriminator.
Right now users have to define a fixed entrypoint with discriminator. This is cumbersome and not flexible
Solution
How did you solve the problem?
We allow users to just pass instruction data, where they could specify in whatever format valid instruction.
The code becomes very simple using anchor, instruction data could be derived using existing native anchor functionality, like:
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