Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion key-wallet-ffi/include/key_wallet_ffi.h
Original file line number Diff line number Diff line change
Expand Up @@ -3789,16 +3789,30 @@ void wallet_manager_free_addresses(char **addresses,

/*
Encrypt a private key with BIP38

# Safety

This function is unsafe because it dereferences raw pointers:
- `private_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/

char *bip38_encrypt_private_key(const char *private_key,
const char *passphrase,
FFINetwork network,
FFINetwork _network,
FFIError *error)
;
Comment on lines +3792 to 3805

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

BIP38 encrypt docs: document allocation/free and current unimplemented behavior

The header doesn’t say how to free the returned string and that this currently always returns NULL with WALLET_ERROR (feature disabled or not implemented). Please document both; also note _network is reserved/ignored.

Apply this header doc tweak (mirror in Rust doc comments so cbindgen regenerates it):

-/*
- Encrypt a private key with BIP38
-
- # Safety
- 
- This function is unsafe because it dereferences raw pointers:
- - `private_key` must be a valid, null-terminated C string
- - `passphrase` must be a valid, null-terminated C string
- - `error` must be a valid pointer to an FFIError or null
- */
+/*
+ Encrypt a private key with BIP38
+ 
+ Returns: newly allocated C string on success; NULL on error.
+ - Free the returned string with `string_free`.
+ - Currently unimplemented: returns NULL and sets error->code = WALLET_ERROR (or when the `bip38` feature is disabled).
+ - Note: `_network` is reserved/ignored at present.
+ 
+ # Safety
+ This function is unsafe because it dereferences raw pointers:
+ - `private_key` must be a valid, null-terminated C string
+ - `passphrase` must be a valid, null-terminated C string
+ - `error` must be a valid pointer to an FFIError or null
+ */
📝 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.

Suggested change
# Safety
This function is unsafe because it dereferences raw pointers:
- `private_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/
char *bip38_encrypt_private_key(const char *private_key,
const char *passphrase,
FFINetwork network,
FFINetwork _network,
FFIError *error)
;
/*
Encrypt a private key with BIP38
Returns: newly allocated C string on success; NULL on error.
- Free the returned string with `string_free`.
- Currently unimplemented: returns NULL and sets error->code = WALLET_ERROR (or when the `bip38` feature is disabled).
- Note: `_network` is reserved/ignored at present.
# Safety
This function is unsafe because it dereferences raw pointers:
- `private_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/
char *bip38_encrypt_private_key(const char *private_key,
const char *passphrase,
FFINetwork _network,
FFIError *error)
;
🤖 Prompt for AI Agents
In key-wallet-ffi/include/key_wallet_ffi.h around lines 3792 to 3805, the BIP38
encrypt function header lacks docs about ownership and current unimplemented
behavior: update the comment to state that the returned char* is heap-allocated
and must be freed by the caller (e.g. free()), or NULL on error; explicitly
document that this implementation currently always returns NULL and sets
error->code = WALLET_ERROR when the feature is disabled/not implemented; also
note that the _network parameter is reserved/ignored. Make the same text change
in the corresponding Rust doc comments so cbindgen regenerates the header with
these notes.


/*
Decrypt a BIP38 encrypted private key

# Safety

This function is unsafe because it dereferences raw pointers:
- `encrypted_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/
Comment on lines 3808 to 3816

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

FFI contract: document allocation/ownership for BIP38 decrypt

State who frees the returned string and typical error path.

-/*
- Decrypt a BIP38 encrypted private key
-
- # Safety
+/*
+ Decrypt a BIP38 encrypted private key
+ 
+ Returns: newly allocated C string (WIF) on success; NULL on error.
+ - Free the returned string with `string_free`.
+ 
+ # Safety
  This function is unsafe because it dereferences raw pointers:
  - `encrypted_key` must be a valid, null-terminated C string
  - `passphrase` must be a valid, null-terminated C string
  - `error` must be a valid pointer to an FFIError or null
 */
📝 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.

Suggested change
Decrypt a BIP38 encrypted private key
# Safety
This function is unsafe because it dereferences raw pointers:
- `encrypted_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/
/*
Decrypt a BIP38 encrypted private key
Returns: newly allocated C string (WIF) on success; NULL on error.
- Free the returned string with `string_free`.
# Safety
This function is unsafe because it dereferences raw pointers:
- `encrypted_key` must be a valid, null-terminated C string
- `passphrase` must be a valid, null-terminated C string
- `error` must be a valid pointer to an FFIError or null
*/
🤖 Prompt for AI Agents
In key-wallet-ffi/include/key_wallet_ffi.h around lines 3800 to 3808, the BIP38
decrypt FFI comment lacks allocation/ownership and error-path details; update
the docblock to state precisely who owns and must free returned memory (e.g.,
the function returns a heap-allocated NUL-terminated C string which the caller
must free with the FFI's free function/standard free), and describe the error
path (on failure return NULL and, if an error pointer is provided, populate
error->message with a heap-allocated NUL-terminated C string that the caller
must also free; clarify that the error object itself is written by callee into
the provided pointer and not allocated). Ensure the wording names the allocator
used (malloc/ffi_free) and that both success and error message lifetimes are the
caller's responsibility.


char *bip38_decrypt_private_key(const char *encrypted_key,
Expand Down
24 changes: 12 additions & 12 deletions key-wallet-ffi/src/account_collection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@ mod tests {
options.option_type = crate::types::FFIAccountCreationOptionType::AllAccounts;

// Add provider operator keys account type
let special_types = vec![crate::types::FFIAccountType::ProviderOperatorKeys];
let special_types = [crate::types::FFIAccountType::ProviderOperatorKeys];
options.special_account_types = special_types.as_ptr();
options.special_account_types_count = special_types.len();

Expand Down Expand Up @@ -1199,7 +1199,7 @@ mod tests {
options.option_type = crate::types::FFIAccountCreationOptionType::AllAccounts;

// Add provider platform keys account type
let special_types = vec![crate::types::FFIAccountType::ProviderPlatformKeys];
let special_types = [crate::types::FFIAccountType::ProviderPlatformKeys];
options.special_account_types = special_types.as_ptr();
options.special_account_types_count = special_types.len();

Expand Down Expand Up @@ -1248,7 +1248,7 @@ mod tests {
options.option_type = crate::types::FFIAccountCreationOptionType::AllAccounts;

// Add various special accounts
let special_types = vec![
let special_types = [
crate::types::FFIAccountType::ProviderVotingKeys,
crate::types::FFIAccountType::ProviderOwnerKeys,
crate::types::FFIAccountType::IdentityRegistration,
Expand All @@ -1258,10 +1258,10 @@ mod tests {
options.special_account_types_count = special_types.len();

// Configure standard accounts - store vectors in variables to keep them alive
let bip44_indices = vec![0, 4, 5, 8];
let bip32_indices = vec![0];
let coinjoin_indices = vec![0, 1];
let topup_indices = vec![0, 1, 2];
let bip44_indices = [0, 4, 5, 8];
let bip32_indices = [0];
let coinjoin_indices = [0, 1];
let topup_indices = [0, 1, 2];

options.bip44_indices = bip44_indices.as_ptr();
options.bip44_count = bip44_indices.len();
Expand Down Expand Up @@ -1392,7 +1392,7 @@ mod tests {
options.option_type = crate::types::FFIAccountCreationOptionType::AllAccounts;

// Add various special accounts
let special_types = vec![
let special_types = [
crate::types::FFIAccountType::ProviderVotingKeys,
crate::types::FFIAccountType::ProviderOwnerKeys,
crate::types::FFIAccountType::IdentityRegistration,
Expand All @@ -1402,10 +1402,10 @@ mod tests {
options.special_account_types_count = special_types.len();

// Configure standard accounts
let bip44_indices = vec![0, 4, 5, 8];
let bip32_indices = vec![0];
let coinjoin_indices = vec![0, 1];
let topup_indices = vec![0, 1, 2];
let bip44_indices = [0, 4, 5, 8];
let bip32_indices = [0];
let coinjoin_indices = [0, 1];
let topup_indices = [0, 1, 2];

options.bip44_indices = bip44_indices.as_ptr();
options.bip44_count = bip44_indices.len();
Expand Down
1 change: 1 addition & 0 deletions key-wallet-ffi/src/account_tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#[cfg(test)]
#[allow(clippy::module_inception)]
mod tests {
use super::super::*;
use crate::error::{FFIError, FFIErrorCode};
Expand Down
102 changes: 54 additions & 48 deletions key-wallet-ffi/src/bip38.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
//! BIP38 encryption support

use std::ffi::{CStr, CString};
use std::ffi::CStr;
use std::os::raw::c_char;
use std::ptr;

use crate::error::{FFIError, FFIErrorCode};
use crate::types::FFINetwork;

/// Encrypt a private key with BIP38
///
/// # Safety
///
/// This function is unsafe because it dereferences raw pointers:
/// - `private_key` must be a valid, null-terminated C string
/// - `passphrase` must be a valid, null-terminated C string
/// - `error` must be a valid pointer to an FFIError or null
#[no_mangle]
pub extern "C" fn bip38_encrypt_private_key(
pub unsafe extern "C" fn bip38_encrypt_private_key(
private_key: *const c_char,
passphrase: *const c_char,
network: FFINetwork,
_network: FFINetwork,
error: *mut FFIError,
) -> *mut c_char {
#[cfg(feature = "bip38")]
Expand All @@ -26,31 +33,27 @@ pub extern "C" fn bip38_encrypt_private_key(
return ptr::null_mut();
}

let privkey_str = unsafe {
match CStr::from_ptr(private_key).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in private key".to_string(),
);
return ptr::null_mut();
}
let _privkey_str = match CStr::from_ptr(private_key).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in private key".to_string(),
);
return ptr::null_mut();
}
};

let passphrase_str = unsafe {
match CStr::from_ptr(passphrase).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in passphrase".to_string(),
);
return ptr::null_mut();
}
let _passphrase_str = match CStr::from_ptr(passphrase).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in passphrase".to_string(),
);
return ptr::null_mut();
}
};

Expand All @@ -75,8 +78,15 @@ pub extern "C" fn bip38_encrypt_private_key(
}

/// Decrypt a BIP38 encrypted private key
///
/// # Safety
///
/// This function is unsafe because it dereferences raw pointers:
/// - `encrypted_key` must be a valid, null-terminated C string
/// - `passphrase` must be a valid, null-terminated C string
/// - `error` must be a valid pointer to an FFIError or null
#[no_mangle]
pub extern "C" fn bip38_decrypt_private_key(
pub unsafe extern "C" fn bip38_decrypt_private_key(
encrypted_key: *const c_char,
passphrase: *const c_char,
error: *mut FFIError,
Expand All @@ -92,31 +102,27 @@ pub extern "C" fn bip38_decrypt_private_key(
return ptr::null_mut();
}

let encrypted_str = unsafe {
match CStr::from_ptr(encrypted_key).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in encrypted key".to_string(),
);
return ptr::null_mut();
}
let _encrypted_str = match CStr::from_ptr(encrypted_key).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in encrypted key".to_string(),
);
return ptr::null_mut();
}
};

let passphrase_str = unsafe {
match CStr::from_ptr(passphrase).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in passphrase".to_string(),
);
return ptr::null_mut();
}
let _passphrase_str = match CStr::from_ptr(passphrase).to_str() {
Ok(s) => s,
Err(_) => {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Invalid UTF-8 in passphrase".to_string(),
);
return ptr::null_mut();
}
};

Expand Down
1 change: 1 addition & 0 deletions key-wallet-ffi/src/derivation_tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Tests for derivation path FFI functions

#[cfg(test)]
#[allow(clippy::module_inception)]
mod tests {
use crate::derivation::*;
use crate::error::{FFIError, FFIErrorCode};
Expand Down
1 change: 1 addition & 0 deletions key-wallet-ffi/src/keys_tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Tests for key derivation FFI functions

#[cfg(test)]
#[allow(clippy::module_inception)]
mod tests {
use crate::error::{FFIError, FFIErrorCode};
use crate::keys::*;
Expand Down
14 changes: 7 additions & 7 deletions key-wallet-ffi/src/managed_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ mod tests {

let mut options = FFIWalletAccountCreationOptions::default_options();
options.option_type = FFIAccountCreationOptionType::BIP44AccountsOnly;
let bip44_indices = vec![0];
let bip44_indices = [0];
options.bip44_indices = bip44_indices.as_ptr();
options.bip44_count = bip44_indices.len();

Expand Down Expand Up @@ -929,9 +929,9 @@ mod tests {
let mut options = FFIWalletAccountCreationOptions::default_options();
options.option_type = FFIAccountCreationOptionType::AllAccounts;

let bip44_indices = vec![0, 1, 2];
let bip32_indices = vec![0];
let coinjoin_indices = vec![0];
let bip44_indices = [0, 1, 2];
let bip32_indices = [0];
let coinjoin_indices = [0];

options.bip44_indices = bip44_indices.as_ptr();
options.bip44_count = bip44_indices.len();
Expand Down Expand Up @@ -1046,7 +1046,7 @@ mod tests {

// Test get_is_watch_only
let is_watch_only = managed_account_get_is_watch_only(account);
assert_eq!(is_watch_only, false);
assert!(!is_watch_only);

// Test get_balance
let mut balance_out = crate::types::FFIBalance {
Expand Down Expand Up @@ -1094,7 +1094,7 @@ mod tests {
assert_eq!(account_type, FFIAccountType::StandardBIP44); // Default type

let is_watch_only = managed_account_get_is_watch_only(ptr::null());
assert_eq!(is_watch_only, false);
assert!(!is_watch_only);

let tx_count = managed_account_get_transaction_count(ptr::null());
assert_eq!(tx_count, 0);
Expand Down Expand Up @@ -1261,7 +1261,7 @@ mod tests {
// Create wallet with CoinJoin account
let mut options = FFIWalletAccountCreationOptions::default_options();
options.option_type = FFIAccountCreationOptionType::SpecificAccounts;
let coinjoin_indices = vec![0];
let coinjoin_indices = [0];
options.coinjoin_indices = coinjoin_indices.as_ptr();
options.coinjoin_count = coinjoin_indices.len();

Expand Down
6 changes: 3 additions & 3 deletions key-wallet-ffi/src/managed_wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ mod tests {

// Create managed wallet info from the wallet
let wallet_rust = unsafe { &(*wallet).wallet };
let managed_info = ManagedWalletInfo::from_wallet(&wallet_rust);
let managed_info = ManagedWalletInfo::from_wallet(wallet_rust);
let mut ffi_managed = FFIManagedWalletInfo::new(managed_info);

// Test get_next_receive_address with valid pointers
Expand Down Expand Up @@ -995,7 +995,7 @@ mod tests {

// We need to work with the existing wallet structure
// Create managed wallet info from the existing wallet
let mut managed_info = ManagedWalletInfo::from_wallet(&wallet_arc);
let mut managed_info = ManagedWalletInfo::from_wallet(wallet_arc);

let network = key_wallet::Network::Testnet;

Expand Down Expand Up @@ -1177,7 +1177,7 @@ mod tests {

// Create managed wallet info
let wallet_arc = unsafe { &(*wallet_ptr).wallet };
let mut managed_info = ManagedWalletInfo::from_wallet(&wallet_arc);
let mut managed_info = ManagedWalletInfo::from_wallet(wallet_arc);

// Set some test balance values
managed_info.balance = WalletBalance {
Expand Down
2 changes: 1 addition & 1 deletion key-wallet-ffi/src/managed_wallet_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ mod tests {
};

// Create invalid UTF-8 string
let invalid_utf8 = vec![0xFF, 0xFE, 0xFD, 0x00]; // Invalid UTF-8 bytes with null terminator
let invalid_utf8 = [0xFF, 0xFE, 0xFD, 0x00]; // Invalid UTF-8 bytes with null terminator
let success = unsafe {
managed_wallet_mark_address_used(
managed_wallet,
Expand Down
7 changes: 4 additions & 3 deletions key-wallet-ffi/src/mnemonic_tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Unit tests for mnemonic FFI module

#[cfg(test)]
#[allow(clippy::module_inception)]
mod tests {
use crate::error::{FFIError, FFIErrorCode};
use crate::mnemonic;
Expand Down Expand Up @@ -380,7 +381,7 @@ mod tests {
let mut error = FFIError::success();

// Create invalid UTF-8 string
let invalid_utf8 = vec![0xFF, 0xFE, 0xFD, 0x00];
let invalid_utf8 = [0xFF, 0xFE, 0xFD, 0x00];
let count = unsafe {
mnemonic::mnemonic_word_count(
invalid_utf8.as_ptr() as *const std::os::raw::c_char,
Expand Down Expand Up @@ -467,7 +468,7 @@ mod tests {
let mut seed_len = 0usize;

// Test invalid UTF-8 in mnemonic
let invalid_utf8 = vec![0xFF, 0xFE, 0xFD, 0x00];
let invalid_utf8 = [0xFF, 0xFE, 0xFD, 0x00];
let success = unsafe {
mnemonic::mnemonic_to_seed(
invalid_utf8.as_ptr() as *const std::os::raw::c_char,
Expand Down Expand Up @@ -500,7 +501,7 @@ mod tests {
let mut error = FFIError::success();

// Create invalid UTF-8 string
let invalid_utf8 = vec![0xFF, 0xFE, 0xFD, 0x00];
let invalid_utf8 = [0xFF, 0xFE, 0xFD, 0x00];
let is_valid = unsafe {
mnemonic::mnemonic_validate(
invalid_utf8.as_ptr() as *const std::os::raw::c_char,
Expand Down
Loading