chore: ensure all sensible combinations of features work#217
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis pull request refactors the SDK's feature flag organization in Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/sdk/Cargo.toml`:
- Line 31: Replace the current hard dependency entries that force-enable Anchor
when debug is set—specifically the anchor-debug feature and the symmetric
anchor-compat-debug feature—so they use Cargo's weak-dep "?/" syntax; update the
feature definitions referencing "anchor-lang-current/anchor-debug" and
"anchor-lang-compat/anchor-compat-debug" to use
"?/anchor-lang-current/anchor-debug" and
"?/anchor-lang-compat/anchor-compat-debug" respectively, ensuring the debug
feature is a no-op unless the parent anchor or anchor-compat feature is also
enabled.
In `@rust/sdk/src/access_control/structs/member.rs`:
- Around line 10-12: Remove the stale commented cfg attribute present above the
unconditional import of borsh; specifically delete the dead comment
"//#[cfg(feature = "anchor-support")]" that precedes "use crate::compat::borsh;"
in member.rs and make the same cleanup in permission.rs and undelegate_args.rs
so the intent (unconditional import with #[allow(unused_imports)]) is clear; no
other changes needed to the use statements or attributes.
In `@rust/sdk/src/compat/as_modern.rs`:
- Around line 24-31: The runtime assert! checks comparing layout of
backward_compat::AccountInfo<'static> and
solana_program::account_info::AccountInfo<'static> should be replaced with
compile-time assertions to avoid panics and runtime overhead before the
subsequent unsafe transmutation of those types; restore the previous const {
assert!(...) } pattern or use a compile-time helper such as
static_assertions::const_assert_eq! for size and align checks on
backward_compat::AccountInfo and solana_program::account_info::AccountInfo, and
if a true compile-time check is impossible due to feature-flag constraints,
replace assert! with debug_assert_eq! and add a comment documenting the
limitation and why debug-only checks are acceptable for the unsafe transmute.
In `@rust/sdk/test-combinations.sh`:
- Around line 1-4: The script can run from any CWD so add a directory-change at
the top of rust/sdk/test-combinations.sh to anchor to the SDK folder; after the
shebang (or immediately after set -euo pipefail) call cd "$(dirname "$0")" ||
exit 1 (or equivalent) so subsequent cargo build invocations run against the
rust/sdk Cargo.toml; modify test-combinations.sh to include that single-line cd
guard.
🪄 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: 108c6b94-73d2-45ff-a2b9-521715436f9b
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust/sdk/Cargo.tomlrust/sdk/src/access_control/structs/member.rsrust/sdk/src/access_control/structs/permission.rsrust/sdk/src/access_control/structs/undelegate_args.rsrust/sdk/src/compat/as_modern.rsrust/sdk/test-combinations.sh
| # anchor 1.0 support (CANNOT be combined with backward-compat) | ||
| anchor = ["anchor-modern"] | ||
| anchor-modern = ["anchor-support", "anchor-lang-current"] | ||
| anchor-debug = ["anchor-lang-current/anchor-debug"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use ?/ so debug toggles don't activate the optional anchor deps on their own.
anchor-debug = ["anchor-lang-current/anchor-debug"] (and the symmetric anchor-compat-debug on line 39) currently force-enable the optional anchor-lang-current / anchor-lang-compat dependency whenever the debug feature is set, even if the user never enabled anchor / anchor-compat. That yields a half-configured build: the Anchor crate is compiled in, but anchor-support is still off, so the structs in access_control/structs/* keep deriving Borsh instead of Anchor traits. Using the weak-dep ?/ syntax keeps the debug flag a no-op unless the parent feature is also active.
♻️ Proposed change
-anchor-debug = ["anchor-lang-current/anchor-debug"]
+anchor-debug = ["anchor-lang-current?/anchor-debug"]
@@
-anchor-compat-debug = ["anchor-lang-compat/anchor-debug"]
+anchor-compat-debug = ["anchor-lang-compat?/anchor-debug"]Worth extending test-combinations.sh with a standalone cargo build --features anchor-debug (and anchor-compat-debug) once this is in place to lock in the invariant.
Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/sdk/Cargo.toml` at line 31, Replace the current hard dependency entries
that force-enable Anchor when debug is set—specifically the anchor-debug feature
and the symmetric anchor-compat-debug feature—so they use Cargo's weak-dep "?/"
syntax; update the feature definitions referencing
"anchor-lang-current/anchor-debug" and "anchor-lang-compat/anchor-compat-debug"
to use "?/anchor-lang-current/anchor-debug" and
"?/anchor-lang-compat/anchor-compat-debug" respectively, ensuring the debug
feature is a no-op unless the parent anchor or anchor-compat feature is also
enabled.
| //#[cfg(feature = "anchor-support")] | ||
| #[allow(unused_imports)] | ||
| use crate::compat::borsh; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Drop the stale //#[cfg(feature = "anchor-support")] comment.
The import is now intentionally unconditional with #[allow(unused_imports)], so the commented-out cfg line is dead documentation that obscures the intent. The same leftover appears in permission.rs (line 13) and undelegate_args.rs (line 8) — please clean up all three.
♻️ Proposed change
-//#[cfg(feature = "anchor-support")]
#[allow(unused_imports)]
use crate::compat::borsh;📝 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.
| //#[cfg(feature = "anchor-support")] | |
| #[allow(unused_imports)] | |
| use crate::compat::borsh; | |
| #[allow(unused_imports)] | |
| use crate::compat::borsh; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/sdk/src/access_control/structs/member.rs` around lines 10 - 12, Remove
the stale commented cfg attribute present above the unconditional import of
borsh; specifically delete the dead comment "//#[cfg(feature =
"anchor-support")]" that precedes "use crate::compat::borsh;" in member.rs and
make the same cleanup in permission.rs and undelegate_args.rs so the intent
(unconditional import with #[allow(unused_imports)]) is clear; no other changes
needed to the use statements or attributes.
| assert!( | ||
| core::mem::size_of::<backward_compat::AccountInfo<'static>>() | ||
| == core::mem::size_of::<solana_program::account_info::AccountInfo<'static>>() | ||
| ); | ||
| assert!( | ||
| core::mem::align_of::<backward_compat::AccountInfo<'static>>() | ||
| == core::mem::align_of::<solana_program::account_info::AccountInfo<'static>>() | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for comments or docs explaining the layout check approach
rg -i -C5 'layout|const.*assert|runtime.*check' rust/sdk/src/compat/Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 575
🏁 Script executed:
cat -n rust/sdk/src/compat/as_modern.rs | head -50Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 1784
🏁 Script executed:
# Check git history for this file to see if const blocks existed
git log --oneline -p rust/sdk/src/compat/as_modern.rs 2>/dev/null | head -100Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 1764
🏁 Script executed:
# Search for any documentation or PR context about layout compatibility
rg -i 'const.*block|transmut|layout.*compat' rust/sdk/src/compat/ -A3 -B3Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 410
🏁 Script executed:
# Search for previous layout checks or const implementations in related files
git log --all --full-history --source -- rust/sdk/src/compat/ | head -50Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 1481
🏁 Script executed:
# Check if backward_compat module itself has layout checks
rg 'const.*fn|size_of|align_of' rust/sdk/src/compat/backward_compat.rs -B2 -A2Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 160
🏁 Script executed:
# Look for any const blocks in the compat directory
fd . rust/sdk/src/compat/ -name '*.rs' -exec rg 'const\s*\{' {} +Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 312
🏁 Script executed:
# Check the entire git log for this specific function to see if it existed before
git log --all -S 'AsModern for backward_compat::AccountInfo' -- rust/sdk/Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 1609
🏁 Script executed:
# Get the specific changes to AccountInfo::as_modern between the two commits
git show f6339f8:rust/sdk/src/compat/as_modern.rs 2>/dev/null || echo "File may not have existed at f6339f8"Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 1521
🏁 Script executed:
# Search more broadly in older commits for AccountInfo and layout checks
git log --all -p -S 'backward_compat::AccountInfo' -- 'rust/sdk/src/compat/' | head -150Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 5549
🏁 Script executed:
# Check what files existed in the compat directory at the previous commit
git ls-tree -r f6339f8 -- rust/sdk/src/compat/Repository: magicblock-labs/ephemeral-rollups-sdk
Length of output: 416
Remove runtime layout checks or provide compile-time assertions.
The layout compatibility checks were moved from compile-time (const { assert!(...) }) to runtime (assert!). This introduces a safety concern:
- Compile-time checks (previous version) guaranteed layout compatibility at build time with zero runtime cost and prevented miscompilation entirely.
- Runtime checks (current version) execute on every function call and can panic in production.
The unsafe transmutation at line 32 depends on these checks. Per coding guidelines for this file path (rust/sdk/**), panic-prone patterns require proper error handling or explicit justification.
Please clarify why the const block was removed. If feature flag constraints made it incompatible, consider whether debug_assert! (checked in debug builds but optimized away in release) would be acceptable, or if the checks can be restored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/sdk/src/compat/as_modern.rs` around lines 24 - 31, The runtime assert!
checks comparing layout of backward_compat::AccountInfo<'static> and
solana_program::account_info::AccountInfo<'static> should be replaced with
compile-time assertions to avoid panics and runtime overhead before the
subsequent unsafe transmutation of those types; restore the previous const {
assert!(...) } pattern or use a compile-time helper such as
static_assertions::const_assert_eq! for size and align checks on
backward_compat::AccountInfo and solana_program::account_info::AccountInfo, and
if a true compile-time check is impossible due to feature-flag constraints,
replace assert! with debug_assert_eq! and add a comment documenting the
limitation and why debug-only checks are acceptable for the unsafe transmute.
There was a problem hiding this comment.
const {} isn't supported by anchor-syn which depends on an older sync = 1.0 which does not understand const-block.
- Turned out that `anchor-syn 1.0` depends on `syn 1.0`, which is quite old and does not understand newer Rust syntax such as `const {}` blocks. As a result, I had to remove them from `as_modern.rs` (which unfortunately turns those assertions from compile-time asserts into runtime asserts 😞 ).
- Interestingly, there is [an issue created on anchor repo](otter-sec/anchor#4521) just 2 days back and [a PR that upgrades to `syn 2.0`](otter-sec/anchor#4523).
- Made changes such that **30 out of 32 sensible feature combinations** now build successfully. See `sdk/scripts/test-combinations.sh`, which is now also executed by CI.
- The remaining 2 combinations fail when `anchor-debug` feature is enabled. This appears to be an `anchor-lang 0.32.0` issue when that feature is enabled. Since it is not a particularly important feature (and any user of anchor 0.32 will face this problem anyway), this is acceptable for now.
https://github.com/user-attachments/assets/b51e8ab9-a81a-4a29-910c-a12a68b90b1c
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Refactor**
* Reorganized internal feature flag definitions for better modularity.
* Streamlined serialization configuration for core data structures.
* Consolidated PDA helper imports to improve code organization.
* **Chores**
* Added automated testing across multiple feature combinations to ensure compatibility.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/magicblock-labs/ephemeral-rollups-sdk/pull/217)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/magicblock-labs/ephemeral-rollups-sdk/pull/217){kind=link}
3 participants
anchor-syn 1.0depends onsyn 1.0, which is quite old and does not understand newer Rust syntax such asconst {}blocks. As a result, I had to remove them fromas_modern.rs(which unfortunately turns those assertions from compile-time asserts into runtime asserts 😞 ).syn 2.0.sdk/scripts/test-combinations.sh, which is now also executed by CI.anchor-debugfeature is enabled. This appears to be ananchor-lang 0.32.0issue when that feature is enabled. Since it is not a particularly important feature (and any user of anchor 0.32 will face this problem anyway), this is acceptable for now.Screen.Recording.2026-05-14.at.11.21.45.AM.mov
Summary by CodeRabbit
Refactor
Chores