feat: added a way to verify validator identity within ER#1133
feat: added a way to verify validator identity within ER#1133
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis pull request introduces a new Suggested reviewers
✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-magic-program-api/src/instruction.rs`:
- Around line 306-310: Update the VerifyValidatorIdentity instruction docs to
match the handler in verify_validator_identity.rs: list index 0 as the Magic
Context account (MAGIC_CONTEXT_PUBKEY) and index 1 as the Validator Authority,
and ensure the account reference descriptions and ordering reflect that the
handler expects the magic context first and validator authority second (so the
documented account layout aligns with VerifyValidatorIdentity and
MAGIC_CONTEXT_PUBKEY usage).
In `@programs/magicblock/src/verify_validator_identity.rs`:
- Around line 32-35: Update the error logging string in the
VerifyValidatorIdentity handler so it correctly references the instruction name
instead of "ScheduleCommit"; locate the ic_msg! call in the
VerifyValidatorIdentity logic that currently logs "ScheduleCommit ERR: Magic
program account not found" and change it to a descriptive message like
"VerifyValidatorIdentity ERR: Magic program account not found" (or similar) so
logs correctly reflect the VerifyValidatorIdentity instruction context.
- Around line 86-101: In setup_transaction_accounts(), avoid using
HashMap::remove().unwrap() which yields an opaque panic; replace both unwrap()
calls with expect(...) that include descriptive messages referencing the keys
and the helper that should have populated them (e.g., mention
MAGIC_CONTEXT_PUBKEY and the validator returned by validator_authority_id()) so
if ensure_started_validator(...) failed to insert the entries the test failure
will report which key was missing and where (setup_transaction_accounts /
ensure_started_validator).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce461575-ae50-45d1-acf6-5288481e2219
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
magicblock-magic-program-api/src/instruction.rsprograms/magicblock/src/lib.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/verify_validator_identity.rs
|
|
||
| /// Verifies if account's pubkey corresponds to current validator identity | ||
| /// # Account references | ||
| /// - **0.** `[]` Validator Authority | ||
| VerifyValidatorIdentity, |
There was a problem hiding this comment.
Instruction documentation does not match the handler's expected account layout.
The handler in verify_validator_identity.rs expects:
- Index 0: Magic Context account (
MAGIC_CONTEXT_PUBKEY) - Index 1: Validator Authority
However, the documentation only lists the Validator Authority at index 0. This mismatch could cause integration issues for developers using this instruction.
📝 Suggested documentation fix
/// Verifies if account's pubkey corresponds to current validator identity
/// # Account references
- /// - **0.** `[]` Validator Authority
+ /// - **0.** `[]` Magic Context account
+ /// - **1.** `[]` Validator Authority
VerifyValidatorIdentity,📝 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.
| /// Verifies if account's pubkey corresponds to current validator identity | |
| /// # Account references | |
| /// - **0.** `[]` Validator Authority | |
| VerifyValidatorIdentity, | |
| /// Verifies if account's pubkey corresponds to current validator identity | |
| /// # Account references | |
| /// - **0.** `[]` Magic Context account | |
| /// - **1.** `[]` Validator Authority | |
| VerifyValidatorIdentity, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-magic-program-api/src/instruction.rs` around lines 306 - 310,
Update the VerifyValidatorIdentity instruction docs to match the handler in
verify_validator_identity.rs: list index 0 as the Magic Context account
(MAGIC_CONTEXT_PUBKEY) and index 1 as the Validator Authority, and ensure the
account reference descriptions and ordering reflect that the handler expects the
magic context first and validator authority second (so the documented account
layout aligns with VerifyValidatorIdentity and MAGIC_CONTEXT_PUBKEY usage).
| ic_msg!( | ||
| invoke_context, | ||
| "ScheduleCommit ERR: Magic program account not found" | ||
| ); |
There was a problem hiding this comment.
Error message references wrong instruction.
The error message says "ScheduleCommit ERR" but this is the VerifyValidatorIdentity instruction handler. This appears to be a copy-paste artifact that could confuse debugging.
📝 Suggested fix
.ok_or_else(|| {
ic_msg!(
invoke_context,
- "ScheduleCommit ERR: Magic program account not found"
+ "VerifyValidatorIdentity ERR: Magic program account not found"
);
InstructionError::UnsupportedProgramId
})?;📝 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.
| ic_msg!( | |
| invoke_context, | |
| "ScheduleCommit ERR: Magic program account not found" | |
| ); | |
| .ok_or_else(|| { | |
| ic_msg!( | |
| invoke_context, | |
| "VerifyValidatorIdentity ERR: Magic program account not found" | |
| ); | |
| InstructionError::UnsupportedProgramId | |
| })?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/verify_validator_identity.rs` around lines 32 - 35,
Update the error logging string in the VerifyValidatorIdentity handler so it
correctly references the instruction name instead of "ScheduleCommit"; locate
the ic_msg! call in the VerifyValidatorIdentity logic that currently logs
"ScheduleCommit ERR: Magic program account not found" and change it to a
descriptive message like "VerifyValidatorIdentity ERR: Magic program account not
found" (or similar) so logs correctly reflect the VerifyValidatorIdentity
instruction context.
| fn setup_transaction_accounts() -> Vec<(Pubkey, AccountSharedData)> { | ||
| let mut account_data = HashMap::new(); | ||
| account_data.insert( | ||
| MAGIC_CONTEXT_PUBKEY, | ||
| AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()), | ||
| ); | ||
| ensure_started_validator(&mut account_data, None); | ||
| let validator = validator_authority_id(); | ||
| vec![ | ||
| ( | ||
| MAGIC_CONTEXT_PUBKEY, | ||
| account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(), | ||
| ), | ||
| (validator, account_data.remove(&validator).unwrap()), | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test helper uses .unwrap() on HashMap::remove() which could silently fail.
While .unwrap() is acceptable in test code, if ensure_started_validator doesn't populate the expected keys, the test would panic with a generic message. Consider using .expect() with a descriptive message for better debugging.
💡 Optional improvement for test debuggability
vec![
(
MAGIC_CONTEXT_PUBKEY,
- account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(),
+ account_data.remove(&MAGIC_CONTEXT_PUBKEY)
+ .expect("MAGIC_CONTEXT_PUBKEY should be in account_data"),
),
- (validator, account_data.remove(&validator).unwrap()),
+ (validator, account_data.remove(&validator)
+ .expect("validator should be in account_data")),
]📝 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.
| fn setup_transaction_accounts() -> Vec<(Pubkey, AccountSharedData)> { | |
| let mut account_data = HashMap::new(); | |
| account_data.insert( | |
| MAGIC_CONTEXT_PUBKEY, | |
| AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()), | |
| ); | |
| ensure_started_validator(&mut account_data, None); | |
| let validator = validator_authority_id(); | |
| vec![ | |
| ( | |
| MAGIC_CONTEXT_PUBKEY, | |
| account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(), | |
| ), | |
| (validator, account_data.remove(&validator).unwrap()), | |
| ] | |
| } | |
| fn setup_transaction_accounts() -> Vec<(Pubkey, AccountSharedData)> { | |
| let mut account_data = HashMap::new(); | |
| account_data.insert( | |
| MAGIC_CONTEXT_PUBKEY, | |
| AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()), | |
| ); | |
| ensure_started_validator(&mut account_data, None); | |
| let validator = validator_authority_id(); | |
| vec![ | |
| ( | |
| MAGIC_CONTEXT_PUBKEY, | |
| account_data.remove(&MAGIC_CONTEXT_PUBKEY) | |
| .expect("MAGIC_CONTEXT_PUBKEY should be in account_data"), | |
| ), | |
| (validator, account_data.remove(&validator) | |
| .expect("validator should be in account_data")), | |
| ] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/verify_validator_identity.rs` around lines 86 - 101,
In setup_transaction_accounts(), avoid using HashMap::remove().unwrap() which
yields an opaque panic; replace both unwrap() calls with expect(...) that
include descriptive messages referencing the keys and the helper that should
have populated them (e.g., mention MAGIC_CONTEXT_PUBKEY and the validator
returned by validator_authority_id()) so if ensure_started_validator(...) failed
to insert the entries the test failure will report which key was missing and
where (setup_transaction_accounts / ensure_started_validator).
1 participant
Summary
With callbacks it is importmant for developers to verify if callback was triggered by validator and not someone else. For that purpose they can now call VerifyValidatorIdentity in magicblock-program with "validator" account, magic-program will compare pubkeys and error if they do not match.
Compatibility
Testing
Checklist
Summary by CodeRabbit