feat: pinocchio ephemeral permission#81
Open
Dodecahedr0x wants to merge 17 commits intomainfrom
Open
Conversation
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughThis PR introduces a complete new Pinocchio Ephemeral Permission Counter program with Rust source code, configuration files, documentation, and comprehensive integration tests demonstrating counter initialization, delegation to ephemeral rollups, permission management, and state verification across base and ephemeral layers. ChangesPinocchio Ephemeral Permission Counter Program
Submodule Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 20
🤖 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 `@pinocchio-ephemeral-permission-counter/Cargo.toml`:
- Around line 17-18: Replace the local path dependency for the crate named
ephemeral-rollups-pinocchio in Cargo.toml with the commented-out git dependency
that pins the specific rev; update the existing line
`ephemeral-rollups-pinocchio = { path =
"../../ephemeral-rollups-sdk/rust/pinocchio" }` to use the git source and rev
from the commented line (the previously commented `ephemeral-rollups-pinocchio =
{ git = "https://github.com/magicblock-labs/ephemeral-rollups-sdk.git", rev =
"a8ecea450d3bbe5bac119a3f4c55cd9f96b4837b" }`) so the example builds for anyone
cloning this repo standalone.
In `@pinocchio-ephemeral-permission-counter/package.json`:
- Line 21: The package's license metadata is inconsistent: the package.json
"license" field currently reads "Apache-2.0" while the repository LICENSE file
is MIT; update one to match the other—either change the package.json "license"
value to "MIT" to match the LICENSE file or replace/update the LICENSE file to
Apache-2.0—ensure the "license" field in package.json and the repository LICENSE
file are identical so tooling and consumers see the same license.
- Line 33: Update the Vitest dependency in package.json from "vitest": "^0.34.0"
to a secure compatible range (recommend "vitest": "^3.0.0" for minimal breaking
changes or "vitest": "^4.0.0" if you will handle migration), then run npm/yarn
install and execute the test suite to catch any breaking changes; if
tests/config fail, follow Vitest migration guide to adjust
config/coverage/mocks/browser settings and ensure package.json scripts
referencing vitest still work.
In `@pinocchio-ephemeral-permission-counter/README.md`:
- Around line 57-59: Update the README Account Structure to reflect the real
on-chain size: change the Counter entry from "8 bytes (u64 count value)" to "48
bytes (struct size)" and mention the field breakdown to match
src/state.rs::Counter (id: Address 32 bytes, count: u64 8 bytes, bump: u8 1 byte
+ 7 bytes padding) and the SIZE = 48 constant; also note that tests read the
count at data.subarray(32, 40) to avoid confusion about layout and rent
footprint.
- Around line 36-55: The README's instruction list is incorrect and must be
updated to match the actual dispatcher in entrypoint.rs: replace entries 4–6
(Commit/IncrementAndCommit/IncrementAndUndelegate) with CreatePermission,
UpdatePermission, and ClosePermission to reflect the discriminators used in the
dispatcher; correct payload descriptions for InitializeCounter (takes a 32-byte
id / Address, not a bump), IncreaseCounter (takes only increase_by: u64), and
Delegate (no payload); remove any references to non-existent handlers and add a
note documenting the special undelegation-callback discriminator bytes
[196,28,41,206,48,37,51,167]; use the actual symbol names from the code
(InitializeCounter, IncreaseCounter, Delegate, CreatePermission,
UpdatePermission, ClosePermission) so the README mirrors entrypoint.rs exactly.
In `@pinocchio-ephemeral-permission-counter/src/entrypoint.rs`:
- Around line 113-120: The InitializeCounter branch currently slices
payload[..32] which panics on short input; update the
InstructionDiscriminator::InitializeCounter handling to use
payload.get(..32).ok_or(ProgramError::InvalidInstructionData) (or equivalent)
and then convert that 32-byte slice into an array before calling
Address::new_from_array, finally passing the resulting id into
process_initialize_counter so a short payload returns InvalidInstructionData
instead of panicking.
In `@pinocchio-ephemeral-permission-counter/src/processor.rs`:
- Around line 211-212: Replace redundant explicit field initializers using
field-init shorthand: in the struct initializers referencing permission_program,
magic_program, and permission (and the other occurrences noted around the blocks
initializing the same fields at the other locations), remove the repeated
`field: field` and use the shorthand `permission_program`, `magic_program`, and
`permission` instead; update the initializers inside the constructor/struct
literal where these identifiers appear (including the other occurrences at the
two additional locations mentioned) so they use the concise field-init form to
satisfy clippy::redundant_field_names.
- Around line 110-115: Extract the repeated "load + derive_pda + address-match"
logic into a helper fn named load_and_verify_counter that takes &AccountView,
checks owned_by(&crate::ID), borrows data, loads Counter::load, derives the PDA
via Counter::derive_pda(&counter.id, &[counter.bump]) and verifies the derived
PDA equals counter_info.address(), then returns Ok((counter.bump, counter.id))
or appropriate ProgramError (IllegalOwner / InvalidSeeds / propagate borrow
errors); replace the five-line blocks in process_increase_counter,
process_delegate, process_create_permission, process_update_permission,
process_close_permission and the scoped use in process_commit_and_undelegate to
call load_and_verify_counter and use its returned (bump, id) instead of
duplicating the logic.
- Around line 87-96: The log eagerly computes counter_data.count + increase_by
which can overflow before the checked_add; change the sequence in the function
where counter_data.count is updated so you first compute new_count using
counter_data.count.checked_add(increase_by).ok_or(ProgramError::ArithmeticOverflow)
(or otherwise compute into an Option/Result), then call log! with the
precomputed new_count, and finally assign counter_data.count = new_count; this
ensures no pre-logging overflow and that the logged "to" value matches the value
actually written.
- Around line 181-227: process_create_permission currently trusts authority_info
from the accounts slice without verifying the signer/owner; require
authority_info.is_signer() before proceeding and, for stronger protection,
compare authority_info.address() against a stored initializer/owner field on the
Counter (load via Counter::load and add owner to Counter struct) and reject if
mismatched; apply the same signer check for payer_info (payer_info.is_signer())
and mirror these checks in process_update_permission and
process_close_permission so the CPI cannot be front-run by an unrelated signer.
- Around line 74-85: The account ownership of counter/state must be verified
before deserializing; for each affected function (process_increase_counter,
process_delegate, process_commit_and_undelegate, process_create_permission,
process_update_permission, process_close_permission) add an explicit check using
counter_info.owned_by(&crate::ID) (matching the pattern in
process_initialize_counter) and return ProgramError::IllegalOwner (or
appropriate ProgramError) if false, before calling Counter::load or
Counter::load_mut (or any deserialization) and before deriving/verifying the PDA
via Counter::derive_pda; ensure you reference the same AccountView binding
(counter_info) used in the diff and perform the ownership check immediately
after extracting the account and before any try_borrow_mut/Counter::load*_
calls.
- Around line 321-327: process_undelegation_callback currently trusts a
hardcoded discriminator and calls undelegate without verifying the call came
from the delegation program; update the handler to require and validate the
Instructions sysvar account (add it to the accounts slice) and inspect the
previous instruction to ensure its program_id equals the delegation program's
Pubkey and its data starts with the expected discriminator before calling
undelegate (alternatively require a specific signer account and verify
payer_info.is_signer). Refer to process_undelegation_callback and undelegate
when adding the check so the entrypoint explicitly enforces the CPI origin.
- Around line 50-60: The inline magic number calculation for ephemeral_rent
should be replaced by named constants to document the account layout: define
constants (e.g., PERMISSION_HEADER_BYTES = 35 and SLOTS_PER_MEMBER = 2) and
compute ephemeral_rent using those with MAX_MEMBER_SIZE (cast to u64) and the
32-byte slot size, then use the new ephemeral_rent in the CreateAccount call
that uses payer_info, counter_info, lamports, and Counter::SIZE; update or
remove the TODO to reflect the refactor so future changes to header size or
slots-per-member won’t silently under-fund rent.
In `@pinocchio-ephemeral-permission-counter/src/state.rs`:
- Around line 15-37: The current load/load_mut docs and safety are misleading
and risk silent bugs: update both methods to require data.len() == Self::SIZE
(reject > as well as <), change the safety comment to state exactly which
invariants the caller must uphold (the byte slice is the account data owned by
this program and is not currently borrowed elsewhere), and make the Counter type
zero-init friendly by deriving or implementing an appropriate
zero-initialization marker (e.g., bytemuck::Zeroable/Pod or equivalent) so
zeroed account memory is valid; reference the Counter type and the load/load_mut
functions when making these changes.
- Line 13: Replace the hard-coded byte sum for the Counter struct's SIZE
constant with a compiler-calculated value using mem::size_of so it stays correct
if fields change: update the declaration of SIZE (the const in state.rs for
Counter) to use core::mem::size_of::<Counter>() (or size_of::<Self>() if inside
an impl) and ensure mem::size_of is in scope (use core::mem or std::mem as
appropriate).
- Line 20: The code uses usize::is_multiple_of (unstable on Rust <1.87) causing
MSRV mismatch; either update the project's MSRV to 1.87+ or replace the uses in
state.rs (functions load and load_mut) with an equivalent modulo-based alignment
check: compute core::mem::align_of::<Self>() and test (ptr as usize) % align ==
0 (negate as appropriate) instead of is_multiple_of, and update the error/branch
logic accordingly so both load and load_mut perform the same portable alignment
validation.
In
`@pinocchio-ephemeral-permission-counter/tests/pinocchio-secret-counter.test.ts`:
- Line 339: Reads using connectionEphemeralRollup.getAccountInfo(counterPda) are
missing an explicit commitment and can return stale data versus the writes made
with sendAndConfirmTransaction(..., { commitment: "confirmed" }); update each
read of getAccountInfo (e.g., the calls in the test that reference counterPda)
to pass { commitment: "confirmed" } so the read commitment matches the write
commitment and prevents flaky CI assertions on subarray(32, 40).
- Line 36: The test's suite name and filename drift from the crate name; update
either the test filename `pinocchio-secret-counter.test.ts` or the `describe`
label (currently "pinocchio-ephemeral-secret-counter") so they match the crate
`pinocchio-ephemeral-permission-counter`; specifically change the
`describe("pinocchio-ephemeral-secret-counter", ...)` label to
`describe("pinocchio-ephemeral-permission-counter", ...)` (or rename the file to
`pinocchio-ephemeral-permission-counter.test.ts`) so test output and searches
consistently reference the crate name.
- Around line 31-34: The test currently hardcodes PROGRAM_ID (and VAULT) which
can drift from the on-chain crate; update the test to load the program id from a
single source of truth instead of the literal
"AAWCg4eJHpdmUtM8Wz6Thm8FDi6C3vnMksf1pt2vfxhf": modify the code that defines
PROGRAM_ID (and optionally VAULT) to first check an environment variable (e.g.
process.env.PROGRAM_ID) and fall back to reading the generated artifact (e.g.
parse idl.json or the program keypair output from cargo build-sbf) to obtain the
program public key, ensuring tests always use the same ID declared by the
on-chain crate.
- Line 480: The test uses optional chaining on result.value
(expect(result.value?.err).toBeNull()), which hides potential bugs because
Connection.confirmTransaction always returns
RpcResponseAndContext<SignatureResult> so value is never undefined; update the
assertion to directly check result.value.err
(expect(result.value.err).toBeNull()) to remove the unnecessary ?. and surface
failures correctly, referencing the result returned by
Connection.confirmTransaction and the RpcResponseAndContext<SignatureResult>
shape.
🪄 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: 3dc09330-c2f7-4fdc-b92e-c6b99385841f
⛔ Files ignored due to path filters (2)
pinocchio-ephemeral-permission-counter/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
pinocchio-ephemeral-permission-counter/.env.examplepinocchio-ephemeral-permission-counter/.gitignorepinocchio-ephemeral-permission-counter/.yarnrc.ymlpinocchio-ephemeral-permission-counter/Cargo.tomlpinocchio-ephemeral-permission-counter/LICENSEpinocchio-ephemeral-permission-counter/README.mdpinocchio-ephemeral-permission-counter/package.jsonpinocchio-ephemeral-permission-counter/src/entrypoint.rspinocchio-ephemeral-permission-counter/src/lib.rspinocchio-ephemeral-permission-counter/src/processor.rspinocchio-ephemeral-permission-counter/src/state.rspinocchio-ephemeral-permission-counter/tests/pinocchio-secret-counter.test.tspinocchio-ephemeral-permission-counter/tsconfig.jsonprivate-payments
💤 Files with no reviewable changes (1)
- private-payments
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #80
Summary by CodeRabbit
New Features
Documentation
Tests
Chores