fix: system program add signer mut account check#1808
Conversation
WalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AccountChecks
participant AccountInfo
Caller->>AccountChecks: check_mut(account_info)
AccountChecks->>AccountInfo: is_writable()
alt Account is writable
AccountChecks-->>Caller: Ok(())
else Account is not writable
AccountChecks-->>Caller: Err(AccountNotMutable)
end
sequenceDiagram
participant SystemProgram
participant AccountChecks
participant FeePayerAccount
SystemProgram->>AccountChecks: check_fee_payer(fee_payer)
AccountChecks->>FeePayerAccount: is_signer()
AccountChecks->>AccountChecks: check_mut(fee_payer)
alt Fee payer is signer and mutable
AccountChecks-->>SystemProgram: Ok(fee_payer)
else
AccountChecks-->>SystemProgram: Err(...)
end
sequenceDiagram
participant Instruction
participant AccountIterator
participant AccountChecks
Instruction->>AccountIterator: next_account_info()
alt account exists
Instruction->>AccountChecks: check_signer(authority)
AccountChecks-->>Instruction: Ok or Err(ProgramError)
else
Instruction-->>Instruction: Err(NotEnoughAccountKeys)
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
programs/system/src/accounts/account_checks.rs (1)
14-18: Use?consistently instead of manualmap_errThroughout the module most helpers rely on
?and the existingFrom<AccountError> for ProgramErrorimpl (e.g.check_non_mut(authority)?;).
Insidecheck_fee_payerthe new mutability check still uses an explicitmap_err. Streamline for readability:- check_signer(fee_payer).map_err(ProgramError::from)?; - check_mut(fee_payer).map_err(ProgramError::from)?; + check_signer(fee_payer)?; + check_mut(fee_payer)?;No behavior change, just cleaner code.
program-libs/account-checks/tests/tests.rs (1)
301-342: Good addition – but reconsider the heavy feature gateThe new
test_check_mutthoroughly validates both success & failure paths – nice work.
However, the entire file is#![cfg(all(feature = "solana", feature = "pinocchio"))], meaning none of these tests run unless both back-ends are enabled simultaneously. That’s uncommon and may hide regressions when only one backend is compiled.Consider:
- Moving the
check_muttests to an always-compiled file, using#[cfg(feature = "...")]inside each case (as you already do), or- Relaxing the top-level gate to
any(...).This will maximise test coverage in CI configurations that build a single backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
program-libs/account-checks/src/checks.rs(1 hunks)program-libs/account-checks/tests/tests.rs(1 hunks)programs/system/src/accounts/account_checks.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
programs/system/src/accounts/account_checks.rs (1)
program-libs/account-checks/src/checks.rs (2)
check_mut(124-129)check_signer(117-122)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: test-double-registration
- GitHub Check: Test program-libs
- GitHub Check: test-state-batched
- GitHub Check: test-state-photon-batched
- GitHub Check: test-e2e
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: test-2-foresters
- GitHub Check: test-address-batched
- GitHub Check: Test sdk-libs
- GitHub Check: lint
- GitHub Check: stateless-js
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
programs/system/src/invoke/instruction.rs (1)
50-52: Propagate the error directly instead of re-mapping it
check_signeralready returns aProgramError(or something that implementsInto<ProgramError>).
The extramap_err(ProgramError::from)is therefore redundant and slightly obscures intent.-let authority = accounts.next().ok_or(ProgramError::NotEnoughAccountKeys)?; -check_signer(authority).map_err(ProgramError::from)?; +let authority = accounts.next().ok_or(ProgramError::NotEnoughAccountKeys)?; +check_signer(authority)?;This keeps the happy path clean and removes one level of allocation in the error-path.
Please double-check the return type ofcheck_signer; if it is alreadyResult<(), ProgramError>(as is typical for the helper), this refactor is safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
program-libs/account-checks/src/checks.rs(2 hunks)program-libs/account-checks/tests/tests.rs(3 hunks)programs/system/src/accounts/account_checks.rs(2 hunks)programs/system/src/invoke/instruction.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- programs/system/src/accounts/account_checks.rs
- program-libs/account-checks/tests/tests.rs
- program-libs/account-checks/src/checks.rs
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: lint
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: stateless-js
- GitHub Check: test-double-registration
- GitHub Check: test-2-foresters
- GitHub Check: test-state-photon-batched
- GitHub Check: test-e2e
- GitHub Check: test-state-batched
- GitHub Check: test-address-batched
🔇 Additional comments (1)
programs/system/src/invoke/instruction.rs (1)
47-47: Confirm thatcheck_fee_payerstill validatessignerstatusWith the new mutability requirement introduced in this PR, make sure
check_fee_payercontinues to enforce that the fee-payer is also a signer.
Silent removal of that check would leave the transaction relying on the runtime’s implicit fee-payer rule, which is not guaranteed during CPI.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests