second review

[crusso]

(No longer sure this builds as my dfx got corrupted). Is CI working?

Some questions spring to mind:

• why not delete invoices that have been verified, perhaps after a delay?
• If you never delete them, perhaps consider storing them in stable memory so upgrades are simple. If you make the representation fixed size, you can just allocated them fairly easily.
• Any awaits can fail with an error - the code will propagate the error to the caller of the method that failed. Is that the behaviour you want, or not?
• The code that checks the balance and rejects an invoice seems racy to me. The balance can change after the check, can it not? Can that cause a problem?

[krpeacock]

1. Dfx 0.9.0 was always a corrupted release and shouldn't be used. There is no CI yet
2. I don't want to delete invoices, because a default behavior of deleting records of financial transactions does not seem like a best/desirable practice to me
3. I'm not sure what the fixed size would be - maybe we can discuss in a dedicated issue
4. For failed awaits - this would be for the ICP.transfer scenario, right? Would wrapping those in a try/catch resolve this behavior?
5. The verifyInvoice is meant to be pollable - all we care is that the invoice is eventually satisfied.
   • overpaying is not a significant concern because we can allow the invoice creator to handle that logic themselves
   • Additional payments sent to a closed invoice for whatever reason could be a problem.
   • I would consider adding some new method to reclaim surplus balance from an invoice account, but only allow it to be called after the invoice has been verified. Otherwise, it could be abused to prevent the invoice from being satisfied