Repository: hyperledger/quilt (java)

Issue:
STREAM Sender: Compute Sender Address Properly
Today the Java STREAM sender encountered issues with Rafiki while sending from XRP (scale=9) to USD (scale=6). The payments would work until the amount of units exceeded 11,000 (XRP, scale=9) in which case the JS receiver would send F99's until the Java sender timed out.

Oddly, this behavior was not found when using the JS sender. In the process of debugging this issue, it seems there are a few bugs in both the JS and Java code bases:

From Kincaid:

1. The async operations in after .on('connection', ...) prevented registering the event handler for incoming streams before the first stream event was emitted, so setReceiveMax was never called for that stream (and it defaults to 0). So, the JS STREAM server would reject all packets where it received > 0. I attribute this largely to the JS server exposing a complex/unintuitive API.

1. Separately from this, when the JS STREAM server gets a new connection, it initiates a rate probe back to the client. If it can't determine the exchange rate, is closes the connection. This needs to be fixed so servers that are only receiving money don't initiate this probe.

1. Lastly, that rate probe failed due to an "R00 packet expired" error on the Java side. I'm guessing (?) this is because there was nothing to handle the packets sent back to the source address, test.xpring-dev.jc.test. Even if it can't receive incoming money, it seems weird to advertise a source address that can't receive packets. For the Java sender, it might be nice to respond to these incoming packets with an F99 or more descriptive error message. (Even better, if it returned an F99 with a STREAM response packet including the amount it received, the rate probe may have succeeded).

This last point, in the Java code-base, should be addressed. Technically the StreamSender should either not advertise a source address, or it should compute it properly in the same manner that the SPSP server would, except using the Stream shared-secret.

The offending code is [here](https://github.com/hyperledger/quilt/blob/d3d4aa7e9fa70eb773d658861d1899dd356be74a/stream-parent/stream-client/src/main/java/org/interledger/stream/sender/SimpleStreamSender.java#L449). 

Pull request description:
Send Correct Sender Address in STREAM Payment (fixes #445)

Patch:
diff --git a/examples-parent/send-money/src/main/java/org/interledger/examples/SendMoneyExample.java b/examples-parent/send-money/src/main/java/org/interledger/examples/SendMoneyExample.java
index 251dba3..af67289 100644
--- a/examples-parent/send-money/src/main/java/org/interledger/examples/SendMoneyExample.java
+++ b/examples-parent/send-money/src/main/java/org/interledger/examples/SendMoneyExample.java
@@ -52,14 +52,8 @@ public class SendMoneyExample {
   private static final String TESTNET_URI =
       "https://prod.wc.wallet.xpring.io/accounts/" + SENDER_ACCOUNT_USERNAME + "/ilp";
 
-  /**
-   * This value will go away once #445 is fixed.
-   *
-   * @see "https://github.com/hyperledger/quilt/issues/445"
-   */
-  @Deprecated
-  private static final InterledgerAddress SENDER_ADDRESS =
-      InterledgerAddress.of("test.xpring-dev.wc.prod.spsp").with(SENDER_ACCOUNT_USERNAME);
+  private static final InterledgerAddress OPERATOR_ADDRESS =
+      InterledgerAddress.of("private.org.interledger.examples.sendmoneyexample").with(SENDER_ACCOUNT_USERNAME);
 
   public static void main(String[] args) throws ExecutionException, InterruptedException {
     SpspClient spspClient = new SimpleSpspClient();
@@ -80,7 +74,7 @@ public class SendMoneyExample {
     // Send payment using STREAM
     SendMoneyResult result = simpleStreamSender.sendMoney(
         SendMoneyRequest.builder()
-            .sourceAddress(SENDER_ADDRESS)
+            // No source address because the client/sender is not routable (i.e., can't receive).
             .amount(UnsignedLong.valueOf(ONE_DROP_IN_SCALE_9))
             .denomination(Denominations.XRP_MILLI_DROPS)
             .destinationAddress(connectionDetails.destinationAddress())
@@ -95,7 +89,7 @@ public class SendMoneyExample {
 
   private static Link newIlpOverHttpLink() {
     return new IlpOverHttpLink(
-        () -> SENDER_ADDRESS,
+        () -> OPERATOR_ADDRESS,
         HttpUrl.parse(TESTNET_URI),
         newHttpClient(),
         createObjectMapperForProblemsJson(),
diff --git a/pom.xml b/pom.xml
index 337ee1c..5804437 100644
--- a/pom.xml
+++ b/pom.xml
@@ -287,7 +287,7 @@
       <dependency>
         <groupId>org.immutables</groupId>
         <artifactId>value</artifactId>
-        <version>2.7.5</version>
+        <version>2.8.3</version>
         <scope>provided</scope>
       </dependency>
       <dependency>
diff --git a/stream-parent/stream-client/src/main/java/org/interledger/stream/sender/SimpleStreamSender.java b/stream-parent/stream-client/src/main/java/org/interledger/stream/sender/SimpleStreamSender.java
index 698af65..2a273d1 100644
--- a/stream-parent/stream-client/src/main/java/org/interledger/stream/sender/SimpleStreamSender.java
+++ b/stream-parent/stream-client/src/main/java/org/interledger/stream/sender/SimpleStreamSender.java
@@ -266,7 +266,7 @@ public class SimpleStreamSender implements StreamSender {
     private final Link link;
     private final SharedSecret sharedSecret;
     private final Optional<Duration> timeout;
-    private final InterledgerAddress senderAddress;
+    private final Optional<InterledgerAddress> senderAddress;
     private final Denomination senderDenomination;
     private final InterledgerAddress destinationAddress;
     private final AtomicBoolean shouldSendSourceAddress;
@@ -366,6 +366,8 @@ public class SimpleStreamSender implements StreamSender {
             // Do all the work of sending packetized money for this Stream/sendMoney request.
             this.sendMoneyPacketized();
             return SendMoneyResult.builder()
+                .senderAddress(this.computeSenderAddressForReportingPurposes())
+                .destinationAddress(destinationAddress)
                 .amountDelivered(paymentTracker.getDeliveredAmountInReceiverUnits())
                 .amountSent(paymentTracker.getDeliveredAmountInSenderUnits())
                 .amountLeftToSend(paymentTracker.getOriginalAmountLeft())
@@ -397,6 +399,8 @@ public class SimpleStreamSender implements StreamSender {
     private SendMoneyResult constructSendMoneyResultForInvalidPreflight(final Instant startPreflight) {
       Objects.requireNonNull(startPreflight);
       return SendMoneyResult.builder()
+          .senderAddress(this.computeSenderAddressForReportingPurposes())
+          .destinationAddress(destinationAddress)
           .sendMoneyDuration(Duration.between(startPreflight, DateUtils.now()))
           .numRejectPackets(0)
           .numFulfilledPackets(0)
@@ -408,6 +412,17 @@ public class SimpleStreamSender implements StreamSender {
           .build();
     }
 
+    /**
+     * Helper method to centralize the computation of this sener's {@link InterledgerAddress}. Basically, if {@link
+     * #senderAddress} is empty, then the implementation will report back the link address. However, during STREAM
+     * operation with a receiver, no address will be reported (e.g., such as inside of a `ConnectionNewAddress` frame).
+     *
+     * @return The {@link InterledgerAddress} of this sender, for usage by the creator of this stream sender.
+     */
+    private InterledgerAddress computeSenderAddressForReportingPurposes() {
+      return this.senderAddress.orElseGet((link.getOperatorAddressSupplier()));
+    }
+
     /**
      * <p>Send a zero-value Prepare packet to "pre-flight" the Connection before actual value is transferred.</p>
      *
@@ -446,6 +461,13 @@ public class SimpleStreamSender implements StreamSender {
               .streamId(UnsignedLong.ONE)
               .shares(UnsignedLong.ONE)
               .build(),
+          // Always send a ConnectionNewAddress frame (sometimes with an empty address) in order to trigger the receiver
+          // on the other side of this STREAM to send us our ConnectionAssetDetails. This is odd behavior, but the
+          // RFC authors can't agree on a standardization in IL-RFC-29. So until then, we have this dirty little secret
+          // in every implementation that just does this. Read more in the following issues:
+          // https://github.com/interledger/rfcs/issues/554
+          // https://github.com/interledger/rfcs/issues/571
+          // https://github.com/interledger/rfcs/pull/573
           ConnectionNewAddressFrame.builder()
               .sourceAddress(senderAddress)
               .build(),
@@ -494,8 +516,8 @@ public class SimpleStreamSender implements StreamSender {
               }, // Do nothing on fulfill
               this::checkForAndTriggerUnrecoverableError // check for unrecoverable error.
           )
-          // We typically expect this Prepare operation to reject, but regardless of whether the response is a fulfill or a
-          // reject, try to read the Receiver's Connection Asset Details and return them.
+          // We typically expect this Prepare operation to reject, but regardless of whether the response is a fulfill
+          // or a reject, try to read the Receiver's Connection Asset Details and return them.
           .map(readDetailsFromStream::apply, readDetailsFromStream::apply);
     }
 
diff --git a/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyRequest.java b/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyRequest.java
index 4906fcc..d7db30a 100644
--- a/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyRequest.java
+++ b/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyRequest.java
@@ -38,11 +38,13 @@ public interface SendMoneyRequest {
   SharedSecret sharedSecret();
 
   /**
-   * The source address of this request.
+   * The optionally-supplied source address of this request. Senders are not required to send a source ILP address,
+   * especially to support scenarios where the sender is not routable (e.g., client making sendMoney requests using an
+   * ILP-over-HTTP link that has no incoming URL, such as from an Android device).
    *
-   * @return The {@link InterledgerAddress} of the source of this payment.
+   * @return The optionally-present {@link InterledgerAddress} of the source of this payment.
    */
-  InterledgerAddress sourceAddress();
+  Optional<InterledgerAddress> sourceAddress();
 
   /**
    * The destination address of this request.
diff --git a/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyResult.java b/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyResult.java
index cef9aaa..6ed48fc 100644
--- a/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyResult.java
+++ b/stream-parent/stream-core/src/main/java/org/interledger/stream/SendMoneyResult.java
@@ -1,11 +1,12 @@
 package org.interledger.stream;
 
 import org.interledger.core.Immutable;
+import org.interledger.core.InterledgerAddress;
 
 import com.google.common.primitives.UnsignedLong;
-import org.immutables.value.Value;
 
 import java.time.Duration;
+import java.util.Optional;
 
 /**
  * The result of a request to send money using the STREAM protocol.
@@ -17,6 +18,20 @@ public interface SendMoneyResult {
     return new SendMoneyResultBuilder();
   }
 
+  /**
+   * The ILP address of the sender, if present.
+   *
+   * @return The optional {@link InterledgerAddress} of the Receiver.
+   */
+  Optional<InterledgerAddress> senderAddress();
+
+  /**
+   * The ILP address of the receiver.
+   *
+   * @return The {@link InterledgerAddress} of the Receiver.
+   */
+  InterledgerAddress destinationAddress();
+
   /**
    * The original amount that was requested to be sent.
    *


Test patch:
diff --git a/stream-parent/stream-client/src/test/java/org/interledger/stream/sender/SendMoneyAggregatorTest.java b/stream-parent/stream-client/src/test/java/org/interledger/stream/sender/SendMoneyAggregatorTest.java
index 36f090a..a7a6e1f 100644
--- a/stream-parent/stream-client/src/test/java/org/interledger/stream/sender/SendMoneyAggregatorTest.java
+++ b/stream-parent/stream-client/src/test/java/org/interledger/stream/sender/SendMoneyAggregatorTest.java
@@ -207,6 +207,7 @@ public class SendMoneyAggregatorTest {
     sendMoneyAggregator.send().get();
 
     // Expect 0 link calls
+    Mockito.verify(linkMock).getOperatorAddressSupplier();
     Mockito.verifyNoMoreInteractions(linkMock);
   }
 
diff --git a/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyRequestTest.java b/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyRequestTest.java
new file mode 100644
index 0000000..acda4ee
--- /dev/null
+++ b/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyRequestTest.java
@@ -0,0 +1,78 @@
+package org.interledger.stream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.interledger.core.InterledgerAddress;
+import org.interledger.core.SharedSecret;
+
+import com.google.common.primitives.UnsignedLong;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import java.time.Duration;
+import java.util.UUID;
+
+/**
+ * Unit tests for {@link SendMoneyRequest}.
+ */
+public class SendMoneyRequestTest {
+
+  private static final UUID RANDOM_UUID = UUID.randomUUID();
+  private static final Denomination DENOMINATION = Denomination.builder().assetCode("USD").assetScale((short) 2)
+      .build();
+  private static final SharedSecret SHARED_SECRET = SharedSecret.of(new byte[32]);
+
+  @Mock
+  private PaymentTracker paymentTrackerMock;
+
+  @Before
+  public void setUp() {
+    MockitoAnnotations.initMocks(this);
+  }
+
+  @Test
+  public void testBuilderWithoutOptionals() {
+    SendMoneyRequest result = SendMoneyRequest.builder()
+        .amount(UnsignedLong.valueOf(10))
+        .destinationAddress(InterledgerAddress.of("example.destination"))
+        .sharedSecret(SHARED_SECRET)
+        .denomination(DENOMINATION)
+        .requestId(RANDOM_UUID)
+        .paymentTracker(paymentTrackerMock)
+        .build();
+
+    assertThat(result.sourceAddress()).isEmpty();
+    assertThat(result.amount()).isEqualTo(UnsignedLong.valueOf(10));
+    assertThat(result.destinationAddress()).isEqualTo(InterledgerAddress.of("example.destination"));
+    assertThat(result.sharedSecret()).isEqualTo(SHARED_SECRET);
+    assertThat(result.denomination()).isEqualTo(DENOMINATION);
+    assertThat(result.timeout()).isEmpty();
+    assertThat(result.requestId()).isEqualTo(RANDOM_UUID);
+    assertThat(result.paymentTracker()).isEqualTo(paymentTrackerMock);
+  }
+
+  @Test
+  public void testBuilderWithOptionals() {
+    SendMoneyRequest result = SendMoneyRequest.builder()
+        .sourceAddress(InterledgerAddress.of("example.sender"))
+        .amount(UnsignedLong.valueOf(10))
+        .destinationAddress(InterledgerAddress.of("example.destination"))
+        .sharedSecret(SHARED_SECRET)
+        .denomination(DENOMINATION)
+        .timeout(Duration.ZERO)
+        .requestId(RANDOM_UUID)
+        .paymentTracker(paymentTrackerMock)
+        .build();
+
+    assertThat(result.sourceAddress().get()).isEqualTo(InterledgerAddress.of("example.sender"));
+    assertThat(result.amount()).isEqualTo(UnsignedLong.valueOf(10));
+    assertThat(result.destinationAddress()).isEqualTo(InterledgerAddress.of("example.destination"));
+    assertThat(result.sharedSecret()).isEqualTo(SHARED_SECRET);
+    assertThat(result.denomination()).isEqualTo(DENOMINATION);
+    assertThat(result.timeout().get()).isEqualTo(Duration.ZERO);
+    assertThat(result.requestId()).isEqualTo(RANDOM_UUID);
+    assertThat(result.paymentTracker()).isEqualTo(paymentTrackerMock);
+  }
+}
diff --git a/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyResultTest.java b/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyResultTest.java
index 4336eca..bf6ace3 100644
--- a/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyResultTest.java
+++ b/stream-parent/stream-core/src/test/java/org/interledger/stream/SendMoneyResultTest.java
@@ -1,7 +1,10 @@
 package org.interledger.stream;
 
+import static com.google.common.primitives.UnsignedLong.ZERO;
 import static org.assertj.core.api.Assertions.assertThat;
 
+import org.interledger.core.InterledgerAddress;
+
 import com.google.common.primitives.UnsignedLong;
 import org.junit.Test;
 
@@ -13,17 +16,55 @@ import java.time.Duration;
 public class SendMoneyResultTest {
 
   @Test
-  public void totalPackets() {
+  public void testBuilderWithNoOptionals() {
     SendMoneyResult result = SendMoneyResult.builder()
-        .originalAmount(UnsignedLong.ZERO)
-        .amountDelivered(UnsignedLong.ZERO)
-        .amountSent(UnsignedLong.ZERO)
-        .amountLeftToSend(UnsignedLong.ZERO)
+        .destinationAddress(InterledgerAddress.of("example.destination"))
+        .originalAmount(ZERO)
+        .amountDelivered(UnsignedLong.ONE)
+        .amountSent(UnsignedLong.valueOf(2))
+        .amountLeftToSend(UnsignedLong.valueOf(3))
         .numFulfilledPackets(10)
         .numRejectPackets(5)
-        .sendMoneyDuration(Duration.ZERO)
+        .sendMoneyDuration(Duration.ofMillis(1000))
         .successfulPayment(true)
         .build();
+    assertThat(result.senderAddress()).isEmpty();
+    assertThat(result.destinationAddress()).isEqualTo(InterledgerAddress.of("example.destination"));
+    assertThat(result.originalAmount()).isEqualTo(ZERO);
+    assertThat(result.amountDelivered()).isEqualTo(UnsignedLong.ONE);
+    assertThat(result.amountSent()).isEqualTo(UnsignedLong.valueOf(2));
+    assertThat(result.amountLeftToSend()).isEqualTo(UnsignedLong.valueOf(3));
+    assertThat(result.numFulfilledPackets()).isEqualTo(10);
+    assertThat(result.numRejectPackets()).isEqualTo(5);
+    assertThat(result.totalPackets()).isEqualTo(15);
+    assertThat(result.sendMoneyDuration()).isEqualTo(Duration.ofMillis(1000));
+    assertThat(result.successfulPayment()).isTrue();
+  }
+
+  @Test
+  public void testBuilderWithOptionals() {
+    SendMoneyResult result = SendMoneyResult.builder()
+        .senderAddress(InterledgerAddress.of("example.sender"))
+        .destinationAddress(InterledgerAddress.of("example.destination"))
+        .originalAmount(ZERO)
+        .amountDelivered(UnsignedLong.ONE)
+        .amountSent(UnsignedLong.valueOf(2))
+        .amountLeftToSend(UnsignedLong.valueOf(3))
+        .numFulfilledPackets(10)
+        .numRejectPackets(5)
+        .sendMoneyDuration(Duration.ofMillis(1000))
+        .successfulPayment(false)
+        .build();
+    assertThat(result.senderAddress().get()).isEqualTo(InterledgerAddress.of("example.sender"));
+    assertThat(result.destinationAddress()).isEqualTo(InterledgerAddress.of("example.destination"));
+    assertThat(result.originalAmount()).isEqualTo(ZERO);
+    assertThat(result.amountDelivered()).isEqualTo(UnsignedLong.ONE);
+    assertThat(result.amountSent()).isEqualTo(UnsignedLong.valueOf(2));
+    assertThat(result.amountLeftToSend()).isEqualTo(UnsignedLong.valueOf(3));
+    assertThat(result.numFulfilledPackets()).isEqualTo(10);
+    assertThat(result.numRejectPackets()).isEqualTo(5);
     assertThat(result.totalPackets()).isEqualTo(15);
+    assertThat(result.sendMoneyDuration()).isEqualTo(Duration.ofMillis(1000));
+    assertThat(result.successfulPayment()).isFalse();
   }
 }