Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Nov 1, 2025

Summary by CodeRabbit

  • Chores

    • Switched CI runners to Ubuntu 22.04 (ARM) across workflows to align build infrastructure.
  • Bug Fixes

    • Improved cross-platform compatibility and stability for native integrations and wallet import flows by normalizing native string handling.
  • Tests

    • Updated and cleaned up FFI-related tests to ensure consistent behavior and reliability on the ARM runner.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 1, 2025

Walkthrough

Updated GitHub Actions workflows to run on ubuntu-22.04-arm instead of ubuntu-latest. Adjusted several FFI test and implementation files to use std::os::raw::c_char for C string pointer types, updating casts, helpers, buffer literals, and one exposed function signature (wallet_manager_add_wallet_from_mnemonic) to accept *const c_char.

Changes

Cohort / File(s) Summary
GitHub Actions runner updates
.github/workflows/fuzz.yml, .github/workflows/rust.yml, .github/workflows/verify-ffi-docs.yml
Replaced runs-on: ubuntu-latest with runs-on: ubuntu-22.04-arm for all jobs across all three workflows. No changes to steps or logic.
FFI pointer type adjustments — tests & helpers
key-wallet-ffi/src/account_derivation_tests.rs, key-wallet-ffi/src/derivation_tests.rs, key-wallet-ffi/src/transaction_tests.rs, key-wallet-ffi/tests/test_import_wallet.rs
Replaced raw i8 pointer uses with std::os::raw::c_char (*const c_char, *mut c_char), updated CString/CStr casts, adjusted small buffer literals (e.g., [0i8;...][0u8;...]), and added relevant imports.
FFI pointer casting refactor
key-wallet-ffi/src/derivation.rs
Refactored pointer casting calls within ptr::copy_nonoverlapping from inline casts (e.g., path_out as *mut u8) to .cast::<u8>() method across derivation path functions. No logic changes.
Public API signature update
key-wallet-ffikey_wallet_ffi::wallet_manager::wallet_manager_add_wallet_from_mnemonic
Updated exposed function signature to accept *const c_char for mnemonic and passphrase parameters (previously *const i8).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Heterogeneous changes: CI workflow edits (simple, repetitive) plus FFI pointer-type refactors and public API signature update (require careful reasoning).
  • Areas requiring extra attention:
    • wallet_manager_add_wallet_from_mnemonic public API signature change—verify all call sites across the codebase and dependent crates match the new signature.
    • Correctness of all CString/CStr casts in tests: validate ownership, lifetime, and proper use of from_raw and from_ptr to avoid undefined behavior.
    • Buffer literal type downgrades (i8u8) and any implicit assumptions about signed vs. unsigned byte semantics in surrounding logic.
    • CI runner change: confirm ubuntu-22.04-arm does not introduce environment-specific workflow step failures or missing tools.

Poem

🐰 I nibble on C strings bright,
Swapped my i8 for c_char tonight,
Runners hop to twenty-two ARM,
Tests and casts all tight and warm,
A tiny hop—a tidy bite! ✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main objective of updating GitHub Actions runners to ubuntu-22.04-arm, which aligns with the majority of changes in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ci/run-on-arm

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69741e7 and 3ab3beb.

📒 Files selected for processing (1)
  • key-wallet-ffi/src/derivation.rs (6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)

**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable

Files:

  • key-wallet-ffi/src/derivation.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}

📄 CodeRabbit inference engine (CLAUDE.md)

Be careful with memory management at FFI boundaries (C/Swift interop)

Files:

  • key-wallet-ffi/src/derivation.rs
**/src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]

Files:

  • key-wallet-ffi/src/derivation.rs
*-ffi/**

📄 CodeRabbit inference engine (AGENTS.md)

FFI bindings live in *-ffi crates

Files:

  • key-wallet-ffi/src/derivation.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Include integration tests that cover full wallet workflows
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/derivation_tests.rs : Place path derivation tests in tests/derivation_tests.rs

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/ffi/**/*.rs : Expose safe C FFI with #[no_mangle] extern "C" functions only for vetted APIs

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/**/*.rs : Prefer using public keys or key fingerprints for identification in logs and messages instead of private keys

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift} : Be careful with memory management at FFI boundaries (C/Swift interop)

Applied to files:

  • key-wallet-ffi/src/derivation.rs
📚 Learning: 2025-06-26T15:48:36.342Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:48:36.342Z
Learning: In Rust FFI code, unwrap() must not be used on CString::new, especially for error messages or untrusted input, as it can panic if the string contains null bytes. Instead, use unwrap_or_else with a fallback to a known-safe, hardcoded string to prevent panics across FFI boundaries.

Applied to files:

  • key-wallet-ffi/src/derivation.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). (16)
  • GitHub Check: Core Components Tests
  • GitHub Check: Lint GitHub Actions
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: SPV Components Tests
  • GitHub Check: Strict Warnings and Clippy Checks
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_deserialize_address)
🔇 Additional comments (1)
key-wallet-ffi/src/derivation.rs (1)

134-134: LGTM! Idiomatic pointer cast refactor.

Using .cast::<u8>() instead of as *mut u8 is the modern, recommended approach for pointer type conversions in Rust. This makes the code more explicit and aligns with Rust best practices while maintaining identical behavior.

Also applies to: 192-192, 247-247, 302-302, 359-359, 420-420


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Replace hardcoded i8 casts with c_char to fix ARM compilation errors.
On ARM architectures, c_char is u8 (not i8 as on x86/x86_64), causing
type mismatches in FFI test code.

- Replace all *mut i8 and *const i8 casts with c_char equivalents
- Update array declarations from [0i8; N] to [0u8; N] with proper casting
- Add c_char import to all affected test files
Replace 'as *mut u8' casts with .cast::<u8>() method to fix clippy
warnings about unnecessary casts on ARM architectures.

On ARM, c_char is u8, making the explicit cast unnecessary. Using
.cast::<u8>() is the modern Rust approach and avoids clippy warnings
while maintaining platform compatibility.
@QuantumExplorer QuantumExplorer merged commit 35e47fd into v0.41-dev Nov 5, 2025
32 checks passed
@QuantumExplorer QuantumExplorer deleted the ci/run-on-arm branch November 5, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants