refactor: feature gate poseidon #1979
Conversation
- Remove proving keys caching and download from setup-and-build action - Remove @lightprotocol/programs from devDependencies in stateless.js, compressed-token, and CLI packages This simplifies the CI workflow and removes unnecessary build dependencies.
WalkthroughFeature-gate the Poseidon functionality across many Rust crates (enable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Hasher as Hasher::hashv
participant Solana as Solana Syscall
participant Native as Native Poseidon
Caller->>Hasher: hashv(bytes[])
alt target_os = "solana"
Hasher->>Solana: sol_poseidon(bytes[])
Solana-->>Hasher: Result / Err(u64)
Hasher-->>Caller: Ok(hash) or Err(PoseidonSyscallError::from(u64))
else target_os != "solana" and feature "poseidon"
Hasher->>Native: Poseidon::new(len).hash_bytes_be(bytes[])
Native-->>Hasher: Result / Err(PoseidonError)
Hasher-->>Caller: Ok(hash) or Err(HasherError::Poseidon)
else target_os != "solana" and feature not enabled
Hasher-->>Caller: Err(HasherError::PoseidonFeatureNotEnabled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
🧹 Nitpick comments (2)
sdk-libs/sdk/src/cpi/v1/invoke.rs (1)
261-303: Consider extracting the shared account-handling logic
with_light_account_poseidonduplicates the body ofwith_light_accountalmost line-for-line. A tiny helper that takes a generic LightAccount-like value (regular or poseidon) would keep the v1 code paths in sync and reduce future drift when the “main” method changes.sdk-libs/sdk/src/cpi/v2/invoke.rs (1)
98-154: Unify the poseidon and default pathsBoth poseidon variants mirror their non-poseidon counterparts line-by-line (for read-only and account-info flows). Hoisting the common conversion/push logic into a small private helper would keep the implementations locked together and shrink future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.github/actions/setup-and-build/action.yml(0 hunks)cli/package.json(0 hunks)forester-utils/Cargo.toml(1 hunks)forester/Cargo.toml(1 hunks)js/compressed-token/package.json(1 hunks)js/stateless.js/package.json(0 hunks)program-libs/batched-merkle-tree/Cargo.toml(1 hunks)program-libs/compressed-account/Cargo.toml(1 hunks)program-libs/concurrent-merkle-tree/Cargo.toml(1 hunks)program-libs/hasher/Cargo.toml(3 hunks)program-libs/hasher/src/errors.rs(5 hunks)program-libs/hasher/src/hash_chain.rs(2 hunks)program-libs/hasher/src/hash_to_field_size.rs(1 hunks)program-libs/hasher/src/poseidon.rs(3 hunks)program-libs/indexed-array/Cargo.toml(1 hunks)program-libs/indexed-merkle-tree/Cargo.toml(1 hunks)program-tests/account-compression-test/Cargo.toml(1 hunks)program-tests/batched-merkle-tree-test/Cargo.toml(1 hunks)program-tests/create-address-test-program/Cargo.toml(1 hunks)program-tests/registry-test/Cargo.toml(1 hunks)program-tests/system-cpi-test/Cargo.toml(1 hunks)program-tests/system-cpi-v2-test/Cargo.toml(1 hunks)program-tests/system-test/Cargo.toml(1 hunks)program-tests/utils/Cargo.toml(1 hunks)programs/compressed-token/Cargo.toml(1 hunks)prover/client/Cargo.toml(1 hunks)sdk-libs/client/Cargo.toml(1 hunks)sdk-libs/macros/Cargo.toml(1 hunks)sdk-libs/program-test/Cargo.toml(1 hunks)sdk-libs/program-test/src/utils/setup_light_programs.rs(1 hunks)sdk-libs/sdk-pinocchio/Cargo.toml(1 hunks)sdk-libs/sdk-types/Cargo.toml(1 hunks)sdk-libs/sdk/Cargo.toml(1 hunks)sdk-libs/sdk/src/account.rs(2 hunks)sdk-libs/sdk/src/cpi/instruction.rs(2 hunks)sdk-libs/sdk/src/cpi/v1/invoke.rs(2 hunks)sdk-libs/sdk/src/cpi/v2/invoke.rs(3 hunks)sdk-libs/sdk/src/lib.rs(1 hunks)sdk-tests/client-test/Cargo.toml(1 hunks)sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/Cargo.toml(1 hunks)sdk-tests/sdk-native-test/Cargo.toml(1 hunks)sdk-tests/sdk-pinocchio-v1-test/Cargo.toml(2 hunks)sdk-tests/sdk-pinocchio-v2-test/Cargo.toml(2 hunks)sdk-tests/sdk-v1-native-test/Cargo.toml(1 hunks)sparse-merkle-tree/Cargo.toml(1 hunks)xtask/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (3)
- cli/package.json
- .github/actions/setup-and-build/action.yml
- js/stateless.js/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
sdk-libs/sdk/src/cpi/v1/invoke.rs (1)
sdk-libs/sdk/src/account.rs (2)
account(577-579)account(641-643)
sdk-libs/sdk/src/cpi/instruction.rs (1)
sdk-libs/sdk/src/account.rs (2)
account(577-579)account(641-643)
sdk-libs/sdk/src/cpi/v2/invoke.rs (1)
sdk-libs/sdk/src/account.rs (2)
account(577-579)account(641-643)
⏰ 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). (20)
- GitHub Check: Test program-libs-slow
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: lint
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: cli-v2
🔇 Additional comments (6)
sdk-libs/sdk-pinocchio/Cargo.toml (1)
15-15: Poseidon gating looks good.Enabling
light-hasher'sposeidonfeature here aligns with the PR objective and keeps the dependency consistent with the new gating elsewhere. Nothing else to fix.sdk-tests/sdk-native-test/Cargo.toml (1)
25-25: Poseidon feature flag enabled for light-hasher.Change looks correct and aligns this test crate with the broader Poseidon gate rollout.
program-tests/registry-test/Cargo.toml (1)
33-33: Poseidon feature flag looks good.Enabling
light-hasherwith theposeidonfeature here keeps the registry test suite aligned with the workspace-wide feature gate roll-out. No issues spotted.program-libs/batched-merkle-tree/Cargo.toml (1)
12-12: Poseidon feature passthrough looks good.Adding this feature cleanly exposes
light-hasher/poseidonwithout affecting existing defaults. 👍xtask/Cargo.toml (1)
16-16: Align xtask with Poseidon-gated hasherEnabling the
poseidonfeature here keeps the xtask build consistent with the rest of the workspace after the new feature-gating rollout. Looks good.sdk-libs/program-test/Cargo.toml (1)
12-12: Poseidon feature forwarding looks good.Forwarding the new
poseidonflag directly tolight-hasher/poseidonkeeps feature gating consistent with the rest of the workspace.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
sdk-libs/sdk/src/cpi/v1/invoke.rs (1)
261-303: Consider consolidating duplicate logic between SHA and Poseidon variants.The
with_light_account_poseidonmethod duplicates ~40 lines of logic fromwith_light_account(lines 221-259). The methods differ only in:
- Type parameter:
LightAccountvsLightAccountPoseidon- Trait bound: presence of
DataHasherThis duplication increases maintenance burden—any bug fix or enhancement must be applied to both methods.
Consider one of these approaches:
Option 1: Generic helper method
// Private helper that both public methods can delegate to fn add_account_internal<Acc>( &mut self, account_info: impl CompressedAccountInfoTrait, ) -> Result<(), ProgramError> { // Shared logic here } fn with_light_account<A>(...) -> Result<Self, ProgramError> { let account_info = account.to_account_info()?; self.add_account_internal(account_info)?; Ok(self) } #[cfg(feature = "poseidon")] fn with_light_account_poseidon<A>(...) -> Result<Self, ProgramError> { let account_info = account.to_account_info()?; self.add_account_internal(account_info)?; Ok(self) }Option 2: Trait-based abstraction
Extract a trait that bothLightAccountandLightAccountPoseidonimplement, allowing a single generic implementation.sdk-libs/sdk/src/cpi/v2/invoke.rs (2)
98-154: Consider consolidating duplicate logic for InstructionDataInvokeCpiWithReadOnly.This method duplicates the logic from
with_light_account(lines 44-96), differing only in the type parameter andDataHashertrait bound. The same duplication pattern appears in the v1 implementation.Consider extracting shared logic into a private helper method or using trait-based abstraction (similar to the suggestion in
sdk-libs/sdk/src/cpi/v1/invoke.rslines 261-303). This would:
- Reduce maintenance burden
- Ensure bug fixes apply to both variants
- Keep the codebase DRY
223-242: Consider consolidating duplicate logic for InstructionDataInvokeCpiWithAccountInfo.This method duplicates the simpler logic from
with_light_account(lines 206-221). While shorter than the previous duplication, it still represents maintenance overhead.The same refactoring approach suggested for lines 98-154 applies here—extract common logic to avoid maintaining parallel implementations.
sdk-libs/sdk-pinocchio/Cargo.toml (1)
15-15: Verify whether poseidon should be an optional feature here.Unlike sdk-types which gates poseidon behind a feature flag (line 13 in sdk-types/Cargo.toml:
poseidon = ["light-hasher/poseidon"]), this crate unconditionally enables poseidon for all builds. If Poseidon functionality is not always required by sdk-pinocchio, consider making it optional to maintain consistency with the broader feature-gating approach.Apply this diff if poseidon should be optional:
+[features] +default = [] +v2 = ["light-sdk-types/v2"] +poseidon = ["light-hasher/poseidon"] + [dependencies] pinocchio = { workspace = true } -light-hasher = { workspace = true, features = ["poseidon"] } +light-hasher = { workspace = true }program-libs/hasher/src/poseidon.rs (1)
88-88: Consider removing the underscore prefix from the parameter name.The parameter was renamed from
valsto_vals. The underscore prefix conventionally signals an intentionally unused variable to suppress compiler warnings, but this parameter is actively used throughout the function (lines 96, 97, 121, 122).Consider either:
- Reverting to
valsif the variable is meant to be used- Or clarifying in a comment why the underscore prefix is intentional despite usage
Apply this diff to remove the underscore prefix:
- fn hashv(_vals: &[&[u8]]) -> Result<Hash, HasherError> { + fn hashv(vals: &[&[u8]]) -> Result<Hash, HasherError> { // Perform the calculation inline, calling this from within a program is // not supported. #[cfg(all(not(target_os = "solana"), feature = "poseidon"))] { use ark_bn254::Fr; use light_poseidon::{Poseidon, PoseidonBytesHasher}; - let mut hasher = Poseidon::<Fr>::new_circom(_vals.len())?; - let res = hasher.hash_bytes_be(_vals)?; + let mut hasher = Poseidon::<Fr>::new_circom(vals.len())?; + let res = hasher.hash_bytes_be(vals)?; Ok(res) } #[cfg(all(not(target_os = "solana"), not(feature = "poseidon")))] { Err(HasherError::PoseidonFeatureNotEnabled) } // Call via a system call to perform the calculation. #[cfg(target_os = "solana")] { use crate::HASH_BYTES; // TODO: reenable once LightHasher refactor is merged // solana_program::msg!("remove len check onchain."); // for val in vals { // if val.len() != 32 { // return Err(HasherError::InvalidInputLength(val.len())); // } // } let mut hash_result = [0; HASH_BYTES]; let result = unsafe { crate::syscalls::sol_poseidon( 0, // bn254 0, // big-endian - _vals as *const _ as *const u8, - _vals.len() as u64, + vals as *const _ as *const u8, + vals.len() as u64, &mut hash_result as *mut _ as *mut u8, ) }; match result { 0 => Ok(hash_result), e => Err(HasherError::from(PoseidonSyscallError::from(e))), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.github/actions/setup-and-build/action.yml(0 hunks)cli/package.json(0 hunks)forester-utils/Cargo.toml(1 hunks)forester/Cargo.toml(1 hunks)js/compressed-token/package.json(1 hunks)js/stateless.js/package.json(0 hunks)program-libs/batched-merkle-tree/Cargo.toml(1 hunks)program-libs/compressed-account/Cargo.toml(1 hunks)program-libs/concurrent-merkle-tree/Cargo.toml(1 hunks)program-libs/hasher/Cargo.toml(3 hunks)program-libs/hasher/src/errors.rs(5 hunks)program-libs/hasher/src/hash_chain.rs(2 hunks)program-libs/hasher/src/hash_to_field_size.rs(1 hunks)program-libs/hasher/src/poseidon.rs(3 hunks)program-libs/indexed-array/Cargo.toml(1 hunks)program-libs/indexed-merkle-tree/Cargo.toml(1 hunks)program-tests/account-compression-test/Cargo.toml(1 hunks)program-tests/batched-merkle-tree-test/Cargo.toml(1 hunks)program-tests/create-address-test-program/Cargo.toml(1 hunks)program-tests/registry-test/Cargo.toml(1 hunks)program-tests/system-cpi-test/Cargo.toml(1 hunks)program-tests/system-cpi-v2-test/Cargo.toml(1 hunks)program-tests/system-test/Cargo.toml(1 hunks)program-tests/utils/Cargo.toml(1 hunks)programs/compressed-token/Cargo.toml(1 hunks)prover/client/Cargo.toml(1 hunks)sdk-libs/client/Cargo.toml(1 hunks)sdk-libs/macros/Cargo.toml(1 hunks)sdk-libs/program-test/Cargo.toml(1 hunks)sdk-libs/program-test/src/utils/setup_light_programs.rs(1 hunks)sdk-libs/sdk-pinocchio/Cargo.toml(1 hunks)sdk-libs/sdk-types/Cargo.toml(1 hunks)sdk-libs/sdk/Cargo.toml(1 hunks)sdk-libs/sdk/src/account.rs(2 hunks)sdk-libs/sdk/src/cpi/instruction.rs(2 hunks)sdk-libs/sdk/src/cpi/v1/invoke.rs(2 hunks)sdk-libs/sdk/src/cpi/v2/invoke.rs(3 hunks)sdk-libs/sdk/src/lib.rs(1 hunks)sdk-tests/client-test/Cargo.toml(1 hunks)sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/Cargo.toml(1 hunks)sdk-tests/sdk-native-test/Cargo.toml(1 hunks)sdk-tests/sdk-pinocchio-v1-test/Cargo.toml(2 hunks)sdk-tests/sdk-pinocchio-v2-test/Cargo.toml(2 hunks)sdk-tests/sdk-v1-native-test/Cargo.toml(1 hunks)sparse-merkle-tree/Cargo.toml(1 hunks)xtask/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (3)
- cli/package.json
- .github/actions/setup-and-build/action.yml
- js/stateless.js/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
sdk-libs/sdk/src/cpi/instruction.rs (1)
sdk-libs/sdk/src/account.rs (2)
account(577-579)account(641-643)
sdk-libs/sdk/src/cpi/v1/invoke.rs (2)
sdk-libs/sdk/src/account.rs (2)
account(577-579)account(641-643)sdk-libs/sdk/src/cpi/invoke.rs (2)
invoke(13-13)invoke(39-84)
sdk-libs/sdk/src/cpi/v2/invoke.rs (1)
sdk-libs/sdk/src/account.rs (2)
account(577-579)account(641-643)
⏰ 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). (20)
- GitHub Check: Test program-libs-slow
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: lint
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: cli-v2
🔇 Additional comments (35)
xtask/Cargo.toml (1)
16-16: Poseidon feature enablement makes sense here.Switching
light-hasherto opt intoposeidonmatches the PR’s gating strategy and keeps xtask aligned with the rest of the workspace. Looks good.sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/Cargo.toml (1)
25-27: Poseidon feature enablement looks goodAdding the
poseidonfeature for bothlight-hasherandlight-sdkkeeps this test program aligned with the new feature gate strategy; no further adjustments needed.sdk-tests/sdk-pinocchio-v1-test/Cargo.toml (1)
25-25: Poseidon feature enablement looks goodOpting this test crate into
poseidon(andsolanafor dev) keeps it aligned with the wider workspace gating; no issues spotted.Also applies to: 34-34
sdk-libs/macros/Cargo.toml (1)
29-30: Poseidon gating looks good.Keeping
light-hasheras a base dependency and enablingposeidononly through the dev-dependency cleanly gates the feature while preserving local test coverage. 👍sdk-libs/sdk/Cargo.toml (1)
24-24: Poseidon feature wiring looks good.Dependency feature forwarding is consistent with the rest of the manifest.
sdk-libs/client/Cargo.toml (1)
13-13: Confirm downstream feature propagation.Should this feature also enable
light-sdk/poseidonso the SDK’s Poseidon-gated APIs compile whenlight-clientconsumers opt in? Please double-check and wire it through if needed.sdk-libs/sdk/src/lib.rs (1)
161-162: LGTM! Feature-gated import is correctly implemented.The conditional import of
DataHasherbehind the "poseidon" feature flag is properly structured and aligns with the PR's objective to make Poseidon functionality opt-in.sdk-libs/sdk/src/cpi/instruction.rs (2)
3-4: LGTM! Conditional import properly structured.The DataHasher import is correctly gated behind the "poseidon" feature, ensuring it's only available when the feature is enabled.
43-50: LGTM! Method properly feature-gated in trait definition.The
with_light_account_poseidonmethod is correctly gated behind the "poseidon" feature flag in the trait definition, maintaining API consistency.sdk-libs/sdk/src/cpi/v1/invoke.rs (1)
5-6: LGTM! Poseidon-specific imports properly gated.The conditional imports for
LightAccountPoseidonandDataHasherare correctly structured behind the "poseidon" feature flag.sdk-libs/sdk/src/cpi/v2/invoke.rs (1)
9-10: LGTM! Poseidon imports correctly feature-gated.The conditional imports are properly structured behind the "poseidon" feature flag.
sdk-libs/sdk-types/Cargo.toml (1)
13-13: LGTM! Feature flag properly declared.The poseidon feature correctly enables the light-hasher/poseidon feature, following Rust's feature composition pattern.
program-tests/system-cpi-v2-test/Cargo.toml (1)
25-25: LGTM! Poseidon feature enabled for testing.The dev-dependency correctly enables the poseidon feature for light-hasher, consistent with the PR's feature-gating approach.
program-libs/compressed-account/Cargo.toml (2)
18-18: LGTM! Feature composition correctly defined.The poseidon feature properly enables both the optional light-poseidon dependency and the light-hasher/poseidon feature using correct Cargo feature syntax.
24-24: LGTM! Optional dependency correctly declared.The light-poseidon dependency is properly marked as optional and will be included only when the poseidon feature is enabled.
sdk-libs/sdk/src/account.rs (3)
131-132: LGTM! Poseidon import properly feature-gated.The conditional import ensures Poseidon is only available when the poseidon feature is enabled.
135-135: LGTM! Re-export correctly updated.Poseidon has been properly removed from the unconditional public re-exports, now only available through the feature-gated module.
156-179: LGTM! Poseidon module properly feature-gated.The module is correctly compiled only when the poseidon feature is enabled, maintaining consistent feature boundaries.
program-libs/hasher/Cargo.toml (3)
14-14: LGTM! Core poseidon feature correctly defined.The feature properly composes three optional dependencies (light-poseidon, ark-bn254, ark-ff) required for Poseidon functionality.
18-18: LGTM! Optional dependencies properly declared.All three dependencies required by the poseidon feature are correctly marked as optional, with ark-bn254 and ark-ff appropriately scoped to non-Solana targets.
Also applies to: 32-32, 35-35
47-47: LGTM! Dev-dependency correctly declared.The non-optional dev-dependency ensures Poseidon functionality is always available for testing within this crate.
program-tests/system-cpi-test/Cargo.toml (1)
30-30: LGTM! Poseidon feature enabled for testing.The dependency correctly enables the poseidon feature for light-hasher in this test program.
program-tests/batched-merkle-tree-test/Cargo.toml (1)
20-20: LGTM! Multiple features correctly enabled.The dev-dependency properly enables both the solana and poseidon features for light-hasher in this test crate.
program-libs/indexed-merkle-tree/Cargo.toml (1)
25-25: LGTM!The explicit enablement of the
poseidonfeature forlight-hasheraligns with the broader refactor to feature-gate Poseidon functionality across the codebase.program-libs/batched-merkle-tree/Cargo.toml (1)
12-12: LGTM!Adding
poseidonas an optional feature is good practice, allowing consumers to opt-in to Poseidon functionality only when needed.program-libs/concurrent-merkle-tree/Cargo.toml (1)
17-17: LGTM!Consistent with the pattern established in other crates to explicitly enable the
poseidonfeature forlight-hasher.program-libs/hasher/src/hash_to_field_size.rs (1)
103-103: LGTM!Gating the test module behind the
poseidonfeature is appropriate, as these tests exercise Poseidon-specific functionality.program-libs/hasher/src/poseidon.rs (2)
40-57: LGTM!The
From<u64>implementation forPoseidonSyscallErrorprovides a clean way to convert syscall error codes into typed errors. The match arms comprehensively cover all error variants with anUnexpectedfallback for unknown codes.
91-104: LGTM!The feature-gating logic is well-structured:
- When
poseidonfeature is enabled (non-Solana): use the light-poseidon implementation- When
poseidonfeature is disabled (non-Solana): return a clear error message- On Solana: always use syscalls regardless of feature flag
This provides a good developer experience with a clear error message when the feature is not enabled.
program-libs/hasher/src/hash_chain.rs (2)
76-76: LGTM!Simplifying the imports by removing dependencies that are only needed in feature-gated test sections makes the code cleaner. These dependencies are appropriately re-imported within their respective feature-gated blocks.
176-190: LGTM!The test for "input larger than modulus" is appropriately feature-gated behind
poseidonsince:
- It verifies Poseidon-specific error handling (
HasherError::Poseidon)- The
PoseidonErrortype is only available when the feature is enabled- The test exercises Poseidon-specific validation logic
program-libs/hasher/src/errors.rs (4)
1-2: LGTM!Conditionally importing
PoseidonErroronly when theposeidonfeature is enabled ensures clean compilation when the feature is disabled.
11-13: LGTM!The feature-gated
Poseidonvariant with#[from]attribute provides seamless error conversion fromPoseidonErrorwhen the feature is enabled, while keeping the error enum clean when the feature is disabled.
30-31: LGTM!The
PoseidonFeatureNotEnablederror variant provides a clear, actionable message to developers when they attempt to use Poseidon functionality without enabling the feature. The message explicitly mentions that syscalls are still accessible on Solana.
40-49: LGTM!The error code assignments are well-structured:
7002for Poseidon errors (feature-gated, maps to the library error)7010for feature-not-enabled (always available)These codes don't conflict with existing error codes and provide clear differentiation between Poseidon-specific errors and the feature-not-enabled scenario.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
program-libs/compressed-account/Cargo.toml(2 hunks)program-libs/compressed-account/src/lib.rs(1 hunks)sdk-libs/client/Cargo.toml(2 hunks)
⏰ 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). (19)
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: lint
🔇 Additional comments (3)
program-libs/compressed-account/Cargo.toml (3)
18-18: LGTM: Feature definition is correct.The
poseidonfeature properly wires the optionallight-poseidondependency and enables the upstreamposeidonfeature inlight-hasher.
24-24: LGTM: Optional dependency correctly configured.The optional
light-poseidondependency uses workspace versioning and will only be included when theposeidonfeature is enabled.
40-41: LGTM: Dev-dependencies enable Poseidon unconditionally for tests.This setup allows tests to always use Poseidon functionality without requiring the feature flag, which is a reasonable development pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/rust.yml(1 hunks)program-libs/compressed-account/src/compressed_account.rs(1 hunks)sdk-libs/sdk/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk-libs/sdk/Cargo.toml
⏰ 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). (20)
- GitHub Check: Forester e2e test
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: lint
- GitHub Check: cli-v2
🔇 Additional comments (1)
.github/workflows/rust.yml (1)
51-51: Nice to see test coverage for poseidon + new-unique.Adding this dedicated run ensures the newly gated Poseidon path stays exercised alongside
new-unique. Looks good.
ananas-block
force-pushed
the
jorrit/refactor-feature-gate-poseidon
branch
from
bc23d83 to
202385a
Compare
Changes:
LightAccountdoes not needlight-poseidonlight-hasherand use it in downstream cratesSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores