interledgerjs / ilp-packet

Moved to monorepo in interledgerjs/interledgerjs
https://github.com/interledgerjs/interledgerjs
Other
6 stars 5 forks source link

Feat: add prepare/fulfill/reject #26

Closed sharafian closed 6 years ago

sharafian commented 6 years ago

As https://github.com/interledger/rfcs/pull/361

codecov-io commented 6 years ago

Codecov Report

Merging #26 into master will increase coverage by 0.31%. The diff coverage is 92.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage    90.5%   90.81%   +0.31%     
==========================================
  Files           3        3              
  Lines         316      403      +87     
  Branches       48       69      +21     
==========================================
+ Hits          286      366      +80     
- Misses         30       37       +7
Impacted Files Coverage Δ
index.ts 90.64% <92.04%> (+0.38%) :arrow_up:

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 0cc948c...471fce1. Read the comment docs.

emschwartz commented 6 years ago

The expiry should be fixed length too, no?

On Sat, Dec 23, 2017, 9:38 PM Ben Sharafian notifications@github.com wrote:

@sharafian commented on this pull request.

In index.ts https://github.com/interledgerjs/ilp-packet/pull/26#discussion_r158588870 :

@@ -613,6 +616,138 @@ export const deserializeIlpRejection = (binary: Buffer): IlpRejection => { } }

+export interface IlpPrepare {

  • amount: string,
  • executionCondition: Buffer,

I originally advocated for that, but executionCondition and amount are fixed length, and expiresAt might not be

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/interledgerjs/ilp-packet/pull/26#discussion_r158588870, or mute the thread https://github.com/notifications/unsubscribe-auth/ADHIkimuzDK0UNwjbnb93qrvvCcLbKwrks5tDWTTgaJpZM4RLibv .

--

Evan Schwartz Software Engineer

sharafian commented 6 years ago

GeneralizedTime is a variable length octet string; it should usually be the same length (unless there are differences in time zone or milliseconds are omitted). I'm not sure how strict we are about the time zone always being Z/UTC and the milliseconds always being included.

emschwartz commented 6 years ago

Ah that's too bad. I think we should require milliseconds and the UTC timezone no matter what the encoding is