feat: add magic-actions-advanced example (post-delegation actions + PDA-paid commits)#79
Open
crocodiledundalk wants to merge 3 commits into
Conversation
|
@crocodiledundalk is attempting to deploy a commit to the MagicBlock Labs Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
WalkthroughThis pull request adds a complete "Magic Actions Advanced" example program for Solana ephemeral rollups. It demonstrates post-delegation actions (queued instructions executed automatically after account delegation) combined with PDA-paid magic action commits using a protocol-owned escrow signer. The PR includes comprehensive documentation, a full Anchor program implementation with initialization, delegation, and leaderboard commit flows, and an end-to-end integration test suite with tooling updates. ChangesMagic Actions Advanced Example Program
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 `@magic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rs`:
- Around line 38-42: In update_leaderboard, don't deserialize
ctx.accounts.counter blindly; first validate the account is the expected PDA and
owned by the allowed program(s) before reading its bytes. Derive the expected
PDA (using the same seeds used to create the Counter account), compare it to
ctx.accounts.counter.to_account_info().key, and assert the account owner equals
the expected program id (or system program if appropriate); also check the
account data length/is_initialized flag if present. Only after these checks
succeed, call try_borrow_data and Counter::try_deserialize to mutate
high_score/leaderboard.
In `@magic-action-shared-payer/README.md`:
- Around line 41-42: The README currently instructs users to run "rm -rf
/target/deploy/*.keypair" which uses an absolute path and can delete /target on
a machine; change the instruction to use a repository-relative path instead
(e.g., "target/deploy/*.keypair" or "./target/deploy/*.keypair") in the README
so it targets the repo's target/deploy directory; update the line referencing
the command (the rm -rf /target/deploy/*.keypair entry) to the safer relative
form and consider adding a short safety note or existence check before removal.
In `@magic-action-shared-payer/tests/magic-action-shared-payer.ts`:
- Around line 292-302: Replace the fixed animated sleep helper
(sleepWithAnimation) and any hard-coded sleeps used before
delegate/commit/undelegate assertions with a polling helper that repeatedly
checks the expected on-chain/state condition until it becomes true or a
configurable timeout elapses; implement a function like
waitForCondition(checkFn, timeoutMs, intervalMs) and use it in the tests where
delegate/commit/undelegate state is asserted (replace calls to
sleepWithAnimation and direct setTimeout waits) so each test polls the relevant
getter/contract call (e.g., the delegate status, commit inclusion, or undelegate
balance) at short intervals and fails only after the timeout instead of waiting
a fixed duration.
In `@magic-action-shared-payer/tsconfig.json`:
- Around line 3-8: The tsconfig.json restricts global type resolution to
["mocha","chai"], causing Node types used by migrations/deploy.ts (which uses
module.exports) to be missing; update the "types" array in tsconfig.json to
include "node" (e.g., ["mocha","chai","node"]) so `@types/node` is picked up and
the migration entrypoint type-checks correctly.
In `@magic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rs`:
- Around line 163-164: The delegation_program account (field delegation_program)
is unconstrained and can be swapped by callers; add an address constraint to
ensure it is the expected MagicBlock delegation program before performing the
PDA-signed CPI. Update the struct field annotation for delegation_program to
include an address check that compares to the MagicBlock delegation program's ID
constant (for example using #[account(address =
<magic_block_program_crate>::ID)] or the appropriate crate/program ID constant),
and keep references in the CPI call sites such as delegate_account_with_actions
unchanged so the inner invocation only ever receives the verified program.
In `@magic-actions-on-delegation/README.md`:
- Around line 38-42: Update the README command that currently uses an absolute
path "/target/deploy/*.keypair" to a project-local path; replace the command
string (rm -rf /target/deploy/*.keypair) with a safe relative version such as rm
-rf target/deploy/*.keypair or rm -f ./target/deploy/*.keypair so it deletes the
local target/deploy keypairs instead of touching the filesystem root.
In `@magic-actions-on-delegation/tests/magic-actions-on-delegation.ts`:
- Around line 193-195: Replace fixed sleep() waits with a polling helper that
repeatedly calls readCounterOnER() or readCounterOnBase() until the expected
value is observed or a real timeout elapses; implement a function like
waitForCounterOnER(expected: number, timeoutMs: number, intervalMs = 500) and
similarly waitForCounterOnBase(...) that measures elapsed time, loops awaiting
the corresponding readCounter... call every interval, returns when value
matches, and throws an error on timeout, then replace all uses of sleep(seconds)
in the tests with calls to the appropriate waitForCounter... helper to avoid
flaky fixed delays.
In `@magic-actions-on-delegation/tsconfig.json`:
- Around line 3-8: The tsconfig currently restricts ambient types to
["mocha","chai"] but migrations/deploy.ts uses CommonJS module.exports, so add
Node ambient types: update the "types" array in
magic-actions-on-delegation/tsconfig.json to include "node" (e.g.
["mocha","chai","node"]) and also add `@types/node` to magic-actions-on-delegation
package.json devDependencies so TypeScript recognizes Node globals and module
typings used by module.exports in migrations/deploy.ts.
🪄 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: 001d36c7-81f6-4dd0-9c0f-a3e1f2dd0bb1
⛔ Files ignored due to path filters (4)
magic-action-shared-payer/Cargo.lockis excluded by!**/*.lockmagic-action-shared-payer/yarn.lockis excluded by!**/yarn.lock,!**/*.lockmagic-actions-on-delegation/Cargo.lockis excluded by!**/*.lockmagic-actions-on-delegation/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
README.mddocs/post-delegation-actions.mdmagic-action-shared-payer/.gitignoremagic-action-shared-payer/.prettierignoremagic-action-shared-payer/Anchor.tomlmagic-action-shared-payer/Cargo.tomlmagic-action-shared-payer/README.mdmagic-action-shared-payer/migrations/deploy.tsmagic-action-shared-payer/package.jsonmagic-action-shared-payer/programs/magic-action-shared-payer/Cargo.tomlmagic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rsmagic-action-shared-payer/tests/magic-action-shared-payer.tsmagic-action-shared-payer/tsconfig.jsonmagic-actions-on-delegation/.gitignoremagic-actions-on-delegation/.prettierignoremagic-actions-on-delegation/Anchor.tomlmagic-actions-on-delegation/Cargo.tomlmagic-actions-on-delegation/README.mdmagic-actions-on-delegation/migrations/deploy.tsmagic-actions-on-delegation/package.jsonmagic-actions-on-delegation/programs/magic-actions-on-delegation/Cargo.tomlmagic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rsmagic-actions-on-delegation/tests/magic-actions-on-delegation.tsmagic-actions-on-delegation/tsconfig.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
magic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rs (1)
38-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill validate the counter owner before deserializing its bytes.
The new seeds constraint fixes the PDA address, but
update_leaderboardstill trusts raw bytes from anUncheckedAccountwithout checking that the owner is one of the expected programs for the delegated/undelegated states. A wrong owner or malformed account at that PDA can still make this action read invalid state or fail unexpectedly.Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rs` around lines 38 - 42, The code in update_leaderboard calls Counter::try_deserialize on an UncheckedAccount (counter) without verifying the account owner; before deserializing, assert that counter.to_account_info().owner matches one of the allowed program IDs for the delegated/undelegated counter states (e.g., compare to the expected program pubkeys/constants) and return an appropriate error if not, then proceed to call Counter::try_deserialize; apply the same owner check before any other Counter::try_deserialize usage (e.g., the other occurrence around lines 145-146) so you never deserialize bytes from an account with an unexpected owner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rs`:
- Around line 31-35: The increment function uses unchecked addition on
counter.count which will wrap at u64::MAX; change the logic in pub fn
increment(ctx: Context<Increment>) -> Result<()> to use
counter.count.checked_add(1) and if it returns None return an explicit error
(e.g., a custom OverflowError or ProgramError::Custom) instead of performing the
addition; update any associated error enum (or define a new variant) so callers
can see the overflow failure and keep the msg! log only after a successful
checked add.
- Around line 22-26: The initialize function currently mutates shared PDA
accounts (counter and leaderboard) unguarded, allowing any caller to rerun and
zero them; change the account constraints used where you see init_if_needed for
these PDAs to init so they can only be created once, or alternatively add an
explicit authority/initialized guard in the initialize function that checks a
designated authority signer or an "initialized" flag before writing (and return
an error if already initialized). Apply the same change to the other
initialization block in this file that sets count/high_score to 0 (the other
function/section using the same counter/leaderboard pattern).
In `@magic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rs`:
- Around line 26-27: The increment function (pub fn increment in lib.rs)
currently does unchecked u64 addition; change it to use checked_add(1) and
return a proper error if None is returned to prevent overflow. Add the suggested
ErrorCode enum with a CounterOverflow variant (#[error_code] pub enum ErrorCode
{ #[msg("counter overflow")] CounterOverflow, }) and update increment to set
ctx.accounts.counter.count =
ctx.accounts.counter.count.checked_add(1).ok_or(ErrorCode::CounterOverflow)?; so
the handler returns an error instead of silently wrapping on overflow.
- Around line 20-21: The initialize handler currently allows reinitialization
and wiping of the global PDA counter by using init_if_needed and then
unconditionally setting ctx.accounts.counter.count = 0; update the account
constraint so the PDA is created only once by replacing init_if_needed with init
on the Counter account (the PDA derived from COUNTER_SEED) and remove or guard
the unconditional reset in the initialize function (Context<Initialize> /
initialize) so the counter is not overwritten on subsequent calls; ensure the
account is created with the same seeds and bump used elsewhere and that access
control prevents repeat creation or modification by unauthorized signers.
---
Duplicate comments:
In `@magic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rs`:
- Around line 38-42: The code in update_leaderboard calls
Counter::try_deserialize on an UncheckedAccount (counter) without verifying the
account owner; before deserializing, assert that counter.to_account_info().owner
matches one of the allowed program IDs for the delegated/undelegated counter
states (e.g., compare to the expected program pubkeys/constants) and return an
appropriate error if not, then proceed to call Counter::try_deserialize; apply
the same owner check before any other Counter::try_deserialize usage (e.g., the
other occurrence around lines 145-146) so you never deserialize bytes from an
account with an unexpected owner.
🪄 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: 3b20ec78-f526-4f80-b656-9fe03413bb68
📒 Files selected for processing (6)
magic-action-shared-payer/README.mdmagic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rsmagic-action-shared-payer/tsconfig.jsonmagic-actions-on-delegation/README.mdmagic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rsmagic-actions-on-delegation/tsconfig.json
Comment on lines
+31
to
+35
| pub fn increment(ctx: Context<Increment>) -> Result<()> { | ||
| let counter = &mut ctx.accounts.counter; | ||
| counter.count += 1; | ||
| msg!("PDA {} count: {}", counter.key(), counter.count); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
Use checked arithmetic for count.
Line 33 can wrap on-chain once count reaches u64::MAX, which silently resets the counter and can corrupt the observed high score. Please switch this to checked_add and return an explicit error on overflow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magic-action-shared-payer/programs/magic-action-shared-payer/src/lib.rs`
around lines 31 - 35, The increment function uses unchecked addition on
counter.count which will wrap at u64::MAX; change the logic in pub fn
increment(ctx: Context<Increment>) -> Result<()> to use
counter.count.checked_add(1) and if it returns None return an explicit error
(e.g., a custom OverflowError or ProgramError::Custom) instead of performing the
addition; update any associated error enum (or define a new variant) so callers
can see the overflow failure and keep the msg! log only after a successful
checked add.
Comment on lines
+20
to
+21
| pub fn initialize(ctx: Context<Initialize>) -> Result<()> { | ||
| ctx.accounts.counter.count = 0; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd magic-actions-on-delegation/programs/magic-actions-on-delegation/src && wc -l lib.rsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 97
🏁 Script executed:
cd magic-actions-on-delegation/programs/magic-actions-on-delegation/src && cat -n lib.rs | head -40Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1825
🏁 Script executed:
cd magic-actions-on-delegation/programs/magic-actions-on-delegation/src && cat -n lib.rs | sed -n '100,130p'Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1084
🏁 Script executed:
cd magic-actions-on-delegation/programs/magic-actions-on-delegation/src && grep -n "init_if_needed\|init," lib.rsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 114
🏁 Script executed:
cd magic-actions-on-delegation/programs/magic-actions-on-delegation/src && cat -n lib.rs | sed -n '1,100p' | grep -A 15 "pub struct Initialize"Repository: magicblock-labs/magicblock-engine-examples
Length of output: 68
initialize can reset the shared PDA counter after deployment.
init_if_needed allows the handler to execute on an existing account. Since Line 21 unconditionally writes 0 to the counter, any signer can re-call initialize and wipe the counter state. This PDA is global (derived from COUNTER_SEED only), and there is no access control preventing repeated calls. Change init_if_needed to init to prevent reinitialization.
Suggested fix
#[derive(Accounts)]
pub struct Initialize<'info> {
#[account(
- init_if_needed,
+ init,
payer = user,
space = 8 + 8,
seeds = [COUNTER_SEED],
bump,
)]
pub counter: Account<'info, Counter>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rs`
around lines 20 - 21, The initialize handler currently allows reinitialization
and wiping of the global PDA counter by using init_if_needed and then
unconditionally setting ctx.accounts.counter.count = 0; update the account
constraint so the PDA is created only once by replacing init_if_needed with init
on the Counter account (the PDA derived from COUNTER_SEED) and remove or guard
the unconditional reset in the initialize function (Context<Initialize> /
initialize) so the counter is not overwritten on subsequent calls; ensure the
account is created with the same seeds and bump used elsewhere and that access
control prevents repeat creation or modification by unauthorized signers.
Comment on lines
+26
to
+27
| pub fn increment(ctx: Context<Increment>) -> Result<()> { | ||
| ctx.accounts.counter.count += 1; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "lib.rs" | head -20Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1193
🏁 Script executed:
cat -n magic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 7810
Guard the counter increment against overflow.
Line 27 uses unchecked u64 arithmetic. In BPF (release) builds, overflow wraps silently, corrupting state. Use checked_add(1) with proper error handling.
Suggested fix
pub fn increment(ctx: Context<Increment>) -> Result<()> {
- ctx.accounts.counter.count += 1;
+ ctx.accounts.counter.count = ctx
+ .accounts
+ .counter
+ .count
+ .checked_add(1)
+ .ok_or(ErrorCode::CounterOverflow)?;
msg!(
"PDA {} count: {} (signer: {})",
ctx.accounts.counter.key(),
ctx.accounts.counter.count,
ctx.accounts.user.key(),Add an error variant alongside the instruction code:
#[error_code]
pub enum ErrorCode {
#[msg("counter overflow")]
CounterOverflow,
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magic-actions-on-delegation/programs/magic-actions-on-delegation/src/lib.rs`
around lines 26 - 27, The increment function (pub fn increment in lib.rs)
currently does unchecked u64 addition; change it to use checked_add(1) and
return a proper error if None is returned to prevent overflow. Add the suggested
ErrorCode enum with a CounterOverflow variant (#[error_code] pub enum ErrorCode
{ #[msg("counter overflow")] CounterOverflow, }) and update increment to set
ctx.accounts.counter.count =
ctx.accounts.counter.count.checked_add(1).ok_or(ErrorCode::CounterOverflow)?; so
the handler returns an error instead of silently wrapping on overflow.
Contributor
|
Hey thanks for the PR! It would be nice if this could be single folder combining delegation actions and PDA-paid intent bundles instead of 2 distinct programs (we're in the process of simplifying this repo) |
Combined example demonstrating two advanced MagicBlock ER patterns: 1. Post-delegation actions — an `increment` instruction is queued inside the `delegate` call via `PostDelegationActions`. The ER validator fires it automatically when the account is first cloned; no separate client tx needed. 2. PDA-paid magic action — `commit_and_update_leaderboard` uses a `global_signer` PDA as the `escrow_authority` so the protocol escrow covers the magic-action fee instead of the individual user's wallet. Uses `.build()` + AccountMeta patch + `invoke_signed` to authorise the PDA over the MagicProgram CPI. Includes full ts-mocha integration test (5 tests covering the full delegate → increment on ER → commit + leaderboard update → undelegate lifecycle) and a wired-up entry in test-locally.sh. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
crocodiledundalk
force-pushed
the
additional-magic-action-examples
branch
from
4c9327c to
7977cfb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magic-actions-advanced/tests/magic-actions-advanced.ts`:
- Around line 529-548: Replace the shell invocation in readCommitSigsFromDb with
a real SQLite client call: open the database at the passed dbPath using a
library like better-sqlite3 or sqlite3, execute the same SELECT query (`SELECT
pubkey, commit_status, commit_stage_signature, finalize_stage_signature FROM
commit_status ORDER BY created_at ASC;`), and map the returned rows into the
CommitRow shape (pubkey defaulting to "", status defaulting to "", commitSig and
finalizeSig set to null when empty). Keep the same try/catch semantics to return
[] on error and ensure you locate and update the readCommitSigsFromDb function
and its use of the dbPath and query constants.
- Around line 264-277: The test currently uses a hardcoded DB_PATH
("/tmp/magicblock-er-storage/committor_service.sqlite") which can be missing in
other environments; update the test to read the database path from an
environment variable (e.g., MAGICBLOCK_ER_DB_PATH) with the current hardcoded
value as the fallback, and use that variable when calling readCommitSigsFromDb;
additionally, make the behavior explicit in readCommitSigsFromDb or the test
(log a clear warning or throw) when the resolved path does not exist so failures
are visible; locate these changes around the DB_PATH declaration and the call to
readCommitSigsFromDb in magic-actions-advanced/tests/magic-actions-advanced.ts.
🪄 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: 178841db-cc1b-4e72-b47f-9563f76e5588
⛔ Files ignored due to path filters (2)
magic-actions-advanced/Cargo.lockis excluded by!**/*.lockmagic-actions-advanced/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.gitignoreREADME.mdmagic-actions-advanced/.gitignoremagic-actions-advanced/.prettierignoremagic-actions-advanced/Anchor.tomlmagic-actions-advanced/Cargo.tomlmagic-actions-advanced/README.mdmagic-actions-advanced/migrations/deploy.tsmagic-actions-advanced/package.jsonmagic-actions-advanced/programs/magic-actions-advanced/Cargo.tomlmagic-actions-advanced/programs/magic-actions-advanced/src/lib.rsmagic-actions-advanced/target/deploy/magic_actions_advanced-keypair.jsonmagic-actions-advanced/tests/magic-actions-advanced.tsmagic-actions-advanced/tsconfig.jsontest-locally.sh
💤 Files with no reviewable changes (4)
- magic-actions-advanced/.prettierignore
- magic-actions-advanced/Cargo.toml
- magic-actions-advanced/migrations/deploy.ts
- magic-actions-advanced/tsconfig.json
Comment on lines
+264
to
+277
| const DB_PATH = "/tmp/magicblock-er-storage/committor_service.sqlite"; | ||
| const commitSigs = readCommitSigsFromDb(DB_PATH); | ||
| console.log(`\n Found ${commitSigs.length} L1 commit transaction(s) in ER database:`); | ||
| for (const { commitSig, finalizeSig, status, pubkey } of commitSigs) { | ||
| console.log(` pubkey=${pubkey} status=${status}`); | ||
| if (commitSig) { | ||
| console.log(` commit-stage sig: ${commitSig}`); | ||
| await printTransactionAccounts("L1 commit-stage", commitSig, baseConnection, knownAddresses); | ||
| } | ||
| if (finalizeSig) { | ||
| console.log(` finalize-stage sig: ${finalizeSig}`); | ||
| await printTransactionAccounts("L1 finalize-stage", finalizeSig, baseConnection, knownAddresses); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded database path may not work in all environments.
The database path /tmp/magicblock-er-storage/committor_service.sqlite is hardcoded and specific to the local ephemeral validator configuration. This test will fail silently (returning empty array from readCommitSigsFromDb) if:
- The ER validator uses a different storage path
- Tests run in CI with a different environment
- The validator is configured differently
Consider making this configurable via environment variable or documenting the dependency on this specific path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magic-actions-advanced/tests/magic-actions-advanced.ts` around lines 264 -
277, The test currently uses a hardcoded DB_PATH
("/tmp/magicblock-er-storage/committor_service.sqlite") which can be missing in
other environments; update the test to read the database path from an
environment variable (e.g., MAGICBLOCK_ER_DB_PATH) with the current hardcoded
value as the fallback, and use that variable when calling readCommitSigsFromDb;
additionally, make the behavior explicit in readCommitSigsFromDb or the test
(log a clear warning or throw) when the resolved path does not exist so failures
are visible; locate these changes around the DB_PATH declaration and the call to
readCommitSigsFromDb in magic-actions-advanced/tests/magic-actions-advanced.ts.
Comment on lines
+529
to
+548
| function readCommitSigsFromDb(dbPath: string): CommitRow[] { | ||
| try { | ||
| const query = `SELECT pubkey, commit_status, commit_stage_signature, finalize_stage_signature FROM commit_status ORDER BY created_at ASC;`; | ||
| const raw = execSync(`sqlite3 "${dbPath}" "${query}"`, { | ||
| encoding: "utf8", | ||
| }).trim(); | ||
| if (!raw) return []; | ||
| return raw.split("\n").map((line) => { | ||
| const [pubkey, status, commitSig, finalizeSig] = line.split("|"); | ||
| return { | ||
| pubkey: pubkey ?? "", | ||
| status: status ?? "", | ||
| commitSig: commitSig || null, | ||
| finalizeSig: finalizeSig || null, | ||
| }; | ||
| }); | ||
| } catch { | ||
| return []; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Static analysis flagged execSync usage, but current code is safe.
The static analysis tool correctly identified that execSync with string interpolation can be dangerous. However, in the current implementation:
dbPathis always the hardcoded constant/tmp/magicblock-er-storage/committor_service.sqlite(line 264)queryis a hardcoded constant (line 531)
So there's no actual command injection risk. The try-catch at line 530 also handles failures gracefully.
For maintainability, consider using a dedicated SQLite client library (e.g., better-sqlite3) instead of shelling out, which would be safer and more performant.
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 532-534: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magic-actions-advanced/tests/magic-actions-advanced.ts` around lines 529 -
548, Replace the shell invocation in readCommitSigsFromDb with a real SQLite
client call: open the database at the passed dbPath using a library like
better-sqlite3 or sqlite3, execute the same SELECT query (`SELECT pubkey,
commit_status, commit_stage_signature, finalize_stage_signature FROM
commit_status ORDER BY created_at ASC;`), and map the returned rows into the
CommitRow shape (pubkey defaulting to "", status defaulting to "", commitSig and
finalizeSig set to null when empty). Keep the same try/catch semantics to return
[] on error and ensure you locate and update the readCommitSigsFromDb function
and its use of the dbPath and query constants.
Source: Linters/SAST tools
- Read ER commit DB path from MAGICBLOCK_ER_DB_PATH env var (fallback to /tmp/magicblock-er-storage/committor_service.sqlite) - Log explicit warning when DB file is missing rather than silently returning [] - Surface sqlite3 error message in the catch branch Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ests No test assertion depended on the DB read — it was purely diagnostic console logging. Removes execSync/existsSync imports, DB_PATH block, CommitRow interface, and readCommitSigsFromDb function. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
crocodiledundalk
changed the title
Author
|
Thanks @Dodecahedr0x - hope the above addresses your feedback |
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
2 participants
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.
Summary
Two new Magic Actions examples that extend the existing
magic-actionsdemo.magic-action-shared-payerShows how to use a protocol-owned PDA (
global_signer, seeds:[b"global_signer"]) as the escrow authority for Magic Action callbacks instead of the individual user's wallet. UsesMagicInstructionBuilder::build()+ manualAccountMetapatching +invoke_signedto authorize the PDA over the MagicProgram CPI.magic-actions-on-delegationDemonstrates post-delegation actions: an
incrementinstruction is bundled into thedelegatecall viadelegate_account_with_actions+PostDelegationActions. The ER validator fires it automatically when the account is first cloned — no separate client transaction needed.Also adds:
docs/post-delegation-actions.md— specification for thePostDelegationActionskey-table encoding (compact AccountMeta, encryption variants, signer constraints)README.mdupdated to list both new examples and link to the new guideTest plan
cd magic-action-shared-payer && yarn && anchor testpassescd magic-actions-on-delegation && yarn && anchor testpassesSummary by CodeRabbit
New Features
Documentation
Tests