chore: add a feature to compile only types needed for the SDK #117
Conversation
WalkthroughGates SDK vs program build paths with feature flags, widens and makes many dependencies optional in Cargo.toml, adds new public modules and a fast entrypoint (fast_process_instruction) when built without the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fast_process_instruction as FastEntry
participant Processor
participant Diff
participant Logger
Caller->>FastEntry: invoke fast_process_instruction(instruction, accounts)
FastEntry->>Processor: prepare & dispatch instruction
Processor->>Diff: compute/apply diff (optional)
Processor-->>FastEntry: ProgramResult (Ok / Err)
alt Ok
FastEntry-->>Caller: Some(Ok)
else Err
FastEntry->>Logger: pinocchio_log!(error)
FastEntry-->>Caller: Some(Err)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Files to pay extra attention to:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-10-15T11:45:25.802ZApplied to files:
📚 Learning: 2025-10-29T09:28:50.282ZApplied to files:
📚 Learning: 2025-10-16T10:04:26.576ZApplied to files:
🧬 Code graph analysis (1)src/lib.rs (1)
🔇 Additional comments (2)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
65-113: Restore compilability when defaults are disabled.
pinocchio,pinocchio-log,pinocchio-pubkey, andpinocchio-systemhave just been marked optional in Cargo, but this whole fast path still compiles undercfg(not(feature = "core")). Building withcargo check --no-default-features(or any config that drops the new default feature set without turning oncore) now tries to compile this block even though those crates are absent, so the build fails. Please gate the fast/entrypoint code on the features that actually enable these dependencies (e.g. wrap in something likecfg(all(not(feature = "core"), feature = "pinocchio", feature = "pinocchio-log", ...))or introduce a dedicated “program” feature that default enables the deps and guards this module) or keep the deps non-optional. Until then, minimal-feature builds regress.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(2 hunks)src/entrypoint.rs(1 hunks)src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
src/entrypoint.rssrc/lib.rs
📚 Learning: 2025-10-29T09:28:50.282Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 111
File: src/diff/algorithm.rs:1-1
Timestamp: 2025-10-29T09:28:50.282Z
Learning: The magicblock-labs/delegation-program project uses std throughout the codebase and no_std compatibility is not a goal. Do not suggest using core instead of std.
Applied to files:
src/lib.rs
🧬 Code graph analysis (1)
src/lib.rs (3)
src/state/delegation_metadata.rs (1)
discriminator(24-26)src/state/utils/discriminator.rs (1)
discriminator(22-22)src/entrypoint.rs (1)
entrypoint(15-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml(2 hunks)src/error.rs(1 hunks)src/lib.rs(5 hunks)src/processor/mod.rs(1 hunks)src/processor/utils/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
src/error.rssrc/lib.rs
📚 Learning: 2025-10-29T09:28:50.282Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 111
File: src/diff/algorithm.rs:1-1
Timestamp: 2025-10-29T09:28:50.282Z
Learning: The magicblock-labs/delegation-program project uses std throughout the codebase and no_std compatibility is not a goal. Do not suggest using core instead of std.
Applied to files:
src/lib.rs
📚 Learning: 2025-10-16T10:04:26.576Z
Learnt from: taco-paco
Repo: magicblock-labs/delegation-program PR: 105
File: src/args/call_handler.rs:6-8
Timestamp: 2025-10-16T10:04:26.576Z
Learning: In `src/args/call_handler.rs`, the `data` field of the `CallHandlerArgs` struct has no maximum length constraint by design, as it needs to support flexible instruction formats including custom discriminators and arguments in any format.
Applied to files:
src/lib.rs
🧬 Code graph analysis (1)
src/lib.rs (6)
src/state/delegation_metadata.rs (1)
discriminator(24-26)src/state/utils/discriminator.rs (1)
discriminator(22-22)src/state/commit_record.rs (1)
discriminator(30-32)src/state/delegation_record.rs (1)
discriminator(33-35)src/state/program_config.rs (1)
discriminator(15-17)src/entrypoint.rs (1)
entrypoint(13-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (12)
src/processor/mod.rs (1)
13-14: LGTM! Feature-gated module is properly declared.The
fastmodule is correctly gated behind thepinocchio-supportfeature and properly integrated with the fast processing path insrc/lib.rs.src/error.rs (1)
52-57: LGTM! Error conversion properly implemented for pinocchio.The conversion from
DlpErrortopinocchio::program_error::ProgramErrormirrors the existing implementation for solana'sProgramErrorand is correctly gated behind thepinocchio-supportfeature.src/lib.rs (6)
3-12: LGTM! Import gating is correctly structured.The imports are properly gated with
not(feature = "sdk")to exclude runtime-only types when building for SDK usage. Line 14'sdeclare_idmacro is correctly left ungated since it's needed in both modes.
21-26: LGTM! Module visibility correctly restricted for SDK builds.The modules are appropriately gated to exclude build-time and runtime-only components when the crate is used as an SDK.
46-49: LGTM! Pinocchio-compatible program ID properly declared.The
fastmodule provides a pinocchio-compatible program ID declaration that complements the standard declaration at line 44, enabling seamless usage with pinocchio SDK types.
51-63: LGTM! Security text properly gated.The security text is correctly excluded from SDK builds and when the entrypoint is disabled, while requiring the explicit
solana-security-txtfeature flag.
115-166: LGTM! Slow path properly gated and handles remaining instructions.The
slow_process_instructionfunction is correctly excluded from SDK builds and properly handles all discriminators not covered by the fast path. The logging in the panic path (lines 160-161) is appropriately gated by theloggingfeature.
65-113: DlpDiscriminator is not feature-gated; the original concern is incorrect.The code is correct. DlpDiscriminator has no feature gating and is always available—it is not gated by
not(feature = "sdk")as claimed in the review. Thefast_process_instructionfunction is properly gated with#[cfg(feature = "pinocchio-support")], theprocessor::fastmodule it calls is gated identically, and all referenced discriminator variants exist. No feature conflicts exist.Likely an incorrect or invalid review comment.
Cargo.toml (3)
28-44: LGTM! Feature structure correctly implements past feedback.The feature definitions properly address the concerns raised in past reviews:
sdk = ["no-entrypoint"]provides clear semantics (line 30)pinocchio-supportbundles all pinocchio-related optional dependencies (lines 32-38)- Default features enable
pinocchio-support, ensuring backward compatibility (lines 40-44)This structure ensures that
--no-default-featuresbuilds won't attempt to compile code referencing optional crates.Based on past review comments.
4-4: LGTM! Appropriate version bump for feature additions.Patch version increment to 1.1.3 is suitable for adding new optional features while maintaining backward compatibility.
73-74: LGTM! Proper configuration for custom cfg values.The
unexpected_cfgslint configuration correctly declares the customtarget_os = "solana"value. This should eliminate the need for the blanket#![allow(unexpected_cfgs)]attribute insrc/lib.rsline 1.src/processor/utils/mod.rs (1)
1-2: Feature gate oncurvemodule is necessary and appropriate.The
curvemodule contains a single functionis_on_curve_fastthat directly depends onpinocchio::pubkey::Pubkeyin its function signature. The function is tightly coupled to pinocchio types and syscalls, making the#[cfg(feature = "pinocchio-support")]gate justified and not overly restrictive. There are no generic cryptographic utilities that could be used independently.
| solana-program = { version = ">=1.16" } | ||
| bytemuck = { version = ">=1", features = [ "derive" ] } | ||
| num_enum = "^0.7.2" | ||
| thiserror = "^1.0.57" | ||
| solana-security-txt = { version = "1.1.1", optional = true } | ||
| solana-curve25519 = "2.2" | ||
| bincode = "1.3.3" | ||
| pinocchio-log = "0.5.0" | ||
| pinocchio = "0.9.2" | ||
| pinocchio-pubkey = "0.3.0" | ||
| pinocchio-system = "0.3.0" | ||
| rkyv = "0.7.45" | ||
| thiserror = { version = ">=1" } | ||
| solana-security-txt = { version = ">=1.1", optional = true } | ||
| solana-curve25519 = { version = ">=2.2", optional = true } | ||
| bincode = { version = "^1.3" } | ||
| pinocchio-log = { version = "0.5.0", optional = true } | ||
| pinocchio = { version = "0.9.2", optional = true } | ||
| pinocchio-pubkey = { version = "0.3.0", optional = true } | ||
| pinocchio-system = { version = "0.3.0", optional = true } | ||
| rkyv = { version = "0.7.45", optional = true } | ||
| static_assertions = "1.1.0" | ||
| strum = { version = "0.27.2", features = ["derive"] } | ||
| strum = { version = ">=0.27", features = ["derive"] } |
There was a problem hiding this comment.
Version constraints are too permissive and risk breakage.
Using >= without upper bounds (lines 51, 52, 54-56, 64) allows any future major version, which may introduce breaking changes. For instance, solana-program = ">=1.16" would accept version 2.0.0 or 3.0.0 if they exist, potentially breaking the build.
Apply more restrictive version constraints:
-solana-program = { version = ">=1.16" }
-bytemuck = { version = ">=1", features = [ "derive" ] }
+solana-program = { version = "^1.16" }
+bytemuck = { version = "^1.0", features = [ "derive" ] }-thiserror = { version = ">=1" }
-solana-security-txt = { version = ">=1.1", optional = true }
-solana-curve25519 = { version = ">=2.2", optional = true }
+thiserror = { version = "^1.0" }
+solana-security-txt = { version = "^1.1", optional = true }
+solana-curve25519 = { version = "^2.2", optional = true }-strum = { version = ">=0.27", features = ["derive"] }
+strum = { version = "^0.27", features = ["derive"] }The caret (^) operator allows compatible updates within the same major version, following semver guarantees.
📝 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.
| solana-program = { version = ">=1.16" } | |
| bytemuck = { version = ">=1", features = [ "derive" ] } | |
| num_enum = "^0.7.2" | |
| thiserror = "^1.0.57" | |
| solana-security-txt = { version = "1.1.1", optional = true } | |
| solana-curve25519 = "2.2" | |
| bincode = "1.3.3" | |
| pinocchio-log = "0.5.0" | |
| pinocchio = "0.9.2" | |
| pinocchio-pubkey = "0.3.0" | |
| pinocchio-system = "0.3.0" | |
| rkyv = "0.7.45" | |
| thiserror = { version = ">=1" } | |
| solana-security-txt = { version = ">=1.1", optional = true } | |
| solana-curve25519 = { version = ">=2.2", optional = true } | |
| bincode = { version = "^1.3" } | |
| pinocchio-log = { version = "0.5.0", optional = true } | |
| pinocchio = { version = "0.9.2", optional = true } | |
| pinocchio-pubkey = { version = "0.3.0", optional = true } | |
| pinocchio-system = { version = "0.3.0", optional = true } | |
| rkyv = { version = "0.7.45", optional = true } | |
| static_assertions = "1.1.0" | |
| strum = { version = "0.27.2", features = ["derive"] } | |
| strum = { version = ">=0.27", features = ["derive"] } | |
| solana-program = { version = "^1.16" } | |
| bytemuck = { version = "^1.0", features = [ "derive" ] } | |
| num_enum = "^0.7.2" | |
| thiserror = { version = "^1.0" } | |
| solana-security-txt = { version = "^1.1", optional = true } | |
| solana-curve25519 = { version = "^2.2", optional = true } | |
| bincode = { version = "^1.3" } | |
| pinocchio-log = { version = "0.5.0", optional = true } | |
| pinocchio = { version = "0.9.2", optional = true } | |
| pinocchio-pubkey = { version = "0.3.0", optional = true } | |
| pinocchio-system = { version = "0.3.0", optional = true } | |
| rkyv = { version = "0.7.45", optional = true } | |
| static_assertions = "1.1.0" | |
| strum = { version = "^0.27", features = ["derive"] } |
🤖 Prompt for AI Agents
In Cargo.toml around lines 51–64 (entries for solana-program, bytemuck,
thiserror, solana-security-txt, solana-curve25519, and rkyv), the version
constraints use open-ended >= ranges which can allow breaking major upgrades;
change each to a semver-compatible, restrictive constraint (e.g., use caret
ranges like ^1.16 or explicit upper-bound ranges like >=1.16, <2.0) so only
compatible patch/minor updates are accepted—update solana-program, bytemuck,
thiserror, solana-security-txt, solana-curve25519, and rkyv accordingly to
conservative caret or bounded ranges.
There was a problem hiding this comment.
I agree with CodeRabbit here: Version constraints are too permissive and risk breakage..
Especially this:
strum = { version = "^0.27", features = ["derive"] }is good, as we use it minimally anyway.
There was a problem hiding this comment.
@snawaz, thank you for confirming! You're absolutely right—using caret ranges like ^0.27 ensures that only compatible minor and patch updates are accepted, while blocking potentially breaking major version changes. This is especially important for dependencies with minimal usage like strum, where unexpected API changes could cause issues without much benefit.
The suggested changes would make the dependency management much safer across the board, particularly for the Solana-related crates where major version bumps could introduce significant breaking changes.
GabrielePicco
changed the title
My thoughts on feature designAs you mentioned, we should ideally have a separate crate for types that will eventually become part of the SDK. But that can sometimes feel like overkill, so here’s my general take on feature design:
I think this design keeps the features well-structured, readable, and easy to reason about later.
|
src/processor/mod.rs
Outdated
| mod validator_claim_fees; | ||
| mod whitelist_validator_for_program; | ||
|
|
||
| #[cfg(feature = "pinocchio-support")] |
There was a problem hiding this comment.
In my head when I see this on mod fast is think that pinocchio-support is some sort of faster library that I can compile dlp with, but if i don't want to there's still a slower version.
Bur it looks like if I compile without pinocchio-support I will not get:
commit_state, delegate, finalize, undelegate functionalities at all.
Does pinocchio-support means some sort of mini version of dlp?
There was a problem hiding this comment.
I totally agree. I think we need to rename this to processor (or no-processor) instead and apply it on all processors, irrespective of fast or slow.
GabrielePicco
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/lib.rs (1)
1-1: Reconsider suppressing unexpected_cfgs warnings globally.The blanket
#![allow(unexpected_cfgs)]suppresses an entire category of warnings that could catch real configuration errors. SinceCargo.tomllines 73-74 already configureunexpected_cfgswith a check-cfg fortarget_os, this global allow may be unnecessary.Cargo.toml (2)
51-64: Version constraints are too permissive and risk breakage.Using
>=without upper bounds allows any future major version, which may introduce breaking changes.Apply more restrictive version constraints using caret (
^) ranges for semver compatibility.
67-70: Consider more restrictive dev-dependency versions.Permissive
>=constraints could cause non-deterministic test failures when new versions are released.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
src/lib.rs
📚 Learning: 2025-10-29T09:28:50.282Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 111
File: src/diff/algorithm.rs:1-1
Timestamp: 2025-10-29T09:28:50.282Z
Learning: The magicblock-labs/delegation-program project uses std throughout the codebase and no_std compatibility is not a goal. Do not suggest using core instead of std.
Applied to files:
src/lib.rs
📚 Learning: 2025-10-16T10:04:26.576Z
Learnt from: taco-paco
Repo: magicblock-labs/delegation-program PR: 105
File: src/args/call_handler.rs:6-8
Timestamp: 2025-10-16T10:04:26.576Z
Learning: In `src/args/call_handler.rs`, the `data` field of the `CallHandlerArgs` struct has no maximum length constraint by design, as it needs to support flexible instruction formats including custom discriminators and arguments in any format.
Applied to files:
src/lib.rs
🧬 Code graph analysis (1)
src/lib.rs (2)
src/entrypoint.rs (1)
entrypoint(13-31)src/discriminator.rs (1)
name(48-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (8)
src/lib.rs (5)
3-10: Excellent compile-time safety checks.The mutual exclusion enforcement for
sdkandprogramfeatures prevents invalid configurations at compile time, which is preferable to runtime errors.
12-21: LGTM: Appropriate conditional imports.The imports are correctly gated for program builds only, aligning with the feature design where SDK builds don't need these types.
30-45: LGTM: Module visibility correctly gated.The module declarations and re-exports are appropriately feature-gated for program-only builds.
120-171: LGTM: Slow path correctly implements fallback.The
slow_process_instructionproperly handles discriminators not processed by the fast path. The logging at lines 165-166 is correctly feature-gated.
70-118: LGTM: Fast path with appropriate fallback.The
fast_process_instructioncorrectly returnsNonefor unhandled discriminators (line 116 in src/lib.rs), enabling fallback to the slow path. The error logging at line 87 correctly usespinocchio_log::log!. The entrypoint (src/entrypoint.rs:28-29) properly handles theNonereturn by callingslow_entrypoint(input), which deserializes and delegates toslow_process_instruction.Cargo.toml (3)
4-4: LGTM: Version bump.Version incremented to 1.1.3 for this release.
28-44: Feature design partially aligns with PR objectives.The feature design implements:
sdk = ["no-entrypoint"]- a composed feature ✓program = [dep:...]- a dependency activation featureno-entrypoint- a granular feature ✓The PR objectives reference snawaz's recommendation for a two-layer model with granular features (controlling specific concerns) and composed features (built from granular ones). While
sdkfollows this pattern,programacts primarily as a dependency enabler rather than composing granular behavioral features.This deviation appears to have been discussed in past comments, but consider whether additional granular features (e.g., "no-processor", "no-fast-path") would better align with the recommended architecture and make the codebase more flexible for future SDK/program variants.
Based on learnings from PR objectives.
73-74: LGTM: Proper lint configuration.The
unexpected_cfgslint configuration withcheck-cfgis the correct approach for handling custom cfg values liketarget_os = "solana". This makes the global#![allow(unexpected_cfgs)]insrc/lib.rsline 1 unnecessary.
@taco-paco @snawaz — this change turned out to be much messier than expected. The pinocchio-support feature was subsequently added to prevent invalid builds (specifically no default build and sdk). Conversely, we still need a feature that keeps Pinocchio dependencies optional so that the SDK doesn’t include them as transitive dependencies. I’ve rolled back to a simpler approach:
|
Summary by CodeRabbit
New Features
Chores