Feat/kos hardware #116
Feat/kos hardware #116
Conversation
WalkthroughThis pull request updates several configuration and build files to reflect a shift in project focus. The Rust analyzer settings now link to different Cargo projects, and the workspace configuration is expanded with a new hardware package. New build targets and Makefiles are added for the hardware package. Additionally, build scripts for Android have been enhanced for OS compatibility, and relevant dependency and feature management modifications have been applied across various Cargo.toml files. New FFI functions and data structures have been introduced to support blockchain operations in the new hardware library. Changes
Suggested labels
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/kos-mobile/build_source.sh (1)
73-94: Ensure secure retrieval of Android NDK.
Downloading and installing the NDK for different platforms looks correct. You clean up downloaded artifacts and handle extraction properly. Just confirm the checksums to ensure the downloaded artifacts were not tampered with.If needed, you could incorporate a checksum verification step:
+log_status "verifying ndk checksum..." +# Example pseudo-code: +# echo "<expected_hash> ndk.zip" | sha256sum --check -packages/kos-hardware/src/lib.rs (3)
93-137: Robust transaction signing with fallback.
rs_sign_txlogs an error withDebugErrorHandleron failures. The code does correct checks for chain presence and uses thechains::Transactiontype. Ensure concurrency or reentrancy is not an issue if multiple threads sign transactions simultaneously.
173-205: Message signing flow matches transaction signing.
This maintains overall consistency. Consider verifying the length of the message before proceeding to ensure it will fit insig.
244-274: Extract transaction info.
rs_get_tx_inforeturns a structuredCTxInfo. Good approach, though consider verifyingraw_txlength before decoding.packages/kos/Cargo.toml (1)
38-50: Optional dependencies for flexible builds.
These newly marked optional dependencies are a good practice for flexible feature gating. Confirm they’re tested in minimal configurations (with them disabled) to avoid compile-time regressions.packages/kos-hardware/src/models.rs (2)
79-94: Reuse common read/write helpers to reduce duplication.
CBufferimplements a pattern nearly identical toCNodeStruct, reading from and writing to unmanaged memory. Consider abstracting repeated logic (pointer checks, length checks, and copying) into a shared helper or module to follow the DRY principle.
96-134: Extend error handling forCTransaction.
Similar toCNodeStructandCBuffer,CTransactionlacks proactive error reporting when pointer writes or reads fail. If this is intentional for a low-level FFI layer, at least consider an internal logging mechanism so failures can be diagnosed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.vscode/settings.json(1 hunks)Cargo.toml(1 hunks)Makefile(1 hunks)packages/kos-hardware/.gitignore(1 hunks)packages/kos-hardware/Cargo.toml(1 hunks)packages/kos-hardware/Makefile(1 hunks)packages/kos-hardware/src/lib.rs(1 hunks)packages/kos-hardware/src/models.rs(1 hunks)packages/kos-mobile/Cargo.toml(1 hunks)packages/kos-mobile/build_android.sh(1 hunks)packages/kos-mobile/build_source.sh(3 hunks)packages/kos/Cargo.toml(2 hunks)packages/kos/src/chains/substrate/mod.rs(1 hunks)packages/kos/src/chains/util.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/kos-mobile/build_android.sh (1)
packages/kos-mobile/build_source.sh (1) (1)
log_status(62-64)
packages/kos-hardware/src/models.rs (1)
packages/kos/src/chains/substrate/mod.rs (1) (1)
new(30-37)
🪛 Shellcheck (0.10.0)
packages/kos-mobile/build_source.sh
[warning] 16-16: OS_TOOLCHAIN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 18-18: LIB_EXTENSION appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 19-19: JNI_PLATFORM appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (26)
Makefile (1)
34-36: New Build Target "build-ksafe" is Configured Correctly
The added target builds thekos-hardwarepackage for the ARM Cortex-M architecture using the specified hardware profile. Please verify that the corresponding hardware profile settings are properly defined in your Cargo.toml so that the build behaves as expected.packages/kos/src/chains/util.rs (1)
2-2: Appropriate Import for Vec in no_std Environment
ImportingVecfromallocensures dynamic array functionality is available in ano_stdcontext, which is likely intended for this project. Just confirm that this aligns with your overall design as some projects may rely on the standard library, but here it seems intentional.packages/kos-hardware/.gitignore (1)
1-4: .gitignore File is Clean and Focused
The new.gitignoreentries properly exclude build artifacts (/target), dependency lock files (Cargo.lock), and IDE configuration directories (/.idea) from version control. This helps keep the repository clean and avoid unnecessary file tracking.packages/kos/src/chains/substrate/mod.rs (1)
3-3: Added Import foralloc::formatis Well-Justified
By importingalloc::format, you ensure that formatted strings can be created in a no_std context. The use offormat!in generating paths (e.g., inget_path) confirms this change is beneficial and consistent with the module's design.packages/kos-hardware/Makefile (1)
1-9: kos-hardware Makefile Targets are Clear and Concise
The new Makefile defines the standard targets (all,clean, andclippy) using appropriate cargo commands for building, cleaning, and linting. These targets are useful for maintaining the package independently. Consider documenting or aligning naming conventions if you add any additional hardware-specific targets in the future.packages/kos-mobile/Cargo.toml (1)
21-23: Enable Default Features for thekosDependencyAdding
default-features = trueto thekosdependency ensures that the default implementation details are included when building the mobile package. This change aligns with the new project focus. Please verify that enabling default features does not unintentionally introduce conflicts with other dependency configurations across the workspace..vscode/settings.json (1)
1-8: Refine Rust Analyzer Project LinksThe modifications in the
"rust-analyzer.linkedProjects"list remove outdated project paths and add the relevant ones (kos,kos-web,kos-hardware, andkos-mobile). This update clearly reflects the shift in focus and helps ensure that the Rust analyzer picks up the correct configurations across the workspace.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
packages/kos-mobile/build_android.sh (1)
42-50: Enhance Portability in the Android Build ScriptIn the
assemble_android_lib_unit_test()function, assigningjni="$JNI_PLATFORM"(line 44) and using the dynamic$LIB_EXTENSION(line 49) instead of hardcoded values improves platform adaptability. Ensure that the environment properly defines bothJNI_PLATFORMandLIB_EXTENSIONto avoid potential build issues on different systems.packages/kos-hardware/Cargo.toml (1)
1-17: Define Newkos-hardwarePackage ManifestThis new manifest correctly declares the
kos-hardwarepackage along with its metadata, dependencies, and features. Specifying the dependency onkoswithdefault-features = falseand theksafefeature—as well as includingtiny-json-rs—appears on target. Confirm that the chosen crate type"staticlib"meets integration requirements.Cargo.toml (2)
4-6: Update Workspace MembersThe inclusion of
"packages/kos-hardware"in the workspace members array reflects the expansion of the project to support hardware targets. This addition is consistent with the broader PR objectives.
11-19: Introduce a Dedicated Hardware Build ProfileThe new
[profile.hardware]section sets optimization parameters such asopt-level = 'z',lto = true, andpanic = 'abort'that are well-suited for building for resource-constrained hardware targets. Please ensure that these optimizations are validated against your target hardware’s performance and binary size requirements.packages/kos-mobile/build_source.sh (1)
131-132: Confirm custom configuration flags for OpenSSL.
You removed the-D__ANDROID_API__="$ANDROID_MIN_API"flag when$archmatches$OS_ARCH. Ensure it doesn’t affect downstream builds. If no issues arise, it’s a good simplification.packages/kos-hardware/src/lib.rs (10)
26-29: Straightforward addition function.
rust_add(a, b)is self-explanatory, with no immediate issues.
31-51: Check error handling for missing chain.
rs_derivereturnsfalseif the chain is not found, which is consistent. Good usage ofunsafeblock for direct pointer operations. Validate thatString::from_utf8_uncheckedis truly safe for your path data.
53-69: Graceful fallback for chain mismatch.
rs_get_pbkcleanly handles the chain existence check. Usingmatchfor error handling is consistent with the rest of the file.
71-91: Return address to provided buffer.
rs_get_addrwrites toresultwithresult.write(...). Confirm that the buffer capacity inCBufferis sufficient for the largest possible addresses.
139-171: Raw transaction signing.
Your approach is consistent withrs_sign_tx. Confirm thatnode.read_pvk()is valid in scenarios where the node is not fully initialized.
207-222: Display chain name.
rs_get_chain_nameuses the same pattern for error handling. No issues.
224-242: Symbol retrieval.
rs_get_chain_symbolparallelsrs_get_chain_name. Looks good, but watch for concurrency if chain details can change at runtime.
276-326: Mnemonic to seed conversion.
Great approach to handle different error states. The usage ofString::from_utf8ensures we catch invalid UTF-8 sequences. The function returnsfalseon errors, which must be documented so callers handle it correctly.
328-335: Transaction info serialization.
rs_tx_info_to_jsonusestiny_json_rs. Straightforward. Double-check memory usage inresult.write.
337-342: Version retrieval.
Simple constant and function to expose version info. This is a neat pattern for C FFI.packages/kos/Cargo.toml (2)
31-31: Confirm pbkdf2 features.
You added"password-hash"which can increase crate size. Ensure it’s needed. Otherwise, no functional issues spotted.
61-77: Use ofnot-ksafedefault feature.
Aggregating random, bip32, bip39, and other dependencies undernot-ksafeis a clever solution to separate out optional crypto functionalities. Ensure that production environments or embedded targets don’t inadvertently enable this.packages/kos-hardware/src/models.rs (2)
41-77: Provide explicit unsafe or safety notes for pointer operations.
WithinCNodeStruct, the read/write methods rely onunsafeblocks inwrite_to_memoryandread_to_vec. Although these are valid in an FFI context, documenting safety contracts or adding run-time checks (e.g., validating that pointers and lengths are correct) can help avoid undefined behavior when this code is used by external consumers.Do you want me to provide a quick script to grep for all callers of
CNodeStructmethods to ensure correct pointer usage throughout the codebase?
191-219: Validate custom chain string usage.
chaincode_stris converted to UTF-8, defaulting to an empty string upon failure. Be sure that upstream code can handle potential empty strings for custom chain logic. If invalid strings require special handling, consider returning an error rather than silently substituting.Would you like me to generate a script that searches all uses of
RequestChainParamsto confirm whether empty chaincode strings may cause issues?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kos-mobile/build_source.sh (2)
5-23: OS Detection Block EnhancementsThe conditional check for macOS and Linux is well implemented, and the appropriate OS-specific variables are exported. However, note that on line 16 there is an extra space before
OS_TOOLCHAIN(i.e.export OS_TOOLCHAIN="linux-x86_64") which should be standardized for greater consistency and readability.
122-131: OpenSSL Library Assembly AdjustmentWithin the
assemble_openssl_libfunction:
- The command
cd "$OPENSSL_PATH" || exitworks to change directories; however, consider logging an appropriate error message before exiting to aid in debugging.- In the else branch of the configuration check, the call to
./Configure "$arch" no-asm no-sharedno longer includes the previously used-D__ANDROID_API__="$ANDROID_MIN_API"flag. Please verify that this simplification works correctly for all target architectures and that it does not introduce unintended side effects during the OpenSSL build process.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/kos-mobile/build_source.sh(4 hunks)
🔇 Additional comments (4)
packages/kos-mobile/build_source.sh (4)
27-29: Export of Android JNI Path VariablesExporting
ANDROID_JNI_LIBS_PATHandANDROID_GENERATED_BINDS_PATHensures that these paths are available to downstream scripts (e.g., for linking in your Android build process). The use of quotes in the assignment helps avoid issues with spaces in file paths.
30-32: Export of Android Build ArraysThe additions of the arrays (
ANDROID_ARCHS,ANDROID_TOOLCHAINS, andANDROID_JNI) accommodate multiple target architectures. Note: Exporting arrays in Bash can be tricky because they may not propagate to child processes as expected. Please verify that these arrays are correctly interpreted in any downstream scripts, or consider alternative approaches if necessary.
36-37: Export of iOS Architectures and Package NameThe export of
IOS_ARCHSandPACKAGE_NAMEis clear and consistent, enabling cross-script use. These variables appear to integrate well with the rest of the build configuration.
40-41: Export of Color VariablesExporting the color variables
ANDROIDandIOSis useful if they are shared with sub-scripts for consistent logging or UI color cues. Consider whether these need to be exported if they are only used locally in this script; if they will be referenced elsewhere, exporting is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/kos-mobile/build_source.sh (1)
44-45: 🛠️ Refactor suggestionImplement
log_warningFunction
The script useslog_warning(e.g. in the macOS NDK configuration branch) but does not define it. Consider adding a function along with an associated color variable (for example, YELLOW) to handle warning messages. For instance:+YELLOW='\033[1;33m' +log_warning() { + echo -e "${YELLOW}$1${NC}" +}Place this snippet alongside the other logging functions to ensure consistent log handling.
🧹 Nitpick comments (5)
packages/kos-mobile/build_source.sh (5)
6-23: OS and Environment Detection Consistency
The conditional checks using[[ "$OSTYPE" == "darwin"* ]]and[[ "$OSTYPE" == "linux"* ]]are well structured and appropriately export key environment variables. For consistency, note that in the Linux branch (line 16) there is an extra space beforeOS_TOOLCHAIN. Consider removing the extra space (i.e. useexport OS_TOOLCHAIN="linux-x86_64"instead ofexport OS_TOOLCHAIN="linux-x86_64").
118-135: Configure OpenSSL – Enhanced Error Handling Suggestion
Theconfigure_opensslfunction downloads and extracts the OpenSSL tarball and sets up directories. To further improve robustness, consider adding error checking after thecurlcommand (line 123) and the tar extraction (line 124) so that any issues during download or extraction are caught and reported.
137-151: Assemble OpenSSL Libraries – Directory Change and Duplication
Withinassemble_openssl_lib, the commandcd "$OPENSSL_PATH" || exit(line 141) may be too abrupt if the change directory fails; logging an error message before exiting would provide better context. Additionally, both conditional branches execute./Configure "$arch" no-asm no-shared. Refactoring to call the configuration command only once (after conditionally setting the environment variables) could reduce duplicate code.
152-165: Assembly Process and Post-Build Cleanup
The subsequent steps—for cleaning, building, and copying libraries—are clear and systematic. Resetting the environment variables (lines 162–165) is good practice. Consider adding error handling after themakecommands in case the build fails, which would help diagnose issues earlier in the build process.
1-166: Overall Script Quality and Robustness
This build script effectively distinguishes between macOS and Linux environments and sets up a multi-platform build process by exporting relevant variables and handling NDK and OpenSSL configuration. Enhancing error reporting (e.g., for directory changes and downloads) and defining the missinglog_warningfunction will improve robustness and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/kos-mobile/build_source.sh(4 hunks)
🔇 Additional comments (5)
packages/kos-mobile/build_source.sh (5)
27-32: Android Build Variables Setup
The exports forANDROID_JNI_LIBS_PATH,ANDROID_GENERATED_BINDS_PATH, and the arrays (ANDROID_ARCHS,ANDROID_TOOLCHAINS, andANDROID_JNI) are correctly defined. Ensure that these variables are being utilized in downstream scripts for cross-script consistency.
36-37: iOS Configuration Exports
The export ofIOS_ARCHSandPACKAGE_NAMEis clear and appropriate for later use.
40-44: Color Variables and Logging Consistency
The color variables forANDROID,IOS,RED,GRAY, andNCare defined appropriately to enable colored log output.
96-113: NDK Configuration for Linux – Download and Extraction
The Linux branch correctly removes any pre-existingndk.zip, downloads the file withcurl -L, checks for errors, and then unzips and copies the required files. The error handling is appropriate.
115-115: Exporting ANDROID_NDK_ROOT
ExportingANDROID_NDK_ROOTafter configuration ensures that subsequent build steps can correctly locate the NDK.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/kos-mobile/build_source.sh (4)
5-23: OS Detection and Environment Variable ExportsThe conditional OS detection logic is clear, and exporting the OS-specific variables ensures downstream scripts have the correct configurations. One minor nitpick: on line 16, there’s an extra space before the variable name. For consistency, consider removing the extra space. For example:
- export OS_TOOLCHAIN="linux-x86_64" + export OS_TOOLCHAIN="linux-x86_64"
78-99: macOS NDK ConfigurationIn the macOS branch of
configure_android_ndk, the script downloads the Android NDK disk image usingcurl -Land manages mounting, copying, and detaching operations with proper error handling. One suggestion: consider parameterizing the hardcoded volume mount path (/Volumes/Android NDK r26b) so that future NDK releases or environment changes can be accommodated more flexibly.
123-140: OpenSSL ConfigurationThe
configure_opensslfunction correctly handles downloading and extracting the OpenSSL source, and it sets the necessary environment variables. To further improve robustness, consider adding error checking for thecurldownload andtarextraction steps so that the script can gracefully handle failures in these operations.
142-171: Assembling OpenSSL LibrariesThe
assemble_openssl_libfunction differentiates between toolchain-specific builds effectively and resets environment variables afterward to avoid contamination. As a suggestion, it might be beneficial to check the exit statuses of themake cleanandmakecommands to catch any build errors early.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/kos-mobile/build_source.sh(4 hunks)
🔇 Additional comments (4)
packages/kos-mobile/build_source.sh (4)
27-32: Android Build Paths and Tools ConfigurationThe exported environment variables for Android JNI libraries, generated binds, architectures, and toolchains are well defined and clearly support the subsequent build processes. Nice work ensuring these paths are correctly set!
39-46: Definition of Color VariablesThe color variables and logging markers are defined clearly. They enhance the readability of log outputs during script execution. This block is straightforward and effective.
47-49: Log Warning Function ImplementationThe
log_warningfunction is implemented concisely to display warning messages in yellow. The function is simple yet effective for its purpose.
100-118: Linux NDK ConfigurationThe Linux branch efficiently downloads the NDK zip file using
curl -L, extracts it, and cleans up afterwards with appropriate error checks. The approach is clear and robust without any issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/kos-hardware/src/models.rs (2)
17-28: Use slice-based copy to simplifyread_to_vec.
Instead of manually looping, consider usingfrom_raw_partsandextend_from_sliceto copy bytes fromsrcinto the vector in a more idiomatic and efficient manner.fn read_to_vec(src: *const u8, src_len: u32) -> Vec<u8> { let mut result = Vec::with_capacity(src_len as usize); if src.is_null() { return result; } unsafe { - for i in 0..src_len as usize { - result.push(*src.add(i)); - } + let slice = core::slice::from_raw_parts(src, src_len as usize); + result.extend_from_slice(slice); } result }
30-39: Consider usingcopy_nonoverlappingforwrite_to_memory.
Usingcore::ptr::copy_nonoverlappingcan reduce loop overhead and clarify the intent of copying bytes from source to destination, while preserving the existing boundary checks.fn write_to_memory(dst: *mut u8, dst_len: u32, src: *const u8, src_len: u32) { if dst.is_null() || src.is_null() || dst_len < src_len { return; } unsafe { - for i in 0..src_len as usize { - *dst.add(i) = *src.add(i); - } + core::ptr::copy_nonoverlapping(src, dst, src_len as usize); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/kos-hardware/src/models.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/kos-hardware/src/models.rs (2)
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:30-39
Timestamp: 2025-03-20T19:09:04.226Z
Learning: In the kos-hardware package, C FFI functions must use plain C-compatible types rather than Rust's native error handling types like Result. Functions like `write_to_memory` are designed to fail silently when preconditions aren't met (null pointers, insufficient buffer size) rather than returning error codes.
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:17-28
Timestamp: 2025-03-20T19:09:12.363Z
Learning: When reviewing Rust code that interfaces with C through FFI (Foreign Function Interface), function signatures must use C-compatible types. Complex Rust types like `Result<T, E>` cannot be directly passed to C. Functions that are called from C should use return values (booleans/integers) to indicate success/failure, out-parameters, or other C-compatible error handling approaches.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (1)
packages/kos-hardware/src/models.rs (1)
177-195: Verify pointer lifetime inDebugErrorHandler.
When callingDebugErrorHandler(format!(...).as_ptr()), the formatted string is dropped once the scope ends. IfDebugErrorHandleruses the pointer asynchronously or stores it, this could lead to undefined behavior. Ensure thatDebugErrorHandleronly uses the string synchronously.Could you confirm whether
DebugErrorHandleris purely synchronous and does not retain the pointer for later use?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/kos-hardware/src/models.rs (2)
17-28: Consider usingslice::from_raw_partsto optimize data copying.Currently,
read_to_vecallocates capacity and then pushes each byte one by one in a loop. Usingslice::from_raw_partsplus a direct copy (e.g.,extend_from_slice) can be more performant for large buffers. However, this loop-based approach is correct from a functional perspective and acceptable for smaller data.
211-239: Gracefully handle potential UTF-8 failures forchaincode_str.If
String::from_utf8(self.chaincode_str.to_vec())fails, the code replaces it with an empty string. This silent fallback is acceptable if you prefer skipping erroneous data. For better diagnostics, consider logging or tracking when invalid bytes occur.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
packages/kos-hardware/src/lib.rs(1 hunks)packages/kos-hardware/src/models.rs(1 hunks)packages/kos/src/chains/klv/mod.rs(1 hunks)packages/kos/src/chains/sol/mod.rs(1 hunks)packages/kos/src/chains/sol/models.rs(1 hunks)packages/kos/src/chains/substrate/mod.rs(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/kos/src/chains/sol/mod.rs
- packages/kos/src/chains/sol/models.rs
- packages/kos/src/chains/klv/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kos/src/chains/substrate/mod.rs
- packages/kos-hardware/src/lib.rs
🧰 Additional context used
🧠 Learnings (1)
packages/kos-hardware/src/models.rs (2)
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:30-39
Timestamp: 2025-03-20T19:58:44.911Z
Learning: In the kos-hardware package, C FFI functions must use plain C-compatible types rather than Rust's native error handling types like Result. Functions like `write_to_memory` are designed to fail silently when preconditions aren't met (null pointers, insufficient buffer size) rather than returning error codes.
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:17-28
Timestamp: 2025-03-20T19:58:17.058Z
Learning: When reviewing Rust code that interfaces with C through FFI (Foreign Function Interface), function signatures must use C-compatible types. Complex Rust types like `Result<T, E>` cannot be directly passed to C. Functions that are called from C should use return values (booleans/integers) to indicate success/failure, out-parameters, or other C-compatible error handling approaches.
🧬 Code Definitions (1)
packages/kos-hardware/src/models.rs (2)
packages/kos-hardware/src/lib.rs (1) (1)
DebugErrorHandler(23-23)packages/kos/src/chains/substrate/mod.rs (1) (1)
new(30-37)
🔇 Additional comments (5)
packages/kos-hardware/src/models.rs (5)
30-39: Silent failure is consistent with the FFI design.This function returns early if pointers are null or if
dst_len < src_len. Based on the retrieved learnings, silently skipping is an intentional choice here to maintain C FFI compatibility. The implementation looks correct for this use case.
85-94: CBuffer implementation aligns withread_to_vec.The
writeandreadmethods mirror the approach inCNodeStruct, ensuring consistent memory handling. This is appropriately minimal for an FFI context.
106-134: CTransaction logic is consistent with other structs.All read/write operations match the established pattern of silent early return on invalid pointers or length mismatch. This uniformly upholds the FFI conventions described in the learnings.
146-209: UTF-8 fallback to empty string is consistent with the existing design.When invalid UTF-8 is encountered (e.g., in sender/receiver), logging occurs via
DebugErrorHandler, followed by returning an emptyString. This aligns with previous decisions to avoid Rust-specific error types in FFI functions.
41-77:❓ Verification inconclusive
Confirm memory allocation and length consistency for seed/pvk/pbk/path.
All the setters (
write_seed,write_pvk, etc.) rely on valid pointers and lengths. Ensure that upstream code allocates memory correctly for these fields before calling these methods.Use this script to locate all occurrences of
CNodeStructusage and confirm memory is allocated:
🏁 Script executed:
#!/bin/bash rg -A 5 'CNodeStruct'Length of output: 3656
Memory Allocation & Length Consistency Check for CNodeStruct Fields
Please verify that the upstream code guarantees that the memory buffers for
seed,pvk,pbk, andpathare allocated with adequate size before calling the corresponding write methods. Each write function (e.g.,write_seed,write_pvk, etc.) assumes that the provided pointer is valid and that its associated length reflects the actual allocated size. Review the callers—such as those inpackages/kos-hardware/src/lib.rs(e.g., inrs_derive,rs_get_pbk, etc.)—to ensure:
- The allocated memory is sufficient to hold the new data of length
data_len.- The length fields (
seed_len,pvk_len, etc.) are updated consistently with the actual memory usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/kos-hardware/src/lib.rs (5)
16-18: Remove unused code or clarify usage.
FreeRtosAllocatoris declared but not used, which might be an oversight. Consider removing it or annotating it with a comment about its intended future usage to keep the codebase clean.
26-29: Consider adding test coverage.While
rust_addis straightforward, ensuring test coverage (particularly in embedded no_std contexts) helps validate basic functionality.
57-59: Ensure consistent error reporting for missing chain.In
rs_get_pbkandrs_get_addr, returningfalsewhen the chain is missing omits any debug message, unlike other functions. For clarity and consistency, consider callingDebugErrorHandlerbefore returningfalse.Also applies to: 79-81
283-311: Refactor repetitive chain-check and error handling logic.The pattern of retrieving the chain with
chains::get_chain_by_paramsand callingDebugErrorHandleron failure is repeated. Extracting this logic into a helper function would reduce duplication, improve readability, and centralize error reporting.
344-346: Add tests for embedded environment.The test module is empty. Although it is a no-std environment, you can often add bare-metal or integration tests to verify critical functionality (e.g., sign, derive, address retrieval). This ensures code stability.
Do you want me to propose a minimal test setup for this library?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
packages/kos-hardware/src/lib.rs(1 hunks)packages/kos/src/chains/sol/mod.rs(2 hunks)packages/kos/src/chains/sol/models.rs(1 hunks)packages/kos/src/chains/substrate/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/kos/src/chains/sol/models.rs
- packages/kos/src/chains/sol/mod.rs
- packages/kos/src/chains/substrate/mod.rs
🧰 Additional context used
🧬 Code Definitions (1)
packages/kos-hardware/src/lib.rs (1)
packages/kos/src/chains/sol/models.rs (2) (2)
encode(172-222)encode(248-264)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (1)
packages/kos-hardware/src/lib.rs (1)
40-40: Validate string pointer usage passed toDebugErrorHandler.Calls to
DebugErrorHandler(format!(...).as_ptr())rely on temporaryStringmemory. IfDebugErrorHandlerprocesses the string asynchronously, the pointer could become invalid once the temporary goes out of scope. Consider ensuring synchronous use or persisting the string data beyond the function call to avoid potential use-after-free errors.Would you like to confirm if
DebugErrorHandlerprocesses the string immediately? We can generate a script to search for the definition ofDebugErrorHandlercalls across the codebase.Also applies to: 105-105, 151-151, 185-185, 213-213, 233-233, 255-255, 289-289, 297-297, 307-307, 317-317
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/kos/src/crypto/mod.rs (1)
18-21: Remove commented-out codeLine 20 contains commented-out code (
// pub mod mnemonic;) which is now redundant since the mnemonic module is properly feature-flagged on line 15-16.#[allow(clippy::module_inception)] #[cfg(not(feature = "ksafe"))] -// pub mod mnemonic; mod crypto {}packages/kos-hardware/src/models.rs (1)
32-41: Consider usingptr::copy_nonoverlappingfor performanceYour current approach iterating over each byte is correct. However, you could simplify and potentially enhance performance by using a built-in memory copy. For example:
fn write_to_memory(dst: *mut u8, dst_len: u32, src: *const u8, src_len: u32) { if dst.is_null() || src.is_null() || dst_len < src_len { return; } unsafe { - for i in 0..src_len as usize { - *dst.add(i) = *src.add(i); - } + std::ptr::copy_nonoverlapping(src, dst, src_len as usize); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
packages/kos-hardware/src/models.rs(1 hunks)packages/kos/src/crypto/cipher.rs(1 hunks)packages/kos/src/crypto/mod.rs(1 hunks)packages/kos/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/kos/src/crypto/cipher.rs
🧰 Additional context used
🧠 Learnings (1)
packages/kos-hardware/src/models.rs (2)
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:30-39
Timestamp: 2025-03-24T12:32:38.386Z
Learning: In the kos-hardware package, C FFI functions must use plain C-compatible types rather than Rust's native error handling types like Result. Functions like `write_to_memory` are designed to fail silently when preconditions aren't met (null pointers, insufficient buffer size) rather than returning error codes.
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:17-28
Timestamp: 2025-03-24T12:32:17.816Z
Learning: When reviewing Rust code that interfaces with C through FFI (Foreign Function Interface), function signatures must use C-compatible types. Complex Rust types like `Result<T, E>` cannot be directly passed to C. Functions that are called from C should use return values (booleans/integers) to indicate success/failure, out-parameters, or other C-compatible error handling approaches.
🧬 Code Definitions (1)
packages/kos-hardware/src/models.rs (3)
packages/kos/src/lib.rs (2)
alloc(26-28)DebugErrorHandler(21-21)packages/kos-hardware/src/lib.rs (1)
DebugErrorHandler(23-23)packages/kos/src/chains/substrate/mod.rs (1)
new(30-37)
🔇 Additional comments (5)
packages/kos/src/lib.rs (1)
38-43:❓ Verification inconclusive
Good refinement of the panic handler configuration
The updated conditional compilation attribute now ensures this panic handler is only included when the "ksafe" feature is enabled, not in test mode, and the "not-ksafe" feature is not enabled. This prevents potential conflicts if both "ksafe" and "not-ksafe" features have their own panic handlers.
🏁 Script executed:
#!/bin/bash # Verify if there's another panic handler when the "not-ksafe" feature is enabled echo "Checking for other panic handlers in the codebase..." rg -A 3 "#\[panic_handler\]" --glob "*.rs" | grep -v "packages/kos/src/lib.rs"Length of output: 193
Panic Handler Setup Verification
The current panic handler in
packages/kos/src/lib.rsis now properly confined by conditional compilation so that it’s only included with the "ksafe" feature enabled (outside of test mode and when "not-ksafe" is not enabled), which should help prevent potential conflicts.However, the automated script intended to locate other
#[panic_handler]definitions produced no output. Given the low quality of that inference, please manually verify that no other panic handlers (especially related to the "not-ksafe" feature) exist elsewhere in the codebase.packages/kos/src/crypto/mod.rs (1)
13-16: Improved feature flag semanticsChanging from negative conditions (
not(feature = "ksafe")) to positive ones (feature = "not-ksafe") improves clarity and maintainability. This makes the code's intent more explicit and aligns with the new feature organization in Cargo.toml.packages/kos-hardware/src/models.rs (3)
19-30: Silent behavior on null pointer is consistentReturning an empty vector if
srcis null aligns with the FFI requirement for silent fallback in this codebase. This is consistent with your documented preference and introduces no correctness issues.
217-233: Logic for selecting chain type looks goodThe
to_chain_typeimplementation properly distinguishes between standard and custom chains while gracefully handling conversion fromchaincode_str. No issues found, and the fallback toNotCustomfor unknown chain IDs is clear.
179-191:❓ Verification inconclusive
Verify the usage of ephemeral string pointers
Because
format!constructs a temporary string before passingas_ptr()toDebugErrorHandler, ensure the function only uses the pointer synchronously. If the pointer is referenced after this method returns (e.g., in an asynchronous context), it may cause undefined behavior.
🏁 Script executed:
#!/bin/bash # Description: Check usage of DebugErrorHandler to ensure it's only used synchronously. rg "DebugErrorHandler"Length of output: 2392
Attention: Synchronous Pointer Usage Verification
The usage of
DebugErrorHandler(format!(...).as_ptr())in the codebase—particularly inpackages/kos-hardware/src/models.rs(lines 179–191)—depends on passing a temporary string’s pointer. Please verify that:
- Synchronous Usage Only: The
DebugErrorHandlerfunction (declared in bothpackages/kos/src/lib.rsand used in other parts of the code) does not store or asynchronously reference the pointer. The temporary string created byformat!is deallocated immediately after the call, so any deferred use could lead to undefined behavior.- Consistent Pattern: Review all call sites (as shown by the grep results) to ensure they are used in a strictly synchronous context.
Taking extra care here will help prevent potential subtle bugs related to unsafe pointer lifetimes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kos/Cargo.toml (1)
31-31: Fixed formatting in pbkdf2 dependencyRemoved an extra space in the version specification for the pbkdf2 dependency, which improves consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.vscode/settings.json(1 hunks)Cargo.toml(1 hunks)packages/kos-mobile/Cargo.toml(1 hunks)packages/kos-web/Cargo.toml(1 hunks)packages/kos/Cargo.toml(2 hunks)packages/kos/src/chains/klv/mod.rs(1 hunks)packages/kos/src/chains/sol/mod.rs(2 hunks)packages/kos/src/chains/substrate/mod.rs(3 hunks)packages/kos/src/crypto/secp256k1.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .vscode/settings.json
- packages/kos/src/chains/sol/mod.rs
- packages/kos-mobile/Cargo.toml
- packages/kos/src/chains/klv/mod.rs
- packages/kos/src/chains/substrate/mod.rs
- Cargo.toml
🔇 Additional comments (8)
packages/kos-web/Cargo.toml (1)
29-29: Dependency configuration update:kosnow uses thenot-ksafefeatureThe dependency on
koshas been updated to explicitly include thenot-ksafefeature. This change aligns with the modular approach being implemented across the project, which separates hardware-secure implementations from software-only implementations.packages/kos/src/crypto/secp256k1.rs (4)
1-1: AddedVecimport from allocThis import is necessary to support the updated return type of
sign_dermethod, which now returns a variable-length vector instead of a fixed-size array.
45-45: Return type change: Fixed array to dynamic VecThe
sign_dermethod's return type has been changed from a fixed-size array to a dynamicVec<u8>, which is appropriate for DER-encoded signatures that can vary in length.
91-104: Hardware implementation forsign_dermethodThis is the implementation of
sign_derfor when theksafefeature is enabled. It correctly initializes a buffer large enough for DER signatures (72 bytes) and uses the C FFI function to fill it before converting to aVec.Is 72 bytes always sufficient for DER-encoded signatures? You might want to verify if there are edge cases where the signature could be larger.
139-145: Software implementation forsign_dermethodThis implementation for when
ksafeis not enabled uses thelibsecp256k1library to generate the signature and properly converts it to a vector.packages/kos/Cargo.toml (3)
38-49: Made multiple dependencies optionalAll these dependencies have been made optional, which supports the more modular approach to feature management. This will allow users to only include the dependencies they need based on selected features.
61-77: Added feature configuration with default set to "not-ksafe"The new feature configuration creates a clear separation between hardware-secure and software implementations. Setting
default = ["not-ksafe"]ensures that by default, all the optional dependencies will be included, maintaining backward compatibility while enabling more flexibility.Consider documenting these features in a README or documentation file to help users understand when to use
ksafevsnot-ksafefeatures.
80-80: Simplified extension featureThe
extensionfeature now only includesgetrandom/jsrather than repeating all dependencies. This is a cleaner approach that avoids duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/kos-hardware/src/models.rs (3)
43-79: Consider reducing repetition of the read/write logic.
Each field’s read/write calls share similar logic. A helper function or macro could reduce code duplication, although this is optional.
81-96: Similar repetitive logic observed.
CBuffer's read/write patterns mirrorCNodeStructand others. Consider centralizing this common functionality if it simplifies maintenance.
98-136: Consistent usage of read/write logic.
This maintains the pattern used elsewhere in the file. If desired, you could still unify these methods in a shared implementation or macro.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
packages/kos-hardware/src/lib.rs(1 hunks)packages/kos-hardware/src/models.rs(1 hunks)packages/kos/src/crypto/cipher.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kos/src/crypto/cipher.rs
- packages/kos-hardware/src/lib.rs
🧰 Additional context used
🧠 Learnings (1)
packages/kos-hardware/src/models.rs (2)
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:30-39
Timestamp: 2025-03-24T13:30:51.738Z
Learning: In the kos-hardware package, C FFI functions must use plain C-compatible types rather than Rust's native error handling types like Result. Functions like `write_to_memory` are designed to fail silently when preconditions aren't met (null pointers, insufficient buffer size) rather than returning error codes.
Learnt from: klever-patrick
PR: klever-io/kos-rs#116
File: packages/kos-hardware/src/models.rs:17-28
Timestamp: 2025-03-24T13:30:36.296Z
Learning: When reviewing Rust code that interfaces with C through FFI (Foreign Function Interface), function signatures must use C-compatible types. Complex Rust types like `Result<T, E>` cannot be directly passed to C. Functions that are called from C should use return values (booleans/integers) to indicate success/failure, out-parameters, or other C-compatible error handling approaches.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (5)
packages/kos-hardware/src/models.rs (5)
7-17: Struct layout looks good for FFI.
#[repr(C)]ensures a predictable memory layout, which is correct for C interop. No further issues noted.
19-30: Silently returning an empty vector on null pointers is a known design choice.
This behavior matches prior discussions regarding FFI and error handling constraints, so no further action is required.
32-41: Skipping writes on null pointers or insufficient buffer space is acceptable for C FFI.
This design choice has been previously reviewed and is recognized as intentional.
138-205: UTF-8 fallback approach is consistent with previous design decisions.
DebugErrorHandleris invoked on invalid UTF-8 conversions, and the code proceeds with an empty string. Looks fine for this FFI context.
208-236: Appropriate fallback for invalid chain code strings.
Following the existing pattern, an empty string is used upon UTF-8 failure. This remains consistent with your established conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
Cargo.toml(2 hunks)packages/kos/src/chains/icp/mod.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/kos/src/chains/icp/mod.rs (9)
packages/kos/src/chains/mod.rs (1)
sign_tx(384-384)packages/kos/src/chains/substrate/mod.rs (1)
sign_tx(108-185)packages/kos/src/chains/xrp/mod.rs (1)
sign_tx(111-119)packages/kos/src/chains/eth/mod.rs (1)
sign_tx(114-144)packages/kos/src/chains/sol/mod.rs (1)
sign_tx(58-92)packages/kos/src/chains/btc/mod.rs (1)
sign_tx(165-275)packages/kos/src/chains/klv/mod.rs (1)
sign_tx(82-114)packages/kos/src/chains/sui/mod.rs (1)
sign_tx(65-74)packages/kos/src/chains/trx/mod.rs (1)
sign_tx(113-147)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (4)
Cargo.toml (3)
5-6: LGTM - New hardware package added to workspace membersThe workspace members list has been updated to include the new kos-hardware package, which aligns with the PR objectives.
13-19: The hardware profile configuration is well-suited for embedded targetsThe new profile has appropriate optimizations for hardware targets:
- Size optimization with
opt-level = 'z'- LTO enabled for size reduction
- Panic handler set to abort (saves space by not unwinding)
- Reduced codegen units for better optimization
- Symbol stripping to minimize binary size
These settings will help produce compact binaries suitable for resource-constrained hardware environments.
54-54:❓ Verification inconclusive
Verify the implications of the "not-ksafe" feature flag
The kos dependency now explicitly enables the "not-ksafe" feature. This appears to be related to the conditional compilation in the ICP module.
🏁 Script executed:
#!/bin/bash # Check how the not-ksafe feature is defined and used grep -r "not-ksafe" --include="*.toml" .Length of output: 332
Action: Confirm that the “not-ksafe” flag is intentionally enabled and its conditional behavior in the ICP module is as expected.
- Our grep results show that the
not-ksafeflag is consistently enabled:
- In the root
Cargo.toml, thekosdependency is defined withfeatures = ["not-ksafe"].- In
./packages/kos-web/Cargo.toml, the same flag is explicitly set.- In
./packages/kos/Cargo.toml, it appears both in the default feature list and in the feature definition.- This consistency suggests that its use is deliberate across the workspace.
- Next Verification: Please ensure that the conditional compilation in the ICP module correctly responds to this flag—confirm that any safety-related logic toggled by
not-ksafebehaves as intended.packages/kos/src/chains/icp/mod.rs (1)
79-113: LGTM - Conditional compilation for the standard sign_tx implementationThe original ICP sign_tx implementation is now conditionally compiled when the "ksafe" feature is not enabled. This allows for different implementations based on feature flags.
Description
Please provide a brief summary of the changes. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Fixes # (issue)
Type of Change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
make checkSummary by CodeRabbit
New Features
koscrate dependency in web and mobile packages to include feature flags.sign_txmethod in theICPstruct to support dual implementations based on feature flags.Chores
.gitignorefile in the hardware package to manage version control.