add per_deposit_minimum validation

[Jiloc]

Description

Add support for the per_deposit_minimum limit from Emily

Changes

• Retrieve the new values from Emily
• Validate both in the coordinator and the signers that the amount is above the minimum

Testing Information

• Add unit tests

Checklist:

• I have performed a self-review of my code
• My changes generate no new warnings
• New and existing unit tests pass locally with my changes

[djordon]

This largely looks good, but I don't think that we should have withdrawal minimums. Well, I don't think that they should be configurable minimums, but instead be the same minimums enforced on bitcoin.

For enforcing the deposit minimums, we should just refactor the code in the utxo module to isolate the limits logic in a proper function which gets used in the filter closure. Doing so will make it easier to understand and test.

Edit: I saw after the fact the withdrawal minimums are just handling Emily responses.

[djordon]

I think that this needs to be req.max_fee.min(amount) - minimum_deposit_fee >= per_deposit_minimum

[djordon]

Actually, the above is incorrect, let me explain. There needs to be two checks:

1. We need a req.amount >= per_deposit_minimum check,
2. We need a new eq.max_fee.min(amount) - minimum_deposit_fee > 546 check.

Check (2) is fixing a bug that we have here. The clarity smart contracts have a check that the amounts are greater than a DUST_LIMIT constant which is 546. So we need to do a filter here as well.

[djordon]

Actually, we don't need to fix the dust limit thing here since it's a bit more involved. I opened #1135 for this.

[djordon]

Can we break these out over two lines, for readability?

let satisfies_limits = is_fee_valid
    && is_above_per_deposit_minimum
    && is_within_per_deposit_cap
    && is_within_max_mintable_cap;

if satisfies_limits {
    ...

[Jiloc]

changed in b813d20

[djordon]

I think it would be better to put these two new parameters, or a SbtcLimits struct, into the DepositReportErrorMapping struct. It's one of those things that affects validation, so it makes sense to put it there.

We could also add a function for creating SbtcLimits from those parameters that is only available in tests.

[Jiloc]

makes lots of sense, changed in b813d20

[djordon]

Why have this here? I mean, I don't think that we should have a withdrawal minimum that is different from the limits that bitcoin already imposes.

So, I think that we need withdrawal minimum limits, but it shouldn't be part of the sBTC limits struct. Instead we should have some constant that is taken from bitcoin-core for these minimums.

[Jiloc]

removed in b813d20

[djordon]

nit: it's much easier to read if we just split this out like:

let limits = SbtcLimits::new(
    None,
    Some(Amount::from_sat(per_deposit_minimum)),
    Some(Amount::from_sat(per_deposit_cap)),
    None,
    None,
    None,
);
let status = mapping
    .report
    .validate(mapping.chain_tip_height, &tx, TX_FEE, limits);

or if we add a test helper implementation, we could do:

#[test_case(DepositReportErrorMapping {
    report: DepositRequestReport { ... },
    status: InputValidationResult::AmountTooLow,
    chain_tip_height: 2,
    sbtc_limits: SbtcLimits::per_deposit(100_000_000, u64::MAX),
} ; "amount-too-low")]
fn deposit_report_validation(mapping: DepositReportErrorMapping) {
    ...
    let status = mapping
        .report
        .validate(mapping.chain_tip_height, &tx, TX_FEE, &mapping.sbtc_limits:);

[Jiloc]

now the limits are included in the DepositReportErrorMapping b813d20

[djordon]

I think that this closure has gotten complex enough where we should break it out into a separate function or create some struct with some function. I feel that doing so would be much easier to understand and test.

[Jiloc]

Done in 555ffba