fix(api): idempotency bug in registerBroadcastHandler

[vindard]

Description

In PR #3550, by fixing the idempotency issue where an InvalidLedgerTransactionStateError is thrown if the handler is re-run and records a reconciliation transaction (see traces), we introduced a new bug where multiple reconciliation transaction are created on re-run.

This PR cleans up the fix from #3550 by introducing an explicit isOnChainFeeReconciliationTxn check and it fixes the new multiple-reconciliation-txns bug.

[dolcalmi]

not sure when we began to use non undefinied/null/empty validation (should be !txn.address we already had an issue in prod with this) also.. should not include bankowner wallet id validation?

[vindard]

In this case I was thinking it should specifically check for the absence of the address property (since the OnChainAddress can technically also be falsy/empty even though it should never be). Or, what sort of issue has this thrown in prod?

should not include bankowner wallet id validation

Maybe not necessary since the reconciliation transaction only ever involves Liabilities:bankOwner account and Asset:OnChain accounts and I believe we only bring in Liabilities accounts in our ledger get methods. address absence should be enough for this check?

Sidenotes:
For this check, ideally it should be a different tx type value instead of also being an onchain_payment so we don't even need the check, but that might be a tricky migration to do. And I also considered maybe setting a custom description for these types of transactions but then there was no way to guarantee that someone couldn't set the same description on normal onchain-payment transactions which would break the heuristic. I switched from checking for satsFee presence to address presence since this is more isolated and might less likely eventually be populated on the reconciliation-type transactions that get constructed inside FeeOnlyEntryBuilder.

[dolcalmi]

at least with validations in data from external services we must be more "defensive", we cant guarantee the undefined so it is safer just to use !txn.address

FYI: the error was a validation using undefined and a service returning null value (typescript in theory was ok but you know we cant trust TS )

[vindard]

Oh yea ok that makes sense.

For here though, address is set explicitly in our translateToLedgerTx function, so there's also less risk of it being something unexpected? We handle the uncertain-falsy-check-from-external-service in the line tx.payee_addresses && tx.payee_addresses.length > 0 inside translateToLedgerTx

[dolcalmi]

can reconciliation be applied to other wallets (different from bankowner)?

[vindard]

Mm, it shouldn't be since reconciliations only involve Liabilities:bankOwner and Assets:OnChain. Why do you ask?

[dolcalmi]

if that is the case then walletId validation should be in isOnChainFeeReconciliationTxn

[vindard]

Not sure if that would be helpful here though, because we'd still need to do the txn.walletId === bankOwnerWalletId check since normal non-reconciliation onchain_payment txns can also be from user wallet ids

I switched the check order around to this to try to make this check more intuitive:

(txn) => txn.walletId  === bankOwnerWalletId && !isOnChainFeeReconciliationTxn(txn),