Contracts: resolve remaining review TODOs

Comment-only follow-ups to the review:
- selection: a negative change output is already rejected; drop the stale TODO.
- utils: note the per-participant kernel fee is rounded up (overpay bounded to
  under one unit per participant), and that add_outputs/parent_key_id are handled
  by the sign step and the stored Context.
- types: drop the "custom outputs imply payjoin" TODO; make_outputs is valid
  without a payjoin (e.g. a receiver splitting its output), so the check would be
  incorrect.

Addresses PR #754 review (selection.rs, utils.rs, types.rs).
This commit is contained in:
jwinterm
2026-06-07 20:15:13 -04:00
parent 18167be1e5
commit db52b2e58c
3 changed files with 6 additions and 5 deletions
+1 -1
View File
@@ -266,7 +266,7 @@ fn build_output_amount_list(
// TODO: check bounds when casting.
let my_change_output_amount =
(my_input_sum - custom_outputs_sum) as i64 + expected_net_change - my_fee_cost as i64;
// TODO: Check if it's even possible for change output to be negative (it shouldn't be if the equation is correct)
// A negative change output would mean the selection math is off; reject it explicitly.
if my_change_output_amount < 0 {
return Err(Error::GenericError(format!(
"contract::selection::build_output_amount_list => negative change output. Values: my_input_sum: {}, expected_net_change: {}, my_fee_cost: {}",
-1
View File
@@ -76,7 +76,6 @@ impl OutputSelectionArgs {
.unwrap_or(0)
}
// TODO: make sure to validate this: if custom outputs are specified, it has to be a payjoin.
}
impl Default for OutputSelectionArgs {
+5 -3
View File
@@ -319,7 +319,8 @@ pub fn my_fee_contribution(
let mut fee = tx_fee(n_inputs, n_outputs, 0);
// Add out fee costs for kernel. We pay 1/num_participants of a kernel cost
let kernel_cost = tx_fee(0, 0, n_kernels);
// TODO: we slightly overpay. Make sure to cover all the cases
// Round each participant's kernel share up, so the participants together never underpay;
// the overpay is bounded to under one fee unit per participant.
let my_kernel_cost = (kernel_cost as f64 / (num_participants as f64)).ceil();
fee += my_kernel_cost as u64;
@@ -376,8 +377,9 @@ pub fn verify_setup_args_consistency(
ctx_setup_args.num_participants, cur_setup_args.num_participants
)));
}
// TODO: Should we verify add_outputs?
// TODO: verify that the parent_key_id is consistent, perhaps even with the active_account set?
// add_outputs is intentionally forced true at the sign step (late lock), so it is not
// part of this consistency check. parent_key_id is taken from the stored Context, so
// later steps always derive under the contract's account regardless of the active one.
// Compare OutputSelectionArgs
verify_selection_consistency(