Handle errors when sending settlement

[sharafian]

https://github.com/interledgerjs/ilp-connector/blob/master/src/middlewares/balance.ts#L270

Currently, when there's an error in sendMoney, the balance middleware ignores it. This makes sense in a lot of cases; if a malicious peer always rejected our settlements despite the fact that we sent them payment channel claims, they could submit the claims and also convince us that we still owe them.

There are also situations where sendMoney may legitimately fail. Maybe the plugin is in a disconnected state, or maybe it has exceeded the maximum balance of a payment channel. Maybe their on-ledger account is out of money.

In these scenarios, we shouldn't apply the settlement to the balance middleware and should instead reverse it. The current behavior doesn't allow for this, however, and so your view of the balance will be incorrect.

It would be useful if there were a MoneyNotSentError that could be thrown by plugins to indicate when a sendMoney error should roll back the settlement. This could also be added to the LPIv2 specification.

[adrianhopebailie]

This makes sense in a lot of cases; if a malicious peer always rejected our settlements despite the fact that we sent them payment channel claims, they could submit the claims and also convince us that we still owe them.

I don't think this is correct. The balance middleware should be very simple. When it sends a sendMoney to the plugin it should adjust the account balance as soon as it is sure that the plugin got the message.

Processing of the sendMoney by the plugin shouldn't impact this unless the plugin reports an error that prevents it from processing the instruction (like insufficient funds).

If a plugin receives a sendMoney it should handle persistence of that instruction and deal with errors from peers (i.e. reconciliation differences). The connector middleware should have no concept of claims.

[sharafian]

I'm not saying that the connector middleware should have any concept of claims; I'm just saying that we should use an error code to distinguish error cases where money was sent but an error occurred in processing from error cases where no money was sent and it's safe to roll back

[adrianhopebailie]

My point is I don't think it is safe to try and distinguish between these.
The process should be synchronous and go like this:

• Connector sends sendMoney to Plugin
• Plugin acks sendMoney
• Connector applies changes to balance

The plugin should keep trying to execute on the 'sendMoney' until it succeeds (it goes in a SAF queue). Until this time, the plugin and the connector will have different views of the account balances.

If the plugin is out of sync with the connector on what the balance is, because the connector thinks it has more liquidity in the account than it actually does, then the plugin must reject payments until this is resolved manually.

[adrianhopebailie]

This issue goes away if we move the balance logic out of the connector.

[sharafian]

I don't think we should be moving the balance logic out of the connector. It's already optional so there's no advantage to removing it and forcing all existing plugins to implement the same logic internally.

[kincaidoneil]

While new plugins should probably implement their own internal balance logic, one idea as a non-breaking change for old plugins would be for sendMoney to return the amount of money leftover from the settlement. e.g. if it successfully sent the entire sum, it could return 0; if the settlement failed and it knows no money was sent, it'd return the original amount; or if it sent a partial amount, it'd return however much was not sent. The return type could change from Promise<void> to Promise<string>, so it wouldn't break existing plugins that don't implement it. The flow would be:

1. Connector applies entire amount to the balance.
2. Connector invokes sendMoney on the plugin with that amount.
3. Connector refunds the leftover amount to the balance, based on how much was returned.

While this doesn't affect our work on eth or lightning since all balance is internal, I generally agree with Ben that there are cases where the plugin knows it wasn't able to the send the money, and that should be somehow reflected in the balance.