hyperledger-archives / quilt

Hyperledger Quilt - An implementation of the Interledger Protocol
https://wiki.hyperledger.org/display/quilt
Apache License 2.0
218 stars 79 forks source link

Fix Bugs in `SimpleStreamSender` #286

Closed sappenin closed 5 years ago

sappenin commented 5 years ago

The current implementation of SimpleStream Client has a few problems:

  1. Payment Timeout. The sendMoney call should be overridden with a sibling variant that accepts a timeout. If not specified, the sender should never timeout. However, if specified, the sender should timeout [FIXED BY #290]

  2. Refactor SendMoneyAggregator: [FIXED BY #290]

  3. Exchange Rates: [FIXED BY #291]

  4. While-loop condition: [FIXED BY #290] This while-loop condition is technically correct, but is confusing. Instead, it should probably look like this:

// Used to count the total number of STREAM packets send for this payment
final AtomicInteger totalStreamPackets = new AtomicInteger(0);
// Used to track the duration of this STREAM payment.
final Instant start = Instant.now();
while (true) {
    if (amountLeftToSend <= 0)) {
          break;
        }
   //... Keep sending packets.
}

4b. TODO: add a Preconditon that amountLeftToSend is NEVER < 0, and "throw" if this ever happens. [FIXED BY #293]

  1. Sleeping Thread: [FIXED BY #290] If the amountToSend is ZERO, the sender sleeps for 100ms and then continues the loop. However, we should consider just breaking out here because this is what occurs now, but in a less obvious way. That said, we should tighten the while-loop to not stop until deliveredAmount is actually the value we want. In other words, we should keep looping until either deliveredAmount is enough, or the timeout is triggered. We need to think carefully though about not overpaying.

  2. ILPv4 Packet Failures: [FIXED BY #289] We need to consider what should happen in the STREAM client when certain ILPv4 errors are encountered. Right now, only T04 and F08 are handled, with all other errors simply being retried. This is probably fine, but we should look through the JS Rejection logic and the Rust Rejection logic to be sure we're doing this correctly.

sappenin commented 5 years ago

Java ILP Call Notes

  1. Consider timeout on sendMoney to be from the point of last fulfill packet. JS is 60s (idle timeout). Timeout increases should be done on fulfills.

  2. Allow sender to indicate which mode:

    1. Fixed sender amount (whatever arrives at receiver arrives). Sender can still tracks rates and stop the STREAM over some FX threshold. Have recipient calculate their rate threshold.
    2. Fixed Receiver Amount (Invoice amount):
    3. Receiver rate calculation?
  3. T04 should not affect any "time backoffs". All other Txx errors should back-off in time (maybe AIMD) --> v1.1

sappenin commented 5 years ago

Closing this issue since new issues have been created for each portion of this issue. See initial description above for more details.