feat: refactor delegation program instructions for both kit and web3js #77
Conversation
WalkthroughThe PR adds a new Rust procedural-macro crate (action-attribute), integrates it into the workspace and SDK exports, updates Solana dependency constraints, replaces handwritten TypeScript delegation instruction implementations with Solita-generated code, adds PDA helpers and tests, and updates CI to publish the new crate. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer code
participant RustMacro as action-attribute proc-macro
participant RustCompiler as Rust compiler
participant SDK as rust/sdk (anchor exports)
participant Solita as solita generator
participant TSIndex as ts/* index exports
Note over Dev,RustMacro: Compile-time struct augmentation
Dev->>RustCompiler: compile crate using #[action] on struct
RustCompiler->>RustMacro: invoke macro with TokenStream input
RustMacro-->>RustCompiler: return TokenStream (struct with injected escrow fields if missing)
RustCompiler->>SDK: compiled crate re-export available under feature "anchor"
Note over Solita,TSIndex: JS generation & export replacement
Solita->>TSIndex: generate delegation instruction modules into ./generated/...
TSIndex-->>Developer: index re-exports point to generated modules (delegate removed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (12)
rust/Cargo.lockis excluded by!**/*.lockts/kit/src/generated/README.mdis excluded by!**/generated/**ts/kit/src/generated/delegation-program-instructions/closeEphemeralBalance.tsis excluded by!**/generated/**ts/kit/src/generated/delegation-program-instructions/delegate.tsis excluded by!**/generated/**ts/kit/src/generated/delegation-program-instructions/index.tsis excluded by!**/generated/**ts/kit/src/generated/delegation-program-instructions/topUpEphemeralBalance.tsis excluded by!**/generated/**ts/kit/yarn.lockis excluded by!**/yarn.lock,!**/*.lockts/web3js/src/generated/README.mdis excluded by!**/generated/**ts/web3js/src/generated/delegation-program-instructions/closeEphemeralBalance.tsis excluded by!**/generated/**ts/web3js/src/generated/delegation-program-instructions/delegate.tsis excluded by!**/generated/**ts/web3js/src/generated/delegation-program-instructions/index.tsis excluded by!**/generated/**ts/web3js/src/generated/delegation-program-instructions/topUpEphemeralBalance.tsis excluded by!**/generated/**
📒 Files selected for processing (18)
.github/workflows/publish-crates-and-sdk.yml(3 hunks)rust/Cargo.toml(3 hunks)rust/action-attribute/Cargo.toml(1 hunks)rust/action-attribute/README.md(1 hunks)rust/action-attribute/src/lib.rs(1 hunks)rust/sdk/Cargo.toml(2 hunks)rust/sdk/src/anchor.rs(1 hunks)ts/kit/package.json(0 hunks)ts/kit/src/__test__/generated.test.ts(1 hunks)ts/kit/src/index.ts(1 hunks)ts/kit/src/instructions/delegate.ts(0 hunks)ts/kit/src/pda.ts(1 hunks)ts/web3js/.solitarc.js(1 hunks)ts/web3js/package.json(1 hunks)ts/web3js/src/__test__/generated.test.ts(1 hunks)ts/web3js/src/index.ts(1 hunks)ts/web3js/src/instructions/delegate.ts(0 hunks)ts/web3js/src/pda.ts(1 hunks)
💤 Files with no reviewable changes (3)
- ts/kit/package.json
- ts/kit/src/instructions/delegate.ts
- ts/web3js/src/instructions/delegate.ts
🧰 Additional context used
🧬 Code graph analysis (5)
rust/sdk/src/anchor.rs (1)
rust/action-attribute/src/lib.rs (1)
action(8-64)
ts/kit/src/pda.ts (1)
ts/web3js/src/pda.ts (1)
escrowPdaFromEscrowAuthority(33-44)
ts/web3js/src/pda.ts (3)
ts/kit/src/pda.ts (1)
escrowPdaFromEscrowAuthority(53-66)ts/kit/src/constants.ts (1)
DELEGATION_PROGRAM_ID(3-5)ts/web3js/src/constants.ts (1)
DELEGATION_PROGRAM_ID(3-5)
ts/kit/src/__test__/generated.test.ts (3)
ts/kit/src/generated/delegation-program-instructions/delegate.ts (2)
createDelegateInstruction(22-72)serializeDelegateInstructionData(74-118)ts/kit/src/generated/delegation-program-instructions/topUpEphemeralBalance.ts (2)
createTopUpEphemeralBalanceInstruction(21-56)serializeTopUpEphemeralBalanceInstructionData(58-79)ts/kit/src/generated/delegation-program-instructions/closeEphemeralBalance.ts (2)
createCloseEphemeralBalanceInstruction(20-50)serializeCloseEphemeralBalanceInstructionData(52-68)
ts/web3js/src/__test__/generated.test.ts (3)
ts/web3js/src/generated/delegation-program-instructions/delegate.ts (2)
createDelegateInstruction(17-51)serializeDelegateInstructionData(53-92)ts/web3js/src/generated/delegation-program-instructions/topUpEphemeralBalance.ts (2)
createTopUpEphemeralBalanceInstruction(16-44)serializeTopUpEphemeralBalanceInstructionData(46-66)ts/web3js/src/generated/delegation-program-instructions/closeEphemeralBalance.ts (2)
createCloseEphemeralBalanceInstruction(15-37)serializeCloseEphemeralBalanceInstructionData(39-55)
🪛 markdownlint-cli2 (0.18.1)
rust/action-attribute/README.md
6-6: Bare URL used
(MD034, no-bare-urls)
⏰ 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 (11)
ts/kit/src/index.ts (1)
1-7: LGTM! Export path correctly updated for generated instructions.The export path has been updated to use the generated delegation program instructions, which aligns with the PR's goal of using Solita-generated code.
ts/web3js/src/index.ts (1)
2-2: LGTM! Export path correctly updated for generated instructions.The export path has been updated to use the generated delegation program instructions, which aligns with the PR's goal of using Solita-generated code.
ts/kit/src/__test__/generated.test.ts (1)
1-150: Excellent test coverage for generated instructions!The test suite comprehensively validates:
- Instruction construction with correct account counts
- Discriminator values for each instruction type
- Field serialization at correct offsets (commitFrequencyMs, amount, index)
- Optional field handling (validator)
- Data length constraints
This provides strong confidence in the generated instruction serialization.
ts/web3js/src/__test__/generated.test.ts (1)
1-151: Strong test coverage for web3.js generated instructions.The test suite provides comprehensive validation of instruction construction, serialization, discriminators, and field encoding. The tests mirror the kit implementation tests, ensuring consistency across both packages.
ts/web3js/package.json (1)
24-24: The Solita dependency version is up-to-date and secure.The npm package @metaplex-foundation/solita was scanned for known vulnerabilities and missing license, and no issues were found. The version specified in the PR (
^0.20.1) matches the latest available version as of November 21, 2025.rust/action-attribute/Cargo.toml (1)
1-16: LGTM!The procedural macro crate configuration is correct. The workspace-managed metadata, appropriate dependencies (syn with "full" feature and quote), and proc-macro library configuration are all properly set up.
.github/workflows/publish-crates-and-sdk.yml (1)
82-82: LGTM!The new cargo publish step for action-attribute is correctly integrated into the workflow, following the same pattern as other crates (ephemeral, delegate, commit-attribute) with appropriate DRY_RUN flag handling.
rust/sdk/Cargo.toml (1)
35-35: LGTM!The workspace dependency on
ephemeral-rollups-sdk-attribute-actionis correctly added, following the same pattern as other attribute crates.rust/sdk/src/anchor.rs (1)
7-8: LGTM!The re-export of the
actionmacro follows the established pattern for other attribute macros (ephemeral,commit,delegate) and is correctly feature-gated underanchor.rust/Cargo.toml (2)
7-7: LGTM!The workspace member
action-attributeand corresponding workspace dependencyephemeral-rollups-sdk-attribute-actionare correctly configured, following the same pattern as other attribute crates in the workspace.Also applies to: 30-30
57-57: Verify the tightened version constraints for Solana dependencies.The version constraints have been made more restrictive:
solana-program:>=1.16→>=1.16, <3.0.0solana-pubkey:>=2→>=2, <=2.3These changes limit the upper bounds, which may be intentional for compatibility reasons. Ensure this aligns with your project's Solana version compatibility requirements and won't block users from upgrading to newer compatible versions.
If this change is related to specific compatibility issues, consider documenting the reasoning in a comment or commit message.
Also applies to: 61-61
| Procedural macro attribute for committing accounts in Ephemeral Rollups. | ||
|
|
||
| Quickstart and integration guide: | ||
| https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Format the URL as a proper markdown link.
The bare URL should be formatted as a markdown link for better readability and to comply with markdown best practices.
Apply this diff to fix the formatting:
-https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart
+[Quickstart and integration guide](https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart)Based on static analysis hints.
📝 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.
| https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart | |
| [Quickstart and integration guide](https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In rust/action-attribute/README.md around line 6 replace the bare URL
"https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart"
with a proper markdown link by wrapping it in square brackets for link text and
parentheses for the URL (e.g.
[Quickstart](https://docs.magicblock.gg/pages/get-started/how-integrate-your-program/quickstart))
so the README displays a formatted clickable link.
| pub fn action(_attr: TokenStream, item: TokenStream) -> TokenStream { | ||
| let input = parse_macro_input!(item as ItemStruct); | ||
|
|
||
| let name = &input.ident; | ||
| let attrs = &input.attrs; // Capture all attributes | ||
| let expanded = if let Fields::Named(fields_named) = &input.fields { | ||
| let mut has_escrow_auth = false; | ||
| let mut has_escrow = false; | ||
|
|
||
| for field in &fields_named.named { | ||
| if let Some(ident) = &field.ident { | ||
| if ident == "escrow_auth" { | ||
| has_escrow_auth = true; | ||
| } else if ident == "escrow" { | ||
| has_escrow = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut new_fields = fields_named.named.clone(); | ||
|
|
||
| if !has_escrow_auth { | ||
| new_fields.push( | ||
| Field::parse_named | ||
| .parse2(quote! { | ||
| /// CHECK: Escrow Authority is an account used to derive `escrow` with `escrow_index`, it is used to verify that action is scheduled with expected authority | ||
| pub escrow_auth: UncheckedAccount<'info> | ||
| }) | ||
| .unwrap(), | ||
| ); | ||
| } | ||
|
|
||
| if !has_escrow { | ||
| new_fields.push( | ||
| Field::parse_named | ||
| .parse2(quote! { | ||
| /// CHECK: Escrow account that is a `signer` in callback, it is derived from `escrow_auth` and `escrow_index` one specified in `ActionArgs` | ||
| pub escrow: UncheckedAccount<'info> | ||
| }) | ||
| .unwrap(), | ||
| ); | ||
| } | ||
|
|
||
| quote! { | ||
| #(#attrs)* | ||
| pub struct #name<'info> { | ||
| #new_fields | ||
| } | ||
| } | ||
| } else { | ||
| quote! { | ||
| compile_error!("Action attribute can only be used with structs with named fields"); | ||
| } | ||
| }; | ||
|
|
||
| TokenStream::from(expanded) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify lifetime parameter existence and UncheckedAccount availability.
The macro always generates a struct with the <'info> lifetime (line 53) and uses UncheckedAccount<'info> (lines 34, 45) without verifying:
- The input struct actually has an
'infolifetime parameter - The
UncheckedAccounttype is available in scope
If users apply this macro to a struct without <'info> or in a context without Anchor imports, they'll get confusing compiler errors.
Consider these improvements:
- Verify the input struct has the
'infolifetime parameter and emit a clear compile_error if missing - Add documentation clarifying this macro requires Anchor's
UncheckedAccounttype in scope
Run this script to check if there are any usages of this macro to understand the expected context:
🏁 Script executed:
#!/bin/bash
# Search for usages of the #[action] attribute macro
rg -n -B3 -A3 '#\[action\]' --type=rustLength of output: 0
I need to gather more context since the macro usage search produced no results. Let me explore the crate structure and verify the validity of the concerns.
🏁 Script executed:
#!/bin/bash
# 1. Check Cargo.toml to understand crate type
echo "=== Cargo.toml ==="
cat Cargo.toml 2>/dev/null || echo "Cargo.toml not found"
echo -e "\n=== Directory structure ==="
find . -type f -name "*.rs" | head -20
echo -e "\n=== Search for test usages of action macro ==="
rg -n "action" --type=rust | grep -i "test\|example" | head -10
echo -e "\n=== Look for UncheckedAccount imports ==="
rg -n "UncheckedAccount" --type=rust
echo -e "\n=== Check if there are examples or integration tests ==="
find . -type d -name "tests" -o -name "examples"Length of output: 1225
Let me examine the full lib.rs file and check for validation logic and documentation:
🏁 Script executed:
#!/bin/bash
# View complete lib.rs to see full implementation and any validation
echo "=== Full lib.rs content ==="
cat git/rust/action-attribute/src/lib.rs
echo -e "\n=== Check parent Cargo.toml for crate metadata ==="
find git/rust -name "Cargo.toml" -type f | xargs head -15
echo -e "\n=== Search for 'info lifetime in macro expansion ==="
rg -n "'info" --type=rust git/rust/action-attribute/
echo -e "\n=== Check for any generic lifetime parsing ==="
rg -n "Generics\|generics\|lifetimes" --type=rust git/rust/action-attribute/Length of output: 428
Let me establish the correct working directory first:
🏁 Script executed:
#!/bin/bash
# Find the actual working directory and locate lib.rs
echo "=== Current directory ==="
pwd
echo -e "\n=== List current contents ==="
ls -la | head -20
echo -e "\n=== Find lib.rs ==="
find . -name "lib.rs" -path "*/action-attribute/*" 2>/dev/null
echo -e "\n=== All Rust files matching action-attribute ==="
find . -path "*action-attribute*" -name "*.rs" 2>/dev/nullLength of output: 1001
🏁 Script executed:
#!/bin/bash
# View full lib.rs
echo "=== Full lib.rs content ==="
cat ./rust/action-attribute/src/lib.rs
echo -e "\n=== Check Cargo.toml for this crate ==="
cat ./rust/action-attribute/Cargo.toml 2>/dev/null || echo "No Cargo.toml in action-attribute"
echo -e "\n=== Search for validation of generics/lifetimes ==="
rg -n "Generics\|generics\|lifetime" --type=rust ./rust/action-attribute/Length of output: 2642
Add validation for 'info lifetime parameter and document UncheckedAccount requirement.
The macro unconditionally generates pub struct #name<'info> (line 53) and uses UncheckedAccount<'info> (lines 34, 45) without verification. This creates two issues:
- Lifetime mismatch risk: If a user applies
#[action]to a struct with existing generics (e.g.,struct Foo<'a>), the macro overwrites them with<'info>, causing silent compilation failures or semantic errors. - Missing type availability check: The macro generates
UncheckedAccount<'info>without verifying it's in scope. Users outside Anchor contexts will get confusing compiler errors.
Suggested fixes:
- Check
input.genericsbefore adding<'info>; emitcompile_error!if lifetimes already exist - Add doc comments clearly stating this macro requires
UncheckedAccountfrom Anchor in scope
| new_fields.push( | ||
| Field::parse_named | ||
| .parse2(quote! { | ||
| /// CHECK: Escrow Authority is an account used to derive `escrow` with `escrow_index`, it is used to verify that action is scheduled with expected authority | ||
| pub escrow_auth: UncheckedAccount<'info> | ||
| }) | ||
| .unwrap(), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider handling parse failure more gracefully.
While .unwrap() in procedural macros becomes a compile-time panic (which is acceptable), it produces a less informative error message. The same applies to line 47.
Consider using .expect() with a descriptive message:
new_fields.push(
Field::parse_named
.parse2(quote! {
/// CHECK: Escrow Authority is an account used to derive `escrow` with `escrow_index`, it is used to verify that action is scheduled with expected authority
pub escrow_auth: UncheckedAccount<'info>
})
- .unwrap(),
+ .expect("Failed to parse escrow_auth field"),
);Apply a similar change at line 47 for the escrow field.
📝 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.
| new_fields.push( | |
| Field::parse_named | |
| .parse2(quote! { | |
| /// CHECK: Escrow Authority is an account used to derive `escrow` with `escrow_index`, it is used to verify that action is scheduled with expected authority | |
| pub escrow_auth: UncheckedAccount<'info> | |
| }) | |
| .unwrap(), | |
| ); | |
| new_fields.push( | |
| Field::parse_named | |
| .parse2(quote! { | |
| /// CHECK: Escrow Authority is an account used to derive `escrow` with `escrow_index`, it is used to verify that action is scheduled with expected authority | |
| pub escrow_auth: UncheckedAccount<'info> | |
| }) | |
| .expect("Failed to parse escrow_auth field"), | |
| ); |
🤖 Prompt for AI Agents
In rust/action-attribute/src/lib.rs around lines 30 to 37 (and also update the
similar occurrence at line 47), the Field::parse_named.parse2(...).unwrap()
calls should be changed to use .expect(...) with a descriptive message; replace
each .unwrap() so that if parsing fails the proc-macro emits a clear, contextual
error (e.g., "failed to parse generated field 'escrow_auth'" and similarly for
the 'escrow' field), ensuring the messages identify which generated field failed
to parse and include intent so debugging is easier.
| magicblock-delegation-program = { version = "1.1.3", features = ["no-entrypoint", "core"], default-features = false } | ||
| # magicblock-delegation-program = { version = "1.1.3", features = ["no-entrypoint", "sdk"], default-features = false } // updating from "core" to "sdk" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the commented-out configuration.
The commented line suggests a potential change from "core" to "sdk" features for magicblock-delegation-program. This appears in multiple files (also rust/sdk/Cargo.toml line 48). If this is a planned change, consider creating a tracking issue. If it's no longer relevant, remove the comments to avoid confusion.
Do you want me to open a tracking issue to track this potential feature switch across the codebase?
| magicblock-delegation-program = { workspace = true, features = ["no-entrypoint", "core"], default-features = false } | ||
| # magicblock-delegation-program = { workspace = true, features = ["no-entrypoint", "sdk"], default-features = false } // updating from "core" to "sdk" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the commented-out configuration.
The commented line suggests a potential change from "core" to "sdk" features for magicblock-delegation-program. If this is a planned change, consider creating a tracking issue. If it's no longer relevant, remove the comment to avoid confusion.
Do you want me to open a tracking issue for this potential feature switch?
| /** | ||
| * Derives the escrow PDA from a payer address | ||
| * @param payer The payer address | ||
| * @param index The index of the ephemeral balance account | ||
| * @param programId The delegation program ID | ||
| * @returns The derived ephemeral balance PDA | ||
| */ |
There was a problem hiding this comment.
Fix JSDoc parameter name mismatch.
The JSDoc refers to the parameter as payer (line 48), but the actual parameter name is escrowAuthority (line 54). Update the JSDoc to match the implementation.
Apply this diff:
/**
* Derives the escrow PDA from a payer address
- * @param payer The payer address
+ * @param escrowAuthority The escrow authority address
* @param index The index of the ephemeral balance account
- * @param programId The delegation program ID
* @returns The derived ephemeral balance PDA
*/📝 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.
| /** | |
| * Derives the escrow PDA from a payer address | |
| * @param payer The payer address | |
| * @param index The index of the ephemeral balance account | |
| * @param programId The delegation program ID | |
| * @returns The derived ephemeral balance PDA | |
| */ | |
| /** | |
| * Derives the escrow PDA from a payer address | |
| * @param escrowAuthority The escrow authority address | |
| * @param index The index of the ephemeral balance account | |
| * @returns The derived ephemeral balance PDA | |
| */ |
🤖 Prompt for AI Agents
In ts/kit/src/pda.ts around lines 46 to 52, the JSDoc parameter name for the
first arg is incorrect: it documents `@param payer` but the actual function
parameter is `escrowAuthority`; update the JSDoc to use `@param escrowAuthority`
(and keep the descriptions for `index` and `programId` unchanged) so the doc
matches the implementation.
| const path = require('path'); | ||
|
|
||
| module.exports = { | ||
| idlUrl: 'https://raw.githubusercontent.com/magicblock-labs/delegation-program/main/idl/delegation.json', | ||
| programId: 'DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh', | ||
| idlGenerator: 'anchor', | ||
| accountsDir: path.join(__dirname, 'src/generated/delegation-program-instructions'), | ||
| instructionsDir: path.join(__dirname, 'src/generated/delegation-program-instructions'), | ||
| typesDir: path.join(__dirname, 'src/generated/delegation-program-instructions'), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider pinning IDL to a specific commit for reproducible builds.
The idlUrl points to the main branch (line 4), which means the IDL can change without notice, potentially breaking builds or causing inconsistencies across environments. Consider pinning to a specific commit or tag for reproducible builds.
Example:
- idlUrl: 'https://raw.githubusercontent.com/magicblock-labs/delegation-program/main/idl/delegation.json',
+ idlUrl: 'https://raw.githubusercontent.com/magicblock-labs/delegation-program/<commit-hash>/idl/delegation.json',Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ts/web3js/.solitarc.js around lines 1-10, the idlUrl currently points to the
repository's main branch which is mutable; replace the branch reference with a
specific commit SHA or tag in the raw.githubusercontent.com URL (e.g.
.../delegation-program/<COMMIT_SHA>/idl/delegation.json) so the IDL is pinned to
an exact revision, verify the commit/tag exists, and update any docs/CI that
assume automatic updates when you intentionally bump the pinned SHA.
|
|
||
| expect(instruction.keys).toHaveLength(7); | ||
| expect(instruction.data).toBeDefined(); | ||
| expect(instruction.programId.equals(instruction.programId)).toBe(true); |
There was a problem hiding this comment.
Fix pointless assertion that always passes.
Line 38 tests instruction.programId.equals(instruction.programId), which always returns true. This should verify against the expected DELEGATION_PROGRAM_ID.
Apply this diff:
+import { DELEGATION_PROGRAM_ID } from "../constants.js";
- expect(instruction.programId.equals(instruction.programId)).toBe(true);
+ expect(instruction.programId.equals(DELEGATION_PROGRAM_ID)).toBe(true);📝 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.
| expect(instruction.programId.equals(instruction.programId)).toBe(true); | |
| import { DELEGATION_PROGRAM_ID } from "../constants.js"; | |
| expect(instruction.programId.equals(DELEGATION_PROGRAM_ID)).toBe(true); |
🤖 Prompt for AI Agents
In ts/web3js/src/__test__/generated.test.ts around line 38 the test currently
asserts instruction.programId.equals(instruction.programId) which is a
tautology; change the assertion to compare instruction.programId against the
expected DELEGATION_PROGRAM_ID (e.g.,
instruction.programId.equals(DELEGATION_PROGRAM_ID) or an equivalent equality
check). Ensure DELEGATION_PROGRAM_ID is imported or referenced in the test file
and use that constant in the expect(...) call.
| export function escrowPdaFromEscrowAuthority( | ||
| escrowAuthority: PublicKey, | ||
| index: number = 255, | ||
| ) { | ||
| if (index < 0 || index > 255) { | ||
| throw new Error("Index must be between 0 and 255"); | ||
| } | ||
| return PublicKey.findProgramAddressSync( | ||
| [Buffer.from("balance"), escrowAuthority.toBytes(), Buffer.from([index])], | ||
| DELEGATION_PROGRAM_ID, | ||
| )[0]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Inconsistent PDA derivation across kit and web3js packages.
The escrowPdaFromEscrowAuthority function uses DELEGATION_PROGRAM_ID as the program address for PDA derivation (Line 42), but the parallel implementation in ts/kit/src/pda.ts (line 62) uses escrowAuthority as the programAddress. This inconsistency will produce different PDAs for identical inputs, breaking cross-package compatibility.
Verify which program address is correct according to the delegation program's on-chain implementation.
🏁 Script executed:
#!/bin/bash
# Check the delegation program IDL and related code for the correct PDA seeds/program
# Fetch the IDL to verify the escrow PDA derivation
curl -s https://raw.githubusercontent.com/magicblock-labs/delegation-program/main/idl/delegation.json | jq '.accounts[] | select(.name | contains("balance") or contains("escrow") or contains("Escrow") or contains("Balance"))'
# Search for any existing usage of escrowPdaFromEscrowAuthority in tests or examples
rg -n -C5 'escrowPdaFromEscrowAuthority' --type=ts
# Search for "balance" seed usage patterns
rg -n -C3 'Buffer\.from.*"balance"' --type=tsLength of output: 1965
I need to gather more context to verify the correct PDA derivation pattern. Let me search for the DELEGATION_PROGRAM_ID definition and examine the complete implementations.Now let me search for the code files to see the full context of both implementations:Let me check the code files directly to understand the full context and see what is actually being used:Based on my verification through the shell script output and research, the critical issue identified in the review comment has been confirmed:
Critical: Inconsistent PDA derivation across kit and web3js packages.
The shell script output clearly shows the discrepancy:
ts/web3js/src/pda.ts(line 41-42): UsesDELEGATION_PROGRAM_IDas the program address for PDA derivationts/kit/src/pda.ts(line 62-63): UsesescrowAuthorityas the program address
Both functions use the same seed structure (["balance", escrowAuthority.toBytes(), index]), but derive from different program addresses. This is a fundamental problem because PDAs are deterministically derived from both the seeds and the program address. When these don't match, the resulting PDA addresses will be different, breaking cross-package compatibility.
More critically, using escrowAuthority (which is a user's account) as a program address in PDA derivation is cryptographically incorrect—the program address parameter must be an actual Solana program ID, not an arbitrary account address.
ts/kit/src/pda.ts (line 62): Change programAddress: escrowAuthority to programAddress: DELEGATION_PROGRAM_ID
🤖 Prompt for AI Agents
In ts/web3js/src/pda.ts around lines 33 to 44: the PDA derivation currently uses
DELEGATION_PROGRAM_ID but the kit implementation used escrowAuthority as the
program address, causing inconsistent PDAs and an incorrect use of a user
account as a program id; update the kit implementation to use
DELEGATION_PROGRAM_ID (the actual program id) as the program address for the
same seed array so both packages derive identical PDAs, keep the same seed order
and index validation, and ensure any references/imports to DELEGATION_PROGRAM_ID
are present and correct.
… defaulting to 255
…l with default 255
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ts/web3js/src/__test__/generated.test.ts (1)
38-38: Fix tautological assertion.Line 38 tests
instruction.programId.equals(instruction.programId), which always returnstrue. This should verify against the expectedDELEGATION_PROGRAM_IDconstant.Apply this diff:
+import { DELEGATION_PROGRAM_ID } from "../constants.js"; + - expect(instruction.programId.equals(instruction.programId)).toBe(true); + expect(instruction.programId.equals(DELEGATION_PROGRAM_ID)).toBe(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
ts/kit/src/generated/delegation-program-instructions/closeEphemeralBalance.tsis excluded by!**/generated/**ts/kit/src/generated/delegation-program-instructions/topUpEphemeralBalance.tsis excluded by!**/generated/**ts/web3js/src/generated/delegation-program-instructions/closeEphemeralBalance.tsis excluded by!**/generated/**ts/web3js/src/generated/delegation-program-instructions/topUpEphemeralBalance.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
ts/kit/src/__test__/generated.test.ts(1 hunks)ts/web3js/src/__test__/generated.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ts/web3js/src/__test__/generated.test.ts (4)
ts/kit/src/generated/delegation-program-instructions/delegate.ts (2)
createDelegateInstruction(22-72)serializeDelegateInstructionData(74-118)ts/web3js/src/generated/delegation-program-instructions/delegate.ts (2)
createDelegateInstruction(17-51)serializeDelegateInstructionData(53-92)ts/web3js/src/generated/delegation-program-instructions/topUpEphemeralBalance.ts (2)
createTopUpEscrowInstruction(16-44)serializeTopUpEscrowInstructionData(46-66)ts/web3js/src/generated/delegation-program-instructions/closeEphemeralBalance.ts (2)
createCloseEscrowInstruction(15-37)serializeCloseEscrowInstructionData(39-55)
ts/kit/src/__test__/generated.test.ts (3)
ts/kit/src/generated/delegation-program-instructions/delegate.ts (2)
createDelegateInstruction(22-72)serializeDelegateInstructionData(74-118)ts/kit/src/generated/delegation-program-instructions/topUpEphemeralBalance.ts (2)
createTopUpEscrowInstruction(21-56)serializeTopUpEscrowInstructionData(58-79)ts/kit/src/generated/delegation-program-instructions/closeEphemeralBalance.ts (2)
createCloseEscrowInstruction(20-50)serializeCloseEscrowInstructionData(52-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: build
🔇 Additional comments (2)
ts/kit/src/__test__/generated.test.ts (1)
12-150: Comprehensive test coverage for generated instructions.The test suite thoroughly validates:
- Instruction construction with correct account counts
- Discriminator values matching the expected delegation program layout
- Field serialization at correct offsets with proper endianness
- Optional field handling (validator presence/absence)
This provides strong confidence in the generated instruction code.
ts/web3js/src/__test__/generated.test.ts (1)
75-151: Well-structured test coverage for topUpEscrow and closeEscrow instructions.The tests comprehensively validate:
- Account configurations matching the instruction requirements
- Exact data lengths (17 bytes for topUpEscrow, 9 bytes for closeEscrow)
- Correct discriminators and field offsets
- Proper BigInt handling for u64 amounts
The parallel structure with the kit test suite ensures consistent validation across both packages.
| createCloseEscrowInstruction, | ||
| serializeCloseEscrowInstructionData, | ||
| } from "../generated/delegation-program-instructions"; | ||
| import { getAddressFromPublicKey } from "@solana/kit"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused import.
The getAddressFromPublicKey import is not used anywhere in this test file.
Apply this diff:
-import { getAddressFromPublicKey } from "@solana/kit";📝 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.
| import { getAddressFromPublicKey } from "@solana/kit"; |
🤖 Prompt for AI Agents
In ts/kit/src/__test__/generated.test.ts around line 10, the import statement
"getAddressFromPublicKey" from "@solana/kit" is unused; remove that named import
(or the entire import line if nothing else is imported) so the test file no
longer contains an unused import and linter/CI will pass.
|
|
||
| # Magicblock | ||
| magicblock-delegation-program = { version = "1.1.3", features = ["no-entrypoint", "core"], default-features = false } | ||
| # magicblock-delegation-program = { version = "1.1.3", features = ["no-entrypoint", "sdk"], default-features = false } // updating from "core" to "sdk" |
| solana-sysvar = { workspace = true, optional = true } | ||
| magicblock-magic-program-api = { workspace = true } | ||
| magicblock-delegation-program = { workspace = true, features = ["no-entrypoint", "core"], default-features = false } | ||
| # magicblock-delegation-program = { workspace = true, features = ["no-entrypoint", "sdk"], default-features = false } // updating from "core" to "sdk" |
Changes
src/generated/delegation-program-instructions/folder for both kit and web3js packagesdelegate(discriminator: [0,0,0,0,0,0,0,0])topUpEphemeralBalance(discriminator: [9,0,0,0,0,0,0,0])closeEphemeralBalance(discriminator: [11,0,0,0,0,0,0,0]).solitarc.jsTesting
All tests pass:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.