interledgerjs / ilp-protocol-stream

Moved to monorepo in interledgerjs/interledgerjs
https://github.com/interledger/rfcs/blob/master/0029-stream/0029-stream.md
32 stars 15 forks source link

feat: server reliability improvements #136

Closed kincaidoneil closed 4 years ago

kincaidoneil commented 4 years ago

Improvements:

Also: fixes vulnerabilities, and updates the logging so the same connection ID is used between the client & server, by using the token in the destination ILP address.

codecov-io commented 4 years ago

Codecov Report

Merging #136 into master will decrease coverage by 2.17%. The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   91.26%   89.09%   -2.18%     
==========================================
  Files          14       14              
  Lines        1500     1733     +233     
  Branches      108      268     +160     
==========================================
+ Hits         1369     1544     +175     
+ Misses        112      111       -1     
- Partials       19       78      +59     
Impacted Files Coverage Δ
src/index.ts 83.33% <ø> (-1.97%) :arrow_down:
src/pool.ts 100.00% <ø> (ø)
src/connection.ts 88.25% <94.73%> (-1.75%) :arrow_down:
src/crypto.ts 100.00% <100.00%> (ø)
src/server.ts 85.71% <100.00%> (-1.79%) :arrow_down:
src/util/rational.ts 75.00% <0.00%> (-6.64%) :arrow_down:
src/stream.ts 87.45% <0.00%> (-4.41%) :arrow_down:
src/util/congestion.ts 80.95% <0.00%> (-4.05%) :arrow_down:
src/util/data-offset-sorter.ts 91.66% <0.00%> (-1.89%) :arrow_down:
... and 7 more

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 d2c6b14...85070e0. Read the comment docs.

kincaidoneil commented 4 years ago

A couple changes:

  1. A hold is applied to totalReceived before calling shouldFulfill, which resolves the overflow issue and prevents a race condition.
  2. I changed shouldFulfill so it's provided a unique packet ID, which is generated via hmac(sharedSecret, "ilp_stream_packet_id" + sequence). This allows shouldFulfill to be used even if there's no connection tag, is a little more intuitive for developers, and maybe resolves #131.

Ready for re-review.

kincaidoneil commented 4 years ago

Right now the packetId is serialized as UTF-8. Should it be serialized differently (base64? hex?) or should the callback just be given the buffer directly so it the developer can choose how they want to serialize it?

sharafian commented 4 years ago

Right now the packetId is serialized as UTF-8

This seems fine to be so long as it has a reasonable text representation. If there are values that don't represent actual character values then let's use a buffer rather than base64 or hex