refactor: reorganize to move api functionalities to dlp_api#152
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces a local dlp-api crate as a workspace dependency and updates Cargo features: root Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
83-84: 🧹 Nitpick | 🔵 TrivialClarify feature differences between dev-dependencies.
In dev-dependencies,
magicblock-delegation-programusesunit_test_configfeature whiledlp-apiuses default features (includingencryption). This is likely intentional for test coverage, but worth documenting if the test matrix relies on this distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 83 - 84, Add a brief comment in Cargo.toml (or the repository README/tests docs) explaining why the dev-dependency entries differ: that magicblock-delegation-program is included with the unit_test_config feature enabled while dlp-api is pulled with default features (including encryption), and note any test-matrix or coverage implications; reference the dev-dependency entries named "magicblock-delegation-program" (feature: unit_test_config) and "dlp-api" (default features/encryption) so readers understand this intentional distinction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Cargo.toml`:
- Around line 83-84: Add a brief comment in Cargo.toml (or the repository
README/tests docs) explaining why the dev-dependency entries differ: that
magicblock-delegation-program is included with the unit_test_config feature
enabled while dlp-api is pulled with default features (including encryption),
and note any test-matrix or coverage implications; reference the dev-dependency
entries named "magicblock-delegation-program" (feature: unit_test_config) and
"dlp-api" (default features/encryption) so readers understand this intentional
distinction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b61d89b6-ecfa-4369-90bf-bc56eb64df98
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomldlp-api/Cargo.tomldlp-api/src/account_size_class.rsdlp-api/src/args/call_handler.rsdlp-api/src/args/commit_state.rsdlp-api/src/args/delegate.rsdlp-api/src/args/delegate_ephemeral_balance.rsdlp-api/src/args/delegate_with_actions.rsdlp-api/src/args/mod.rsdlp-api/src/args/top_up_ephemeral_balance.rsdlp-api/src/args/types.rsdlp-api/src/args/validator_claim_fees.rsdlp-api/src/args/whitelist_validator_for_program.rsdlp-api/src/compact/account_meta.rsdlp-api/src/compact/instruction.rsdlp-api/src/compact/mod.rsdlp-api/src/consts.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/lib.rsdlp-api/src/pda.rsdlp-api/src/pod_view.rsdlp-api/src/requires.rsdlp-api/src/state/commit_record.rsdlp-api/src/state/delegation_metadata.rsdlp-api/src/state/delegation_record.rsdlp-api/src/state/mod.rsdlp-api/src/state/program_config.rsdlp-api/src/state/utils/discriminator.rsdlp-api/src/state/utils/mod.rsdlp-api/src/state/utils/to_bytes.rsdlp-api/src/state/utils/try_from_bytes.rssrc/lib.rs
💤 Files with no reviewable changes (2)
- dlp-api/src/consts.rs
- dlp-api/src/error.rs
snawaz
changed the title
snawaz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 49: The root Cargo.toml's unit_test_config feature is not forwarded to
the dlp-api dependency, so when magicblock-delegation-program enables
unit_test_config the dlp-api crate still builds without its unit_test_config
feature and its test-gated code (e.g., DEFAULT_VALIDATOR_IDENTITY in
dlp-api/src/consts.rs and require_authorization in dlp-api/src/requires.rs)
stays on production branches; fix this by updating the dependency entry for
dlp-api in Cargo.toml (the existing dlp-api = { package =
"magicblock-delegation-program-api", path = "dlp-api", default-features = false
} line) to include features = ["unit_test_config"] behind a conditional
forwarding of the root feature (i.e., forward the root unit_test_config to the
dependency) so enabling the root feature also enables dlp-api's
unit_test_config.
🪄 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: b9cdcc78-4f9d-4a77-85b1-32a7300e1213
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomldlp-api/Cargo.tomldlp-api/src/account_size_class.rsdlp-api/src/args/call_handler.rsdlp-api/src/args/commit_state.rsdlp-api/src/args/delegate.rsdlp-api/src/args/delegate_ephemeral_balance.rsdlp-api/src/args/delegate_with_actions.rsdlp-api/src/args/mod.rsdlp-api/src/args/top_up_ephemeral_balance.rsdlp-api/src/args/types.rsdlp-api/src/args/validator_claim_fees.rsdlp-api/src/args/whitelist_validator_for_program.rsdlp-api/src/compact/account_meta.rsdlp-api/src/compact/instruction.rsdlp-api/src/compact/mod.rsdlp-api/src/consts.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/lib.rsdlp-api/src/pda.rsdlp-api/src/pod_view.rsdlp-api/src/requires.rsdlp-api/src/state/commit_record.rsdlp-api/src/state/delegation_metadata.rsdlp-api/src/state/delegation_record.rsdlp-api/src/state/mod.rsdlp-api/src/state/program_config.rsdlp-api/src/state/utils/discriminator.rsdlp-api/src/state/utils/mod.rsdlp-api/src/state/utils/to_bytes.rsdlp-api/src/state/utils/try_from_bytes.rssrc/lib.rs
💤 Files with no reviewable changes (2)
- dlp-api/src/error.rs
- dlp-api/src/consts.rs
snawaz
changed the title
snawaz
force-pushed
the
snawaz/restruct
branch
2 times, most recently
from
7f67404 to
a40fa02
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib.rs (1)
49-54: 🛠️ Refactor suggestion | 🟠 MajorUse
dlp_api::fastas the single source of truth.The same Pinocchio program ID is now declared here and in
dlp-api/src/lib.rs. Re-exporting the API crate’sfastmodule keeps the new crate boundary clean and removes a future drift point.Suggested refactor
-#[cfg(any(feature = "processor", feature = "pinocchio-rt"))] -pub mod fast { - pinocchio::address::declare_id!( - "DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh" - ); -} +#[cfg(any(feature = "processor", feature = "pinocchio-rt"))] +pub use dlp_api::fast;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 49 - 54, Remove the local duplicate fast module and its declare_id! invocation and instead re-export the API crate’s module so dlp_api::fast is the single source of truth; replace the existing pub mod fast { pinocchio::address::declare_id!(...); } block with a re-export like pub use dlp_api::fast; so the program ID is maintained only in dlp_api and this crate forwards that module.dlp-api/src/instruction_builder/delegate_with_actions.rs (1)
29-35:⚠️ Potential issue | 🟠 MajorRemove
.expect()calls or encode preconditions in the function signature.Line 31 panics when
validatorisNone, butDelegateArgs.validatorisOption<Pubkey>. Either require validator in the type signature or returnResult<Instruction, _>to surface the error. Line 34 panics on encryption failure—propagate theEncryptionErrorinstead of panicking.(Line 71's
to_vec().unwrap()is acceptable; serialization in instruction builders follows standard patterns.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dlp-api/src/instruction_builder/delegate_with_actions.rs` around lines 29 - 35, The code currently panics via expect() when DelegateArgs.validator is None and when actions.encrypt(...) fails; change delegate_with_actions.rs to surface these failures instead: either make the function require a validator in its signature (turn DelegateArgs.validator into Pubkey) or, preferably, change the function return type to Result<Instruction, E> (or a crate error type) and replace the expect on delegate.validator with an early-return Err when missing, and replace the expect on actions.encrypt(...) to propagate the EncryptionError (e.g., map or ?-return the error) so both missing validator and encryption failures are returned to the caller instead of panicking. Ensure references to delegate.validator and actions.encrypt are updated to use the new error-returning control flow.dlp-api/src/requires.rs (2)
835-845: 🧹 Nitpick | 🔵 TrivialConsider adding a SAFETY comment for the raw pointer cast.
The bounds checking at line 831 ensures sufficient data length, but the
unsafeblock performing a raw pointer cast to&Addresswould benefit from an explicit SAFETY comment documenting the invariants being upheld (32-byte slice, alignment requirements for Address type).📝 Suggested documentation
let bytes = &data[offset_of_upgrade_authority_address + 1 ..offset_of_upgrade_authority_address + 33]; + // SAFETY: `bytes` is exactly 32 bytes (verified by the length check above), + // and `Address` is a 32-byte array type with alignment of 1. let upgrade_authority_address = unsafe { &*(bytes.as_ptr() as *const Address) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dlp-api/src/requires.rs` around lines 835 - 845, Add a SAFETY comment immediately above the unsafe cast that documents the invariants that make the raw pointer cast safe: that the slice "bytes" is exactly 32 bytes (checked by the earlier bounds check on data using offset_of_upgrade_authority_address), that the memory is properly aligned for Address, that the lifetime of the resulting &Address does not outlive data, and that the bytes represent a valid Address bit pattern; reference the variables involved (bytes, upgrade_authority_address, Address, data, offset_of_upgrade_authority_address, admin.address(), and the require_eq_keys check) so future readers know why the unsafe block is sound.
523-543:⚠️ Potential issue | 🟡 MinorRemove unused
require_programfunction.The
#[allow(dead_code)]attribute indicates this function is never called. Verification confirms it has no usages anywhere in the codebase or tests. Dead code should be removed to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dlp-api/src/requires.rs` around lines 523 - 543, Remove the dead helper function require_program and its attributes to eliminate unused code; specifically delete the function signature and body that reference AccountView, Address, and ProgramError (including the #[inline(always)] and #[allow(dead_code)] annotations) from requires.rs, and run cargo check/tests to confirm no references remain and nothing else needs adjustment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Around line 4-7: The crate's public API omits re-exporting check_id(),
breaking callers that expect dlp::check_id(...); update the pub use
dlp_api::{...} declaration to include check_id alongside ID and id so the root
crate re-exports the same symbols as solana_program::declare_id! (i.e., add
check_id to the list in the pub use dlp_api block that currently exports
account_size_class, args, ..., ID).
---
Outside diff comments:
In `@dlp-api/src/instruction_builder/delegate_with_actions.rs`:
- Around line 29-35: The code currently panics via expect() when
DelegateArgs.validator is None and when actions.encrypt(...) fails; change
delegate_with_actions.rs to surface these failures instead: either make the
function require a validator in its signature (turn DelegateArgs.validator into
Pubkey) or, preferably, change the function return type to Result<Instruction,
E> (or a crate error type) and replace the expect on delegate.validator with an
early-return Err when missing, and replace the expect on actions.encrypt(...) to
propagate the EncryptionError (e.g., map or ?-return the error) so both missing
validator and encryption failures are returned to the caller instead of
panicking. Ensure references to delegate.validator and actions.encrypt are
updated to use the new error-returning control flow.
In `@dlp-api/src/requires.rs`:
- Around line 835-845: Add a SAFETY comment immediately above the unsafe cast
that documents the invariants that make the raw pointer cast safe: that the
slice "bytes" is exactly 32 bytes (checked by the earlier bounds check on data
using offset_of_upgrade_authority_address), that the memory is properly aligned
for Address, that the lifetime of the resulting &Address does not outlive data,
and that the bytes represent a valid Address bit pattern; reference the
variables involved (bytes, upgrade_authority_address, Address, data,
offset_of_upgrade_authority_address, admin.address(), and the require_eq_keys
check) so future readers know why the unsafe block is sound.
- Around line 523-543: Remove the dead helper function require_program and its
attributes to eliminate unused code; specifically delete the function signature
and body that reference AccountView, Address, and ProgramError (including the
#[inline(always)] and #[allow(dead_code)] annotations) from requires.rs, and run
cargo check/tests to confirm no references remain and nothing else needs
adjustment.
In `@src/lib.rs`:
- Around line 49-54: Remove the local duplicate fast module and its declare_id!
invocation and instead re-export the API crate’s module so dlp_api::fast is the
single source of truth; replace the existing pub mod fast {
pinocchio::address::declare_id!(...); } block with a re-export like pub use
dlp_api::fast; so the program ID is maintained only in dlp_api and this crate
forwards that module.
🪄 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: 99f76fd1-e601-4fd3-b3a8-19a9c30c7a31
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
Cargo.tomldlp-api/Cargo.tomldlp-api/src/account_size_class.rsdlp-api/src/args/call_handler.rsdlp-api/src/args/commit_state.rsdlp-api/src/args/delegate.rsdlp-api/src/args/delegate_ephemeral_balance.rsdlp-api/src/args/delegate_with_actions.rsdlp-api/src/args/mod.rsdlp-api/src/args/top_up_ephemeral_balance.rsdlp-api/src/args/types.rsdlp-api/src/args/validator_claim_fees.rsdlp-api/src/args/whitelist_validator_for_program.rsdlp-api/src/compact/account_meta.rsdlp-api/src/compact/instruction.rsdlp-api/src/compact/mod.rsdlp-api/src/consts.rsdlp-api/src/diff/algorithm.rsdlp-api/src/diff/mod.rsdlp-api/src/diff/types.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/delegate_with_actions.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/lib.rsdlp-api/src/pda.rsdlp-api/src/pod_view.rsdlp-api/src/requires.rsdlp-api/src/state/commit_record.rsdlp-api/src/state/delegation_metadata.rsdlp-api/src/state/delegation_record.rsdlp-api/src/state/mod.rsdlp-api/src/state/program_config.rsdlp-api/src/state/utils/discriminator.rsdlp-api/src/state/utils/mod.rsdlp-api/src/state/utils/to_bytes.rsdlp-api/src/state/utils/try_from_bytes.rssrc/lib.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
💤 Files with no reviewable changes (2)
- dlp-api/src/consts.rs
- dlp-api/src/error.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dlp-api/src/instruction_builder/delegate_with_actions.rs (1)
29-34:⚠️ Potential issue | 🟠 MajorAvoid panic paths in the instruction builder.
Line 31, Line 34, and Line 71 use
.expect()/.unwrap()in production code. These can abort callers on recoverable build failures (missing validator, encryption error, serialization error).Proposed fix
-pub fn delegate_with_actions( +pub fn delegate_with_actions( payer: Pubkey, delegated_account: Pubkey, owner: Option<Pubkey>, delegate: DelegateArgs, actions: Vec<PostDelegationInstruction>, -) -> Instruction { +) -> Result<Instruction, DelegateWithActionsBuildError> { let encrypt_key = delegate .validator - .expect("validator must be provided for encryption"); + .ok_or(DelegateWithActionsBuildError::MissingValidator)?; let (actions, signers) = actions .encrypt(&encrypt_key) - .expect("post-delegation actions encryption failed"); + .map_err(DelegateWithActionsBuildError::Encryption)?; - Instruction { + Ok(Instruction { program_id: dlp::id(), accounts: { let owner = owner.unwrap_or(system_program::id()); @@ data: { let args = DelegateWithActionsArgs { delegate, actions }; let mut data = DlpDiscriminator::DelegateWithActions.to_vec(); - data.extend_from_slice(&to_vec(&args).unwrap()); + let encoded = to_vec(&args) + .map_err(DelegateWithActionsBuildError::Serialization)?; + data.extend_from_slice(&encoded); data }, - } + }) }As per coding guidelines
{dlp-api,src}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dlp-api/src/instruction_builder/delegate_with_actions.rs` around lines 29 - 34, The code currently panics by calling expect()/unwrap() when a validator is missing, when actions.encrypt fails, and during serialization; change the instruction builder (the function in delegate_with_actions.rs that constructs post-delegation actions) to return a Result and propagate errors instead of panicking: replace delegate.validator.expect(...) by matching the Option and returning an Err variant (with a clear error enum/variant) when None, replace actions.encrypt(...).expect(...) with actions.encrypt(&encrypt_key).map_err(|e| /* convert to your function's error */)? so the error is propagated, and similarly map or ? any serialization calls (the line around 71) to return errors instead of unwrap(); update callers to handle the Result. Use the unique symbols encrypt_key, delegate.validator, actions.encrypt, and the serialization call to locate and change the code.
♻️ Duplicate comments (1)
src/lib.rs (1)
4-7:⚠️ Potential issue | 🟠 MajorRe-export
check_idto preserve root API compatibility.Line 4-7 re-exports
id/IDbut omitscheck_id, which can break callers that previously useddlp::check_id(...).Proposed fix
pub use dlp_api::{ - account_size_class, args, compact, consts, diff, discriminator, error, id, + account_size_class, args, check_id, compact, consts, diff, discriminator, error, id, pda, pod_view, requires, state, ID, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 4 - 7, The crate currently re-exports id and ID from dlp_api but omits check_id, breaking callers using dlp::check_id; update the re-export statement that pulls from dlp_api (the existing `pub use dlp_api::{ ... id, ... ID, };` line) to include check_id so callers retain the original API surface: add check_id to the list of items exported from dlp_api (alongside id and ID) so dlp::check_id(...) continues to resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dlp-api/src/instruction_builder/delegate_with_actions.rs`:
- Around line 29-34: The code currently panics by calling expect()/unwrap() when
a validator is missing, when actions.encrypt fails, and during serialization;
change the instruction builder (the function in delegate_with_actions.rs that
constructs post-delegation actions) to return a Result and propagate errors
instead of panicking: replace delegate.validator.expect(...) by matching the
Option and returning an Err variant (with a clear error enum/variant) when None,
replace actions.encrypt(...).expect(...) with
actions.encrypt(&encrypt_key).map_err(|e| /* convert to your function's error
*/)? so the error is propagated, and similarly map or ? any serialization calls
(the line around 71) to return errors instead of unwrap(); update callers to
handle the Result. Use the unique symbols encrypt_key, delegate.validator,
actions.encrypt, and the serialization call to locate and change the code.
---
Duplicate comments:
In `@src/lib.rs`:
- Around line 4-7: The crate currently re-exports id and ID from dlp_api but
omits check_id, breaking callers using dlp::check_id; update the re-export
statement that pulls from dlp_api (the existing `pub use dlp_api::{ ... id, ...
ID, };` line) to include check_id so callers retain the original API surface:
add check_id to the list of items exported from dlp_api (alongside id and ID) so
dlp::check_id(...) continues to resolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d865c06-448b-426a-a2c8-15a85b3f0907
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
Cargo.tomldlp-api/Cargo.tomldlp-api/src/account_size_class.rsdlp-api/src/args/call_handler.rsdlp-api/src/args/commit_state.rsdlp-api/src/args/delegate.rsdlp-api/src/args/delegate_ephemeral_balance.rsdlp-api/src/args/delegate_with_actions.rsdlp-api/src/args/mod.rsdlp-api/src/args/top_up_ephemeral_balance.rsdlp-api/src/args/types.rsdlp-api/src/args/validator_claim_fees.rsdlp-api/src/args/whitelist_validator_for_program.rsdlp-api/src/compact/account_meta.rsdlp-api/src/compact/instruction.rsdlp-api/src/compact/mod.rsdlp-api/src/consts.rsdlp-api/src/diff/algorithm.rsdlp-api/src/diff/mod.rsdlp-api/src/diff/types.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/delegate_with_actions.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/lib.rsdlp-api/src/pda.rsdlp-api/src/pod_view.rsdlp-api/src/requires.rsdlp-api/src/state/commit_record.rsdlp-api/src/state/delegation_metadata.rsdlp-api/src/state/delegation_record.rsdlp-api/src/state/mod.rsdlp-api/src/state/program_config.rsdlp-api/src/state/utils/discriminator.rsdlp-api/src/state/utils/mod.rsdlp-api/src/state/utils/to_bytes.rsdlp-api/src/state/utils/try_from_bytes.rssrc/lib.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
💤 Files with no reviewable changes (2)
- dlp-api/src/error.rs
- dlp-api/src/consts.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_undelegate_without_commit.rs (1)
63-71:⚠️ Potential issue | 🟡 MinorRemove duplicate assertion block.
Lines 68-71 duplicate the assertion at lines 63-66 (same variable, same check, same comment). This appears to be a copy-paste oversight.
🧹 Proposed fix
// Assert the delegation_record_pda was closed let delegation_record_account = banks.get_account(delegation_record_pda).await.unwrap(); assert!(delegation_record_account.is_none()); - // Assert the delegation_record_pda was closed - let delegation_record_account = - banks.get_account(delegation_record_pda).await.unwrap(); - assert!(delegation_record_account.is_none()); - // Assert the delegated account seeds pda was closed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_undelegate_without_commit.rs` around lines 63 - 71, Remove the duplicated assertion block that checks the delegation_record_pda closure: keep a single call to banks.get_account(delegation_record_pda).await.unwrap() assigned to delegation_record_account and the assert!(delegation_record_account.is_none()) check; delete the repeated lines (duplicate comment, call, and assert) so the test only asserts the delegation_record_pda was closed once.dlp-api/src/instruction_builder/delegate_with_actions.rs (1)
22-34:⚠️ Potential issue | 🟠 MajorReplace the panic paths in this public builder.
This API currently panics on missing
delegate.validator, encryption failure, and serialization failure. Please make the validator requirement explicit in the signature or returnResult<Instruction, _>so callers can handle these errors instead of aborting.As per coding guidelines:
{dlp-api,src}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants.Also applies to: 69-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dlp-api/src/instruction_builder/delegate_with_actions.rs` around lines 22 - 34, The public builder delegate_with_actions currently panics on missing delegate.validator and on encryption/serialization failures; change its signature to return Result<Instruction, YourErrorType> (or a suitable error enum) and propagate errors instead of calling expect, e.g., replace the validator extraction with a match on delegate.validator returning an Err if absent (or make validator a required Pubkey in the function signature or DelegateArgs type), replace the .encrypt(...).expect(...) with a fallible call using ? to propagate the error, and similarly propagate any serialization failure (the later serialization calls referenced around lines 69-71) so callers can handle errors rather than aborting. Ensure you update call sites to handle the Result.
♻️ Duplicate comments (1)
src/lib.rs (1)
5-8:⚠️ Potential issue | 🟠 MajorRoot ID helpers are crate-private, which can break existing
dlp::*callersLine 22 makes
id/IDinternal (pub(crate)), and there is no publiccheck_idre-export. If backward compatibility is required for the root crate, this is a breaking API change.Suggested compatibility-preserving change
-pub(crate) use dlp_api::{id, ID}; +pub use dlp_api::{check_id, id, ID};#!/bin/bash set -euo pipefail echo "Inspect ID helper re-exports in src/lib.rs" rg -nP 'use\s+dlp_api::\{[^}]*\b(id|ID|check_id)\b[^}]*\}' src/lib.rs echo echo "Show the exact section around current visibility" nl -ba src/lib.rs | sed -n '1,40p' echo echo "Check for any remaining in-repo callsites expecting root crate ID helpers" rg -nP '\bdlp::(id|ID|check_id)\b' --type rustAlso applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 5 - 8, The crate currently makes the root ID helpers internal (pub(crate)) which breaks external callers expecting dlp::id, dlp::ID, or dlp::check_id; update the re-exports so these symbols are public again by adding a public re-export or changing their visibility to pub (e.g., ensure id, ID, and check_id are included in a pub use of dlp_api rather than pub(crate)), and verify the module export list includes id, ID, and check_id so backward-compatible callers of those helpers continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dlp-api/Cargo.toml`:
- Around line 13-19: The crate declares a "diff" feature in Cargo.toml but
exposes the module unconditionally; update dlp-api/src/lib.rs to gate the diff
module with #[cfg(feature = "diff")] (e.g., change or wrap the `pub mod diff;`
declaration) so the feature actually toggles compilation, or alternatively
remove the "diff" feature from Cargo.toml's default/features list to reflect
that `pub mod diff;` is always compiled; ensure references to the module (uses
or tests) are also conditionally compiled with #[cfg(feature = "diff")] if you
choose the conditional-gating approach.
---
Outside diff comments:
In `@dlp-api/src/instruction_builder/delegate_with_actions.rs`:
- Around line 22-34: The public builder delegate_with_actions currently panics
on missing delegate.validator and on encryption/serialization failures; change
its signature to return Result<Instruction, YourErrorType> (or a suitable error
enum) and propagate errors instead of calling expect, e.g., replace the
validator extraction with a match on delegate.validator returning an Err if
absent (or make validator a required Pubkey in the function signature or
DelegateArgs type), replace the .encrypt(...).expect(...) with a fallible call
using ? to propagate the error, and similarly propagate any serialization
failure (the later serialization calls referenced around lines 69-71) so callers
can handle errors rather than aborting. Ensure you update call sites to handle
the Result.
In `@tests/test_undelegate_without_commit.rs`:
- Around line 63-71: Remove the duplicated assertion block that checks the
delegation_record_pda closure: keep a single call to
banks.get_account(delegation_record_pda).await.unwrap() assigned to
delegation_record_account and the assert!(delegation_record_account.is_none())
check; delete the repeated lines (duplicate comment, call, and assert) so the
test only asserts the delegation_record_pda was closed once.
---
Duplicate comments:
In `@src/lib.rs`:
- Around line 5-8: The crate currently makes the root ID helpers internal
(pub(crate)) which breaks external callers expecting dlp::id, dlp::ID, or
dlp::check_id; update the re-exports so these symbols are public again by adding
a public re-export or changing their visibility to pub (e.g., ensure id, ID, and
check_id are included in a pub use of dlp_api rather than pub(crate)), and
verify the module export list includes id, ID, and check_id so
backward-compatible callers of those helpers continue to work.
🪄 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: 5cb67fbb-dda0-4478-b289-e8717d9481c9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
Cargo.tomldlp-api/Cargo.tomldlp-api/src/account_size_class.rsdlp-api/src/args/call_handler.rsdlp-api/src/args/commit_state.rsdlp-api/src/args/delegate.rsdlp-api/src/args/delegate_ephemeral_balance.rsdlp-api/src/args/delegate_with_actions.rsdlp-api/src/args/mod.rsdlp-api/src/args/top_up_ephemeral_balance.rsdlp-api/src/args/types.rsdlp-api/src/args/validator_claim_fees.rsdlp-api/src/args/whitelist_validator_for_program.rsdlp-api/src/compact/account_meta.rsdlp-api/src/compact/instruction.rsdlp-api/src/compact/mod.rsdlp-api/src/consts.rsdlp-api/src/diff/algorithm.rsdlp-api/src/diff/mod.rsdlp-api/src/diff/types.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/delegate_with_actions.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/lib.rsdlp-api/src/pda.rsdlp-api/src/pod_view.rsdlp-api/src/requires.rsdlp-api/src/state/commit_record.rsdlp-api/src/state/delegation_metadata.rsdlp-api/src/state/delegation_record.rsdlp-api/src/state/mod.rsdlp-api/src/state/program_config.rsdlp-api/src/state/utils/discriminator.rsdlp-api/src/state/utils/mod.rsdlp-api/src/state/utils/to_bytes.rsdlp-api/src/state/utils/try_from_bytes.rssrc/lib.rstests/fixtures/accounts.rstests/test_call_handler.rstests/test_call_handler_v2.rstests/test_cleartext_with_insertable_encrypted.rstests/test_close_validator_fees_vault.rstests/test_commit_fees_on_undelegation.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rstests/test_commit_on_curve.rstests/test_commit_state.rstests/test_commit_state_from_buffer.rstests/test_commit_state_with_program_config.rstests/test_commit_undelegate_zero_lamports_system_owned.rstests/test_delegate.rstests/test_delegate_magic_fee_vault.rstests/test_delegate_on_curve.rstests/test_delegate_with_actions.rstests/test_delegation_confined_accounts.rstests/test_finalize.rstests/test_init_fees_vault.rstests/test_init_magic_fee_vault.rstests/test_init_validator_fees_vault.rstests/test_lamports_settlement.rstests/test_protocol_claim_fees.rstests/test_top_up.rstests/test_undelegate.rstests/test_undelegate_confined_account.rstests/test_undelegate_on_curve.rstests/test_undelegate_without_commit.rstests/test_validator_claim_fees.rstests/test_whitelist_validator_for_program.rs
💤 Files with no reviewable changes (2)
- dlp-api/src/error.rs
- dlp-api/src/consts.rs
snawaz
force-pushed
the
snawaz/restruct
branch
5 times, most recently
from
a8c01ae to
2bf5382
Compare
…onalities are in api and dlp depends on it
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.
With this PR, now we have:
dlp_apithat contains all user-facing types: args, state, ix builder, encryption/decryption, etc.dlpcontains the processors only (basically onchain-only specific stuff).From now on, users (sdk, validator etc) will use
dlp_apionly.