Skip to content
This repository has been archived by the owner on Oct 7, 2019. It is now read-only.

Feat: add prepare/fulfill/reject #26

Merged
merged 1 commit into from Dec 23, 2017
Merged

Feat: add prepare/fulfill/reject #26

merged 1 commit into from Dec 23, 2017

Conversation

sharafian
Copy link

@codecov-io
Copy link

codecov-io commented Dec 23, 2017

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%) ⬆️

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.

@sharafian sharafian merged commit 97a8eae into master Dec 23, 2017
@sharafian sharafian deleted the bs-pfr branch December 23, 2017 01:55
@@ -613,6 +616,138 @@ export const deserializeIlpRejection = (binary: Buffer): IlpRejection => {
}
}

export interface IlpPrepare {
amount: string,
executionCondition: Buffer,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have the expires at field before the condition so the two fields that change come first

Copy link
Author

Choose a reason for hiding this comment

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

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

@emschwartz
Copy link
Member

emschwartz commented Dec 23, 2017 via email

@sharafian
Copy link
Author

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
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants