Skip to content

Refactor/transaction builder #496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ZocoLini wants to merge 6 commits into
base: v0.42-dev
Choose a base branch
from
Draft

Refactor/transaction builder #496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
614b36e
drop function to create unisgned transactions in WalletManager and in…
ZocoLini Feb 27, 2026
1e19077
TransactionBuilder and CoinSelector logic refactor/fix
ZocoLini Mar 3, 2026
94de010
utxos selection logic refactor
ZocoLini Mar 4, 2026
fb4b186
transaction signing implemented
ZocoLini Mar 4, 2026
06e968a
updating to the new changes
ZocoLini Mar 4, 2026
5144810
TransactionBuilder constructor improvement
ZocoLini Mar 5, 2026
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
2 changes: 1 addition & 1 deletion key-wallet-ffi/include/key_wallet_ffi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3525,7 +3525,7 @@ bool mnemonic_to_seed(const char *mnemonic,
*/

bool wallet_build_and_sign_transaction(const FFIWalletManager *manager,
const FFIWallet *wallet,
const uint8_t *wallet_id,
uint32_t account_index,
const FFITxOutput *outputs,
size_t outputs_count,
Expand Down
171 changes: 12 additions & 159 deletions key-wallet-ffi/src/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,18 @@

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

use dashcore::{
consensus, hashes::Hash, sighash::SighashCache, EcdsaSighashType, Network, OutPoint, Script,
ScriptBuf, Transaction, TxIn, TxOut, Txid,
};
use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
use key_wallet_manager::FeeRate;
use key_wallet::transaction::{FeeRate, TransactionBuilder};
use secp256k1::{Message, Secp256k1, SecretKey};

use crate::error::{FFIError, FFIErrorCode};
use crate::types::{FFINetwork, FFITransactionContext, FFIWallet};
use crate::FFIWalletManager;
use crate::{wallet, FFIWalletManager};

// MARK: - Transaction Types

Expand Down Expand Up @@ -81,7 +79,7 @@ pub struct FFITxOutput {
#[no_mangle]
pub unsafe extern "C" fn wallet_build_and_sign_transaction(
manager: *const FFIWalletManager,
wallet: *const FFIWallet,
wallet_id: *const u8,
account_index: u32,
outputs: *const FFITxOutput,
outputs_count: usize,
Expand All @@ -93,7 +91,7 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
) -> bool {
// Validate inputs
if manager.is_null()
|| wallet.is_null()
|| wallet_id.is_null()
|| outputs.is_null()
|| tx_bytes_out.is_null()
|| tx_len_out.is_null()
Expand All @@ -103,67 +101,16 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
return false;
}

if outputs_count == 0 {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"At least one output required".to_string(),
);
return false;
}

unsafe {
use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy;
use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder;

let manager_ref = &*manager;
let wallet_ref = &*wallet;
let network_rust = wallet_ref.inner().network;
let wallet_id = slice::from_raw_parts(wallet_id, 32).try_into().unwrap();
let outputs_slice = slice::from_raw_parts(outputs, outputs_count);

manager_ref.runtime.block_on(async {
let mut manager = manager_ref.manager.write().await;

let managed_wallet = manager.get_wallet_info_mut(&wallet_ref.inner().wallet_id);

let Some(managed_wallet) = managed_wallet else {
FFIError::set_error(
error,
FFIErrorCode::InvalidInput,
"Could not obtain ManagedWalletInfo for the provided wallet".to_string(),
);
return false;
};

// Get the managed account
let managed_account =
match managed_wallet.accounts.standard_bip44_accounts.get_mut(&account_index) {
Some(account) => account,
None => {
FFIError::set_error(
error,
FFIErrorCode::WalletError,
format!("Account {} not found", account_index),
);
return false;
}
};

let wallet_account =
match wallet_ref.inner().accounts.standard_bip44_accounts.get(&account_index) {
Some(account) => account,
None => {
FFIError::set_error(
error,
FFIErrorCode::WalletError,
format!("Wallet account {} not found", account_index),
);
return false;
}
};

// Convert FFI outputs to Rust outputs
let mut tx_builder = TransactionBuilder::new();
let mut tx_builder = TransactionBuilder::new(&mut manager, wallet_id, account_index);

for output in outputs_slice {
if output.address.is_null() {
Expand Down Expand Up @@ -193,7 +140,7 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
let address = match dashcore::Address::from_str(address_str) {
Ok(addr) => {
// Verify network matches
let addr_network = addr.require_network(network_rust).map_err(|e| {
let addr_network = addr.require_network(wallet.network).map_err(|e| {
FFIError::set_error(
error,
FFIErrorCode::InvalidAddress,
Expand Down Expand Up @@ -229,93 +176,10 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
};
}

// Get change address (next internal address)
let xpub = wallet_account.extended_public_key();
let change_address = match managed_account.next_change_address(Some(&xpub), true) {
Ok(addr) => addr,
Err(e) => {
FFIError::set_error(
error,
FFIErrorCode::WalletError,
format!("Failed to get change address: {}", e),
);
return false;
}
};

tx_builder = tx_builder
.set_change_address(change_address)
.set_fee_rate(FeeRate::new(fee_per_kb));

// Get available UTXOs (collect owned UTXOs, not references)
let utxos: Vec<key_wallet::Utxo> = managed_account.utxos.values().cloned().collect();

// Get the wallet's root extended private key for signing
use key_wallet::wallet::WalletType;

let root_xpriv = match &wallet_ref.inner().wallet_type {
WalletType::Mnemonic {
root_extended_private_key,
..
} => root_extended_private_key,
WalletType::Seed {
root_extended_private_key,
..
} => root_extended_private_key,
WalletType::ExtendedPrivKey(root_extended_private_key) => root_extended_private_key,
_ => {
FFIError::set_error(
error,
FFIErrorCode::WalletError,
"Cannot sign with watch-only wallet".to_string(),
);
return false;
}
};
let fee_rate = FeeRate::new(fee_per_kb);

// Build a map of address -> derivation path for all addresses in the account
use std::collections::HashMap;
let mut address_to_path: HashMap<dashcore::Address, key_wallet::DerivationPath> =
HashMap::new();

// Collect from all address pools (receive, change, etc.)
for pool in managed_account.account_type.address_pools() {
for addr_info in pool.addresses.values() {
address_to_path.insert(addr_info.address.clone(), addr_info.path.clone());
}
}

// Select inputs and build transaction
let mut tx_builder_with_inputs = match tx_builder.select_inputs(
&utxos,
SelectionStrategy::BranchAndBound,
managed_wallet.synced_height(),
|utxo| {
// Look up the derivation path for this UTXO's address
let path = address_to_path.get(&utxo.address)?;

// Convert root key to ExtendedPrivKey and derive the child key
let root_ext_priv = root_xpriv.to_extended_priv_key(network_rust);
let secp = secp256k1::Secp256k1::new();
let derived_xpriv = root_ext_priv.derive_priv(&secp, path).ok()?;

Some(derived_xpriv.private_key)
},
) {
let transaction = match tx_builder.set_fee_rate(fee_rate).build() {
Ok(builder) => builder,
Err(e) => {
FFIError::set_error(
error,
FFIErrorCode::WalletError,
format!("Coin selection failed: {}", e),
);
return false;
}
};

// Build and sign the transaction
let transaction = match tx_builder_with_inputs.build() {
Ok(tx) => tx,
Err(e) => {
FFIError::set_error(
error,
Expand All @@ -326,19 +190,6 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
}
};

// This is tricky, the transaction creation + fee calculation need a little
// bit of love to avoid this kind of logic.
//
// First, we need to know that TransactionBuilder may add an extra output for change
// to the final transaction but not to itself, with that knowledge, we can compare the
// number of outputs in the transaction with the number of outputs in the TransactionBuilder
// to then call the appropriate fee calculation method
*fee_out = if transaction.output.len() > tx_builder_with_inputs.outputs().len() {
tx_builder_with_inputs.calculate_fee_with_extra_output()
} else {
tx_builder_with_inputs.calculate_fee()
};

// Serialize the transaction
let serialized = consensus::serialize(&transaction);
let size = serialized.len();
Expand All @@ -349,6 +200,8 @@ pub unsafe extern "C" fn wallet_build_and_sign_transaction(
*tx_bytes_out = tx_bytes;
*tx_len_out = size;

*fee_out = fee_rate.calculate_fee(size);

FFIError::set_success(error);
true
})
Expand Down
3 changes: 1 addition & 2 deletions key-wallet-manager/examples/wallet_creation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@

use key_wallet::account::StandardAccountType;
use key_wallet::wallet::initialization::WalletAccountCreationOptions;
use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference;
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
use key_wallet::{AccountType, Network};
use key_wallet_manager::wallet_interface::WalletInterface;
use key_wallet_manager::wallet_manager::WalletManager;
use key_wallet_manager::wallet_manager::{AccountTypePreference, WalletManager};

fn main() {
println!("=== Wallet Creation Example ===\n");
Expand Down
5 changes: 0 additions & 5 deletions key-wallet-manager/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,5 @@ pub use dashcore::{OutPoint, TxIn, TxOut};

// Export our high-level types
pub use events::WalletEvent;
pub use key_wallet::wallet::managed_wallet_info::coin_selection::{
CoinSelector, SelectionResult, SelectionStrategy,
};
pub use key_wallet::wallet::managed_wallet_info::fee::FeeRate;
pub use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder;
pub use wallet_interface::BlockProcessingResult;
pub use wallet_manager::{WalletError, WalletManager};
15 changes: 13 additions & 2 deletions key-wallet-manager/src/wallet_manager/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

mod matching;
mod process_block;
mod transaction_building;

pub use crate::wallet_manager::matching::{check_compact_filters_for_addresses, FilterMatchKey};
use alloc::collections::BTreeMap;
Expand All @@ -16,7 +15,6 @@ use dashcore::blockdata::transaction::Transaction;
use dashcore::prelude::CoreBlockHeight;
use key_wallet::account::AccountCollection;
use key_wallet::transaction_checking::TransactionContext;
use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference;
use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
use key_wallet::wallet::managed_wallet_info::{ManagedWalletInfo, TransactionRecord};
use key_wallet::wallet::WalletType;
Expand All @@ -41,6 +39,19 @@ pub type WalletId = [u8; 32];
/// Unique identifier for an account within a wallet
pub type AccountId = u32;

/// Account type preference for transaction building
#[derive(Debug, Clone, Copy)]
pub enum AccountTypePreference {
/// Use BIP44 account only
BIP44,
/// Use BIP32 account only
BIP32,
/// Prefer BIP44, fallback to BIP32
PreferBIP44,
/// Prefer BIP32, fallback to BIP44
PreferBIP32,
}

/// The actual account type that was used for address generation
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum AccountTypeUsed {
Expand Down
56 changes: 0 additions & 56 deletions key-wallet-manager/src/wallet_manager/transaction_building.rs
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions key-wallet-manager/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@
//! works correctly with the low-level key-wallet primitives.

use key_wallet::wallet::initialization::WalletAccountCreationOptions;
use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference;
use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
use key_wallet::{mnemonic::Language, Mnemonic, Network};
use key_wallet_manager::wallet_interface::WalletInterface;
use key_wallet_manager::wallet_manager::{WalletError, WalletManager};
use key_wallet_manager::wallet_manager::{AccountTypePreference, WalletError, WalletManager};

#[test]
fn test_wallet_manager_creation() {
Expand Down
1 change: 1 addition & 0 deletions key-wallet/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ test-utils = ["dashcore/test-utils"]
internals = { path = "../internals", package = "dashcore-private" }
dashcore_hashes = { path = "../hashes", default-features = false }
dashcore = { path="../dash" }
key-wallet-manager = { path = "../key-wallet-manager", features = ["std"] }
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the key-wallet and key-wallet-manager Cargo.toml files
find . -name "Cargo.toml" -path "*/key-wallet*" | head -20

Repository: dashpay/rust-dashcore

Length of output: 148

🏁 Script executed:

# Read the key-wallet/Cargo.toml file to see the dependency at line 25
if [ -f "key-wallet/Cargo.toml" ]; then
  echo "=== key-wallet/Cargo.toml ===" 
  cat -n key-wallet/Cargo.toml | head -40
fi

Repository: dashpay/rust-dashcore

Length of output: 2548

🏁 Script executed:

# Read the key-wallet-manager/Cargo.toml file to check dependencies
if [ -f "key-wallet-manager/Cargo.toml" ]; then
  echo "=== key-wallet-manager/Cargo.toml ===" 
  cat -n key-wallet-manager/Cargo.toml
fi

Repository: dashpay/rust-dashcore

Length of output: 1737

Circular dependency detected between key-wallet and key-wallet-manager.

Adding key-wallet-manager as a dependency here creates a cycle: key-wallet → key-wallet-manager (line 20) → key-wallet. Cargo will fail to resolve this dependency graph and prevent compilation.

Consider restructuring by either:

  1. Extracting shared types into a separate key-wallet-types crate that both depend on
  2. Moving the functionality that requires cross-crate access into one of the crates
  3. Using traits/interfaces to break the dependency cycle
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@key-wallet/Cargo.toml` at line 25, The Cargo.toml entry adding
key-wallet-manager as a dependency creates a circular crate dependency between
key-wallet and key-wallet-manager; remove that direct dependency and break the
cycle by extracting shared types/interfaces into a new crate (e.g.,
key-wallet-types) that both key-wallet and key-wallet-manager depend on, or move
the implementation that requires cross-crate access into a single crate
(key-wallet or key-wallet-manager) or convert the shared API into trait-based
interfaces to eliminate the mutual dependency; update Cargo.toml to depend on
the new key-wallet-types crate and adjust imports/usages in functions/modules
referencing types from key-wallet-manager (or move those functions into the
owning crate).

secp256k1 = { version = "0.30.0", default-features = false, features = ["hashes", "recovery"] }
bip39 = { version = "2.2.0", default-features = false, features = ["chinese-simplified", "chinese-traditional", "czech", "french", "italian", "japanese", "korean", "portuguese", "spanish", "zeroize"] }
serde = { version = "1.0", default-features = false, features = ["derive"], optional = true }
Expand Down
Loading