Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use best rate when ILP packet amount is zero #397

Merged
merged 2 commits into from
Nov 29, 2017
Merged

Conversation

justmoon
Copy link
Contributor

As proposed in interledger/rfcs#313.

@michielbdejong suggested that overloading the existing packet format is a bad idea. In a general sense, I agree. As a practical matter, this patch is (as you can see below) very simple, breaks none of the existing tests (and doesn't break any existing ILP use cases) and allows us to build modern chunked/streaming payment transport layer protocols on our existing reference implementation. (Plan is that @emschwartz will start to work on back-porting the "ILP3" transport layer into the ilp module this week.)

I think the ultimate goal should be a cleaned-up ILPv2/BestEffort packet, where the amount is removed, see interledger/rfcs#323. However, I like the idea of experimenting with the new transport layer protocols right away and building this new packet using the experience gathered from that work. So I would urge us to adopt @adrianhopebailie's proposal (specifically the part about taking amount = 0 to mean best-effort) for now.

See also the sister PR for ilp-connector-shard: interledger-deprecated/ilp-connector-shard#4

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #397 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #397   +/-   ##
=======================================
  Coverage   84.82%   84.82%           
=======================================
  Files          22       22           
  Lines         336      336           
  Branches       49       49           
=======================================
  Hits          285      285           
  Misses         51       51

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 0f95f82...fd61e4e. Read the comment docs.

Copy link
Member

@emschwartz emschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should wait for @michielbdejong's comments before merging this, but this sounds good to me.

In terms of whether we should go with this change or something like interledger/rfcs#323 in the long run, one of the questions we should consider is whether we want to continue to support the delivery behavior going forward or just deprecate it. If we want to support it, this seems like the best way to have both. Long term, I don't think we're going to have two different packet types going across the network (you could make the case that two functions baked into one type isn't much difference, which is probably true so we might need to pick one or the other anyway).

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this now as a quick hack to unblock development; I created #398 as a follow-up PR where we can discuss the proper longer-term implementation.

@justmoon justmoon merged commit 1ba272b into master Nov 29, 2017
@emschwartz emschwartz deleted the feat/st-best-rate-zero branch December 14, 2017 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants