Skip to content
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.

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

added tests for calculate_fee(), calculate_waste() and effective_value() #25

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Changes from 4 commits
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
232 changes: 219 additions & 13 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//! A blockchain-agnostic Rust Coinselection library

use rand::{seq::SliceRandom, thread_rng};
use std::matches;

/// A [`OutputGroup`] represents an input candidate for Coinselection. This can either be a
/// single UTXO, or a group of UTXOs that should be spent together.
Expand Down Expand Up @@ -75,6 +76,8 @@ pub enum ExcessStrategy {
pub enum SelectionError {
InsufficientFunds,
NoSolutionFound,
NegativeFeeRate,
Copy link
Collaborator

Choose a reason for hiding this comment

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

NonPositiveFeeRate will be accurate

AbnormallyHighFee,
Copy link
Collaborator

Choose a reason for hiding this comment

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

AbnormallyHighFeeRate

}

/// Calculated waste for a specific selection.
Expand Down Expand Up @@ -143,16 +146,16 @@ pub fn select_coin_lowestlarger(
let target = options.target_value + options.min_drain_value;

let mut sorted_inputs: Vec<_> = inputs.iter().enumerate().collect();
sorted_inputs.sort_by_key(|(_, input)| effective_value(input, options.target_feerate));
sorted_inputs.sort_by_key(|(_, input)| effective_value(input, options.target_feerate).unwrap());
rajarshimaitra marked this conversation as resolved.
Show resolved Hide resolved

let mut index = sorted_inputs.partition_point(|(_, input)| {
input.value <= (target + calculate_fee(input.weight, options.target_feerate))
input.value <= (target + calculate_fee(input.weight, options.target_feerate).unwrap())
});

for (idx, input) in sorted_inputs.iter().take(index).rev() {
accumulated_value += input.value;
accumulated_weight += input.weight;
estimated_fees = calculate_fee(accumulated_weight, options.target_feerate);
estimated_fees = calculate_fee(accumulated_weight, options.target_feerate)?;
selected_inputs.push(*idx);

if accumulated_value >= (target + estimated_fees.max(options.min_absolute_fee)) {
Expand All @@ -164,7 +167,7 @@ pub fn select_coin_lowestlarger(
for (idx, input) in sorted_inputs.iter().skip(index) {
accumulated_value += input.value;
accumulated_weight += input.weight;
estimated_fees = calculate_fee(accumulated_weight, options.target_feerate);
estimated_fees = calculate_fee(accumulated_weight, options.target_feerate)?;
selected_inputs.push(*idx);

if accumulated_value >= (target + estimated_fees.max(options.min_absolute_fee)) {
Expand Down Expand Up @@ -209,7 +212,7 @@ pub fn select_coin_fifo(
sorted_inputs.sort_by_key(|(_, a)| a.creation_sequence);

for (index, inputs) in sorted_inputs {
estimated_fees = calculate_fee(accumulated_weight, options.target_feerate);
estimated_fees = calculate_fee(accumulated_weight, options.target_feerate)?;
if accumulated_value
>= (options.target_value
+ estimated_fees.max(options.min_absolute_fee)
Expand Down Expand Up @@ -268,15 +271,15 @@ pub fn select_coin_srd(

let necessary_target = options.target_value
+ options.min_drain_value
+ calculate_fee(options.base_weight, options.target_feerate);
+ calculate_fee(options.base_weight, options.target_feerate)?;

for (index, input) in randomized_inputs {
selected_inputs.push(index);
accumulated_value += input.value;
accumulated_weight += input.weight;
input_counts += input.input_count;

estimated_fee = calculate_fee(accumulated_weight, options.target_feerate);
estimated_fee = calculate_fee(accumulated_weight, options.target_feerate)?;

if accumulated_value
>= options.target_value
Expand Down Expand Up @@ -337,7 +340,9 @@ fn calculate_waste(
}

if options.excess_strategy != ExcessStrategy::ToDrain {
waste += accumulated_value - options.target_value - estimated_fee;
waste += accumulated_value
.saturating_sub(options.target_value)
.saturating_sub(estimated_fee);
} else {
waste += options.drain_cost;
}
Expand All @@ -346,21 +351,29 @@ fn calculate_waste(
}

#[inline]
fn calculate_fee(weight: u32, rate: f32) -> u64 {
(weight as f32 * rate).ceil() as u64
fn calculate_fee(weight: u32, rate: f32) -> Result<u64, SelectionError> {
if rate <= 0.0 {
Err(SelectionError::NegativeFeeRate)
} else if rate > 1000.0 {
Err(SelectionError::AbnormallyHighFee)
} else {
Ok((weight as f32 * rate).ceil() as u64)
}
}

/// Returns the effective value which is the actual value minus the estimated fee of the OutputGroup
#[inline]
fn effective_value(output: &OutputGroup, feerate: f32) -> u64 {
output
fn effective_value(output: &OutputGroup, feerate: f32) -> Result<u64, SelectionError> {
Ok(output
.value
.saturating_sub(calculate_fee(output.weight, feerate))
.saturating_sub(calculate_fee(output.weight, feerate)?))
Copy link
Contributor

Choose a reason for hiding this comment

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

A better name for this could be output.spending_weight. Weight can imply two things, the weight of the output, or the weight of the spending transaction's input. We need the second weight here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any input on this?

Copy link
Author

Choose a reason for hiding this comment

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

I agree that spending_weight is a better name than weight. Perhaps input_weight or utxo_weight would be other options to consider. Anything to differentiate the two would improve the readability of the code.

}

#[cfg(test)]
mod test {

use std::fmt::Error;

use super::*;

fn setup_basic_output_groups() -> Vec<OutputGroup> {
Expand Down Expand Up @@ -578,4 +591,197 @@ mod test {
let result = select_coin_lowestlarger(&inputs, options);
assert!(matches!(result, Err(SelectionError::InsufficientFunds)));
}

#[test]
fn test_calculate_fee() {
struct TestVector {
weight: u32,
fee: f32,
output: Result<u64, SelectionError>,
}

let test_vectors = [
TestVector {
weight: 60,
fee: 5.0,
output: Ok(300),
},
TestVector {
weight: 60,
fee: -5.0,
output: Err(SelectionError::NegativeFeeRate),
},
TestVector {
weight: 60,
fee: 1001.0,
output: Err(SelectionError::AbnormallyHighFee),
},
TestVector {
weight: 60,
fee: 0.0,
output: Err(SelectionError::NegativeFeeRate),
},
];

for vector in test_vectors {
let result = calculate_fee(vector.weight, vector.fee);
match result {
Ok(val) => {
assert_eq!(val, vector.output.unwrap())
}
Err(err) => {
let output = vector.output.err();
assert!(matches!(err, output));
}
}
}
}

#[test]
fn test_effective_value() {
struct TestVector {
output: OutputGroup,
feerate: f32,
result: Result<u64, SelectionError>,
}

let test_vectors = [
// Value minus weight would be less Than Zero but will return zero because of saturating_subtraction for u64
TestVector {
output: OutputGroup {
value: 100,
weight: 101,
input_count: 1,
is_segwit: false,
creation_sequence: None,
},
feerate: 1.0,
result: Ok(0),
},
// Value greater than zero
TestVector {
output: OutputGroup {
value: 100,
weight: 99,
input_count: 1,
is_segwit: false,
creation_sequence: None,
},
feerate: 1.0,
result: Ok(1),
},
// Test negative fee rate return appropriate error
TestVector {
output: OutputGroup {
value: 100,
weight: 99,
input_count: 1,
is_segwit: false,
creation_sequence: None,
},
feerate: -1.0,
result: Err(SelectionError::NegativeFeeRate),
},
// Test very high fee rate
TestVector {
output: OutputGroup {
value: 100,
weight: 99,
input_count: 1,
is_segwit: false,
creation_sequence: None,
},
feerate: 2000.0,
result: Err(SelectionError::AbnormallyHighFee),
},
// Test high value
TestVector {
output: OutputGroup {
value: 100_000_000_000,
weight: 10,
input_count: 1,
is_segwit: false,
creation_sequence: None,
},
feerate: 1.0,
result: Ok(999_99_999_990),
},
];

for vector in test_vectors {
let effective_value = effective_value(&vector.output, vector.feerate);

match effective_value {
Ok(val) => {
assert_eq!(val, vector.result.unwrap())
}
Err(err) => {
let output = vector.result.err();
assert!(matches!(err, output));
}
}
}
}

#[test]
fn test_calculate_waste() {
struct TestVector {
options: CoinSelectionOpt,
accumulated_value: u64,
accumulated_weight: u32,
estimated_fee: u64,
result: u64,
}

let inputs = setup_basic_output_groups();
let options = setup_options(100);
let selection_output = select_coin_lowestlarger(&inputs, options).unwrap();

let test_vectors = [
// Test for excess srategy to drain(change output)
TestVector {
options,
accumulated_value: 1000,
accumulated_weight: 50,
estimated_fee: 20,
result: options.drain_cost,
},
// Test for excess srategy to miners
TestVector {
options: CoinSelectionOpt {
excess_strategy: ExcessStrategy::ToFee,
..options
},
accumulated_value: 1000,
accumulated_weight: 50,
estimated_fee: 20,
result: 880,
},
// Test accumulated_value minus target_value < 0
TestVector {
options: CoinSelectionOpt {
target_value: 1000,
excess_strategy: ExcessStrategy::ToFee,
..options
},
accumulated_value: 200,
accumulated_weight: 50,
estimated_fee: 20,
result: 0,
},
];

for vector in test_vectors {
let waste = calculate_waste(
&inputs,
&selection_output.selected_inputs,
&vector.options,
vector.accumulated_value,
vector.accumulated_weight,
vector.estimated_fee,
);

assert_eq!(waste, vector.result)
}
}
}
Loading