fix: greedy forwarding

[michielbdejong]

When the incoming amount is higher than expected, don't forward this difference on to the next connector; instead, always pay as little as possible (based on the curve from next to final).

cc @emschwartz @justmoon

[michielbdejong]

Working in a companion in integration tests: https://circleci.com/gh/interledgerjs/five-bells-integration-test/526

[codecov-io]

Codecov Report

Merging #392 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
- Coverage   83.38%   83.37%   -0.02%     
==========================================
  Files          34       34              
  Lines        1210     1209       -1     
  Branches      199      199              
==========================================
- Hits         1009     1008       -1     
  Misses        201      201

Impacted Files	Coverage Δ
src/lib/route-builder.js	93.87% <100%> (-0.19%)	⬇️
src/lib/quoter.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 734b61c...886dbdd. Read the comment docs.

[sentientwaffle]

Does this patch resolve interledger/rfcs#316?

[sentientwaffle]

Is findBestPathForSourceAmount still used or was this the only call? If its the latter, the function should be removed.

comments addressed, now awaiting @emschwartz's confirmation of interledgerjs/five-bells-integration-test#88 (comment)

[emschwartz]

What happens if the connector doesn't have a curve to the destination?

Also, unless there are major objections to this idea: interledger/rfcs#312 (comment) I think we should implement that along with this change.

[michielbdejong]

What happens if the connector doesn't have a curve to the destination?

That doesn't change, it was doing a quote-by-source and with this change will do a quote-by-destination; both will trigger a liquidity-quote if the curve is missing/unknown

I think we should implement [best effort payments] along with this change.

I think best-effort payments are a new feature, unrelated to 'normal' payments; what are the downsides of merging this PR first, and then implementing that new feature in a new PR? Would you feel the master branch would be an incorrect state if we merge this PR now, without adding support for best-effort payments?

[emschwartz]

I think best-effort payments are a new feature, unrelated to 'normal' payments

Fair enough. I made that point because this seems to move in the opposite direction so I'm not a big fan of making it worse (from my perspective, thinking we need to support forwarding without delivery).

Separately, I realized (from interledger/rfcs#312 (comment)) that you might want to specifically handle the 0 amount case because there's a good chance a lot of ledgers won't let you send a 0 amount.

[michielbdejong]

this seems to move in the opposite direction so I'm not a big fan of making it worse

How normal ("type 1") payments are handled, and how best-effort ("type 9") payments are handled, are two entirely separate things, and it would be a bug if you try to change the normal behavior in the direction of the best-effort behavior. I think, even if we add support for best effort payments, we still want to keep support for normal payments, right?

Noted, you are not a big fan of Current Interledger with its three types of quote requests, but surely you're not advocating that we should remove support for it from the code base because of that, right? Even if the ilp-connector code supports it, you will still be free to not use it. :)

Fair enough

So where does that leave us, can this be merged now?

[emschwartz]

Actually, interledger/rfcs#77 was the last agreed upon behavior, so this is arguably just about as much of a departure from that as forwarding all the way to the end.

I think, even if we add support for best effort payments, we still want to keep support for normal payments, right?

I actually think we should (not right now but in the not too distant future) remove support for payments with the amount in the packet in cleartext. Everything you can do with the amount in the packet you can do with it not in there, and there are a bunch of other things you can build on top of ILP without the amount in the packet that you can't do with it in there. So I think it's worse in pretty much every way, except for the fact that it's there now.

[michielbdejong]

@emschwartz on our 1:1 call yesterday we (if I'm wording this correctly) discussed:

• the current combination of forwarding and delivery was not a bug, it was implemented this way on purpose. Having said that, it was never a good idea, and was also never properly documented, so we should probably switch to either 100% forwarding, or 100% delivery (this PR proposes a switch to 100% delivery)
• it's probably a good idea to introduce ILP packet type 9, which has no amount, and therefore will be 100% forwarding by necessity
• even if you do follow a quote, the packet amount would have been implied by the combination of that recent quote response and the transfer amount, so delivery then becomes a special case of "very tight" forwarding, so there's not really a need to ever specify an amount in the packet
• if we're going to introduce a new packet type, it's probably better to deprecate the current one, rather than keep two

So how about the following sequence of steps:

1. implement "delivery" for packet type 1 (this PR)
2. implement "forwarding" for packet type 9
3. deprecate packet type 1

[adrianhopebailie]

What's the motivation behind:

deprecate packet type 1

As a sender I may want to put the final destination amount in the packet because my experience tells me that the connectors I use are able to find better routes if they have that information.

I understand the motivation for 1 and 2 (and support moving amount up the stack and making it invisible to the intermediaries) but shouldn't that be optional?

I guess we could have a transport layer protocol that explicitly provides intermediaries with some contextual data (i.e. it's not encrypted).

@sappenin is this something you'd likely be working on with regulatory requirements in mind?

[michielbdejong]

@emschwartz can this be merged now?