refactor: sdk tests #1911
refactor: sdk tests #1911
Conversation
WalkthroughWorkspace members were repointed from program-tests/* to sdk-tests/*; sdk-tests/sdk-test added a workspace dependency and a public ARRAY_LEN = 2200. Multiple structs, tests, and defaults were updated to use ARRAY_LEN, logging (msg!) was added, and a test feature gate was commented out. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
sdk-tests/sdk-test/src/create_pda.rs (2)
29-35: Avoid unwraps and unchecked indexing in CPI setup; return typed errors instead.
- Line 34 unwrap() will panic; on-chain this aborts the TX and masks the real cause.
- Lines 30-33 use unchecked indexing into accounts (Line 30 uses accounts[0], Line 31 uses a slice starting at system_accounts_offset). Both can panic if the input is malformed.
Refactor to validate the slice bounds and propagate errors. If try_new_with_config returns Result, prefer using
?. If it returns Option, useok_or(...)with a suitable LightSdkError variant.Apply along these lines (adjust the error variants to your LightSdkError enum):
- let cpi_accounts = CpiAccounts::try_new_with_config( - &accounts[0], - &accounts[instruction_data.system_accounts_offset as usize..], - config, - ) - .unwrap(); + let sys_off = instruction_data.system_accounts_offset as usize; + if accounts.is_empty() || sys_off > accounts.len() { + return Err(LightSdkError::InvalidAccounts); + } + let payer = &accounts[0]; + let remaining = &accounts[sys_off..]; + let cpi_accounts = CpiAccounts::try_new_with_config( + payer, + remaining, + config, + )?;
38-43: Replace unwrap() on hash derivation with error propagation.The call to hashv_to_bn254_field_size_be_const_array(...).unwrap() will panic on failure. Prefer mapping the error to LightSdkError and returning it.
Example (adjust based on the function’s return type: Result vs Option, and available error variants):
- let address_seed = hashv_to_bn254_field_size_be_const_array::<3>(&[ - b"compressed", - instruction_data.data.as_slice(), - ]) - .unwrap(); + let address_seed = hashv_to_bn254_field_size_be_const_array::<3>(&[ + b"compressed", + instruction_data.data.as_slice(), + ]) + .ok_or(LightSdkError::Hasher)?;
🧹 Nitpick comments (3)
sdk-tests/sdk-test/src/create_pda.rs (3)
24-26: Avoid name shadowing for instruction_data to improve readability.The mutable slice (Line 24) and the deserialized struct (Line 25) are both called instruction_data. Rename the struct instance to reduce cognitive load.
- let mut instruction_data = instruction_data; - let instruction_data = CreatePdaInstructionData::deserialize(&mut instruction_data) + let mut data = instruction_data; + let ix = CreatePdaInstructionData::deserialize(&mut data) .map_err(|_| LightSdkError::Borsh)?;Then update subsequent references from instruction_data.* to ix.*.
23-23: Gate diagnostic logs behind a feature to save CU in production.The added msg! calls are useful, but each costs CU. Consider gating them behind a cargo feature like trace to avoid accidental CU bloat.
For example:
- msg!("pre instruction_data"); + if cfg!(feature = "trace") { msg!("pre instruction_data"); } - msg!("pre config"); + if cfg!(feature = "trace") { msg!("pre config"); } - msg!("pre account"); + if cfg!(feature = "trace") { msg!("pre account"); }Also applies to: 27-27, 59-59
91-98: repr(C) on a Borsh-serialized struct is usually unnecessary; verify intent and padding implications.repr(C) doesn't affect Borsh serialization, but it may introduce padding in-memory and suggests FFI intent. If you don’t cross an FFI boundary with CreatePdaInstructionData, consider dropping repr(C) to avoid confusion.
Also, verify that the expanded data: [u8; ARRAY_LEN] field (Line 97) doesn't break backward compatibility in any consumers expecting the old size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)sdk-tests/sdk-test/Cargo.toml(1 hunks)sdk-tests/sdk-test/src/create_pda.rs(5 hunks)sdk-tests/sdk-test/src/lib.rs(1 hunks)sdk-tests/sdk-test/src/update_pda.rs(2 hunks)sdk-tests/sdk-test/tests/test.rs(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
sdk-tests/sdk-test/src/update_pda.rs (3)
sdk-tests/sdk-test/tests/test.rs (1)
create_pda(74-122)sdk-tests/sdk-pinocchio-test/tests/test.rs (1)
create_pda(85-133)js/stateless.js/src/state/types.ts (1)
ValidityProof(397-410)
sdk-tests/sdk-test/tests/test.rs (5)
sdk-tests/sdk-test/src/lib.rs (1)
update_pda(47-47)sdk-tests/sdk-test/src/update_pda.rs (1)
update_pda(16-51)sdk-tests/sdk-pinocchio-test/src/lib.rs (1)
update_pda(44-44)sdk-tests/sdk-pinocchio-test/src/update_pda.rs (1)
update_pda(17-54)sdk-tests/sdk-pinocchio-test/tests/test.rs (1)
update_pda(135-203)
sdk-tests/sdk-test/src/create_pda.rs (1)
sdk-tests/sdk-test/src/update_pda.rs (2)
default(61-68)default(78-83)
⏰ 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-v2
- GitHub Check: stateless-js-v1
- GitHub Check: lint
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test concurrent-merkle-tree
🔇 Additional comments (15)
sdk-tests/sdk-test/Cargo.toml (1)
29-29: LGTM! Light-zero-copy dependency addition is consistent with the refactoring.The addition of the
light-zero-copyworkspace dependency aligns well with the SDK test refactoring and the introduction of standardized array lengths viaARRAY_LEN.Cargo.toml (1)
37-43: LGTM! Workspace member restructuring is clear and consistent.The relocation of test crates from
program-tests/tosdk-tests/creates a more organized structure that better distinguishes SDK-specific tests from program tests. All updated paths are consistent and properly structured.sdk-tests/sdk-test/src/lib.rs (1)
10-11: LGTM! Centralized array length constant improves maintainability.The introduction of
ARRAY_LENas a public constant provides a single source of truth for array sizes across the test crate, replacing hardcoded 31-byte arrays. This improves maintainability and consistency.sdk-tests/sdk-test/tests/test.rs (3)
17-17: LGTM! ARRAY_LEN import enables dynamic array sizing.The import of
ARRAY_LENfrom the crate root allows the test to use the centralized constant for consistent array sizing.
32-32: LGTM! Consistent migration to ARRAY_LEN-based arrays.The migration from hardcoded 31-byte arrays to
ARRAY_LEN-based arrays is consistent across all test data initialization and function signatures. This standardization improves maintainability and allows for centralized control of test data sizes.Also applies to: 69-69, 78-78, 127-127
1-1: Confirm intent:#![cfg(feature = "test-sbf")]was commented outThe attribute in sdk-tests/sdk-test/tests/test.rs is commented out, which makes that test compile/run unconditionally. The repo still declares and uses the
test-sbffeature in several crates, so this change can affect test/build behavior.
- File to review: sdk-tests/sdk-test/tests/test.rs — currently:
//#![cfg(feature = "test-sbf")]- This crate declares the feature: sdk-tests/sdk-test/Cargo.toml — test-sbf = []
- Other examples still gating tests with the feature:
- sdk-tests/sdk-pinocchio-test/tests/test.rs
- sdk-tests/client-test/tests/light_program_test.rs
- program-tests/system-test/tests/test.rs
- program-tests/system-cpi-test/tests/test.rs
Please confirm whether removing the cfg guard was intentional. If not, restore the attribute; if intentional, ensure you want this test to run unconditionally and update any docs/Cargo feature usage accordingly.
sdk-tests/sdk-test/src/update_pda.rs (5)
10-10: LGTM! ARRAY_LEN import enables consistent array sizing.The import of
ARRAY_LENfrom the crate root provides access to the standardized constant for array length definitions.
53-59: LGTM! Consistent derive attribute cleanup and ARRAY_LEN usage.The removal of
Defaultfrom the derive list and the use ofARRAY_LENfor thenew_datafield are both appropriate changes that maintain consistency with the refactoring goals.
60-69: LGTM! Explicit Default implementation handles large arrays correctly.The explicit
Defaultimplementation properly initializes the[u8; ARRAY_LEN]array with zeros, which is more reliable than relying on derived implementations for large arrays (ARRAY_LEN = 2200).
71-75: LGTM! Consistent struct definition with ARRAY_LEN.The
UpdateMyCompressedAccountstruct correctly usesARRAY_LENfor the data field and removesDefaultfrom derives, maintaining consistency with the overall refactoring pattern.
77-84: LGTM! Proper explicit Default implementation.The explicit
Defaultimplementation forUpdateMyCompressedAccountcorrectly initializes both the metadata and the large array field, ensuring proper zero-initialization.sdk-tests/sdk-test/src/create_pda.rs (4)
36-58: Confirm move semantics for address_tree_info; ensure it’s Copy or this will partially move ix.You move address_tree_info out of the deserialized struct and later consume it with into_new_address_params_packed (Line 58). This pattern is fine if PackedAddressTreeInfo is Copy; otherwise, using ix after the move would be illegal.
Can you confirm that light_sdk::instruction::PackedAddressTreeInfo implements Copy? If not, borrow it instead and clone/copy only when consuming.
If needed, a safe alternative is:
- let address_tree_info = instruction_data.address_tree_info; + let address_tree_info = instruction_data.address_tree_info.clone();This requires Clone on PackedAddressTreeInfo.
77-81: ARRAY_LEN adoption and hash field annotation look good.Switching to [u8; ARRAY_LEN] and marking the field with #[hash] aligns with the new canonical length and hashing semantics.
83-89: Explicit Default impl is correct and consistent.Zero-initializing data to [0u8; ARRAY_LEN] is sensible. Keeping an explicit impl (rather than derive) is fine.
97-100: Do not remove —tree_accounts_offsetis referenced in the repoThe ripgrep results show
tree_accounts_offsetis used when buildingCreatePdaInstructionDataand returned byaccounts.to_account_metas(). The original request to remove it is incorrect unless you also update all call sites.Occurrences found:
- sdk-tests/sdk-test/src/create_pda.rs — field declaration (lines ~97–99)
- sdk-tests/sdk-test/tests/test.rs — tuple assignment and use in instruction (lines 101 and 109)
- sdk-tests/sdk-pinocchio-test/src/create_pda.rs — field declaration (lines ~84–86)
- sdk-tests/sdk-pinocchio-test/tests/test.rs — tuple assignment and use in instruction (lines 113 and 120)
- sdk-libs/sdk/src/instruction/mod.rs — example/docs reference (commented, line ~15)
Action: keep the field as-is, or if you intend to remove it, update all listed call sites to stop expecting/using
tree_accounts_offset.Likely an incorrect or invalid review comment.
ananas-block
force-pushed
the
jorrit/refactor-sdk-tests
branch
from
f8e0479 to
2fb4241
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sdk-tests/sdk-anchor-test/package.json (3)
8-18: Dev dependencies appear unused by current scripts; either wire up TS/Mocha tests or drop them.There’s no npm script invoking mocha/ts tests, yet mocha, ts-mocha, chai, TS type packages, and prettier are present. This can slow installs and drift over time.
- Option A (wire them up): add a TS test script and prefer mocha + ts-node over ts-mocha (which has had compatibility churn with newer mocha/TS versions).
- Option B (trim): remove mocha/chai/ts-mocha/types if not needed in this package.
Apply this diff for Option A (recommended if TS tests will live here):
"scripts": { - "test": "cargo test-sbf -p sdk-test" + "test": "npm run test:anchor", + "test:anchor": "cargo test-sbf -p sdk-test", + "test:ts": "mocha -r ts-node/register --extensions ts,tsx \"test/**/*.ts\"" }, "dependencies": { "@coral-xyz/anchor": "^0.29.0" }, "devDependencies": { "@lightprotocol/zk-compression-cli": "workspace:*", "chai": "^5.2.0", - "mocha": "^11.7.1", - "ts-mocha": "^11.1.0", + "mocha": "^11.7.1", + "ts-node": "^10.9.2", "@types/bn.js": "^5.2.0", "@types/chai": "^5.2.2", "@types/mocha": "^10.0.10", "typescript": "^5.9.2", "prettier": "^3.6.2" }Alternatively, for Option B, remove mocha/ts-mocha/chai/types/ts-related entries if they’re not intended to be used here.
6-7: Anchor JS version pinning: consider aligning with the on-chain Anchor version or pin exact for reproducibility.If the Rust programs/tests use a specific Anchor toolchain, consider matching the JS SDK version more tightly (e.g., exact 0.29.x) to avoid accidental minor updates during CI that might change IDL/serialization nuances.
17-17: Optional: add convenience scripts for formatting.Prettier is present without a script. Adding a format/lint script helps consistency.
Example additions (outside current selected lines):
- "format": "prettier --write ."
- "format:check": "prettier --check ."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)sdk-tests/sdk-anchor-test/package.json(1 hunks)sdk-tests/sdk-test/Cargo.toml(1 hunks)sdk-tests/sdk-test/src/create_pda.rs(4 hunks)sdk-tests/sdk-test/src/lib.rs(1 hunks)sdk-tests/sdk-test/src/update_pda.rs(2 hunks)sdk-tests/sdk-test/tests/test.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- sdk-tests/sdk-test/src/lib.rs
- Cargo.toml
- sdk-tests/sdk-test/Cargo.toml
- sdk-tests/sdk-test/tests/test.rs
- sdk-tests/sdk-test/src/update_pda.rs
- sdk-tests/sdk-test/src/create_pda.rs
⏰ 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: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: Test sdk-libs
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: lint
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: cli-v2
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
🔇 Additional comments (3)
sdk-tests/sdk-anchor-test/package.json (3)
2-4: Confirm Solana SBF tooling is available in CI forcargo test-sbf.
cargo test-sbfis provided via Solana’s cargo plugins and won’t exist in a stock Rust toolchain. Ensure CI images install Solana and the cargo plugins so this script doesn’t fail.Would you like me to add a short CI step snippet (e.g., GitHub Actions) to install the Solana toolchain and cache it?
9-9: Workspace devDependency requires this package to be part of the repo workspace.Double-check that this folder is included in the root workspace config so "workspace:*" resolves properly. Adding the "name" (and optionally "version") as suggested will also help the workspace tool pick it up.
If helpful, I can generate a quick script to scan the root workspace config and confirm this path is included.
10-16: Validate mocha, ts-mocha, and TypeScript compatibility.Mocha 11 and TS 5.9 are recent; historically, ts-mocha has lagged behind Mocha/TS changes. If you stick with ts-mocha, verify it runs under your Node/TS versions. Otherwise, the mocha + ts-node route above is more future-proof.
I can provide a minimal sample TS test and tsconfig.json to validate end-to-end locally/CI.
| { | ||
| "scripts": { | ||
| "test": "cargo test-sbf -p sdk-test" | ||
| }, | ||
| "dependencies": { | ||
| "@coral-xyz/anchor": "^0.29.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@lightprotocol/zk-compression-cli": "workspace:*", | ||
| "chai": "^5.2.0", | ||
| "mocha": "^11.7.1", | ||
| "ts-mocha": "^11.1.0", | ||
| "@types/bn.js": "^5.2.0", | ||
| "@types/chai": "^5.2.2", | ||
| "@types/mocha": "^10.0.10", | ||
| "typescript": "^5.9.2", | ||
| "prettier": "^3.6.2" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add required package metadata for workspace compatibility (name/private/version).
This package references a workspace dependency ("workspace:") but lacks "name", "private", and "version". Most workspace managers (npm/yarn/pnpm) require at least "name" (and often "version" + "private": true) for the package to be recognized as a workspace member. Without these, resolution of "workspace:" can fail or behave unexpectedly.
Apply this diff to add minimal, safe metadata:
{
+ "name": "@lightprotocol/sdk-anchor-test",
+ "private": true,
+ "version": "0.0.0",
"scripts": {
"test": "cargo test-sbf -p sdk-test"
},
"dependencies": {
"@coral-xyz/anchor": "^0.29.0"
},
"devDependencies": {
"@lightprotocol/zk-compression-cli": "workspace:*",
"chai": "^5.2.0",
"mocha": "^11.7.1",
"ts-mocha": "^11.1.0",
"@types/bn.js": "^5.2.0",
"@types/chai": "^5.2.2",
"@types/mocha": "^10.0.10",
"typescript": "^5.9.2",
"prettier": "^3.6.2"
}
}📝 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.
| { | |
| "scripts": { | |
| "test": "cargo test-sbf -p sdk-test" | |
| }, | |
| "dependencies": { | |
| "@coral-xyz/anchor": "^0.29.0" | |
| }, | |
| "devDependencies": { | |
| "@lightprotocol/zk-compression-cli": "workspace:*", | |
| "chai": "^5.2.0", | |
| "mocha": "^11.7.1", | |
| "ts-mocha": "^11.1.0", | |
| "@types/bn.js": "^5.2.0", | |
| "@types/chai": "^5.2.2", | |
| "@types/mocha": "^10.0.10", | |
| "typescript": "^5.9.2", | |
| "prettier": "^3.6.2" | |
| } | |
| } | |
| { | |
| "name": "@lightprotocol/sdk-anchor-test", | |
| "private": true, | |
| "version": "0.0.0", | |
| "scripts": { | |
| "test": "cargo test-sbf -p sdk-test" | |
| }, | |
| "dependencies": { | |
| "@coral-xyz/anchor": "^0.29.0" | |
| }, | |
| "devDependencies": { | |
| "@lightprotocol/zk-compression-cli": "workspace:*", | |
| "chai": "^5.2.0", | |
| "mocha": "^11.7.1", | |
| "ts-mocha": "^11.1.0", | |
| "@types/bn.js": "^5.2.0", | |
| "@types/chai": "^5.2.2", | |
| "@types/mocha": "^10.0.10", | |
| "typescript": "^5.9.2", | |
| "prettier": "^3.6.2" | |
| } | |
| } |
🤖 Prompt for AI Agents
In sdk-tests/sdk-anchor-test/package.json around lines 1 to 19, the package.json
is missing required workspace metadata (name, version, private) which prevents
resolving "workspace:*"; add a valid "name" (unique, npm-safe), a "version"
(e.g., "1.0.0"), and "private": true at the top level of the JSON so the package
is recognized as a workspace member while keeping the existing
scripts/dependencies unchanged.
Summary by CodeRabbit
New Features
Tests
Chores