interledger-deprecated / java-ilp-core

WE'VE MOVED: This project has moved to Hyperledger Quilt
https://github.com/hyperledger/quilt/tree/master/ilp-core
Apache License 2.0
16 stars 10 forks source link

refactor: remove withNonce from PskMessageBuilder #40

Closed andrew-g-za closed 7 years ago

andrew-g-za commented 7 years ago

Changes as per #30 Note that although the .withNonce methods have been removed, users can still explicitly add a nonce header via the builder. Maybe that needs to be prevented too?

Ps. note sure whats gotten into git, the PR seems to reference all the changes I've made today, it should really only show 2d5c34e?

codecov-io commented 7 years ago

Codecov Report

Merging #40 into development will decrease coverage by 0.64%. The diff coverage is 30%.

Impacted file tree graph

@@                Coverage Diff                @@
##             development      #40      +/-   ##
=================================================
- Coverage          57.02%   56.38%   -0.65%     
  Complexity           159      159              
=================================================
  Files                 57       57              
  Lines                989     1002      +13     
  Branches              90       94       +4     
=================================================
+ Hits                 564      565       +1     
- Misses               375      388      +13     
+ Partials              50       49       -1
Impacted Files Coverage Δ Complexity Δ
...java/org/interledger/psk/model/PskMessageImpl.java 57.77% <0%> (-26.1%) 12 <0> (ø)
...in/java/org/interledger/psk/PskMessageBuilder.java 87.87% <100%> (+5.52%) 10 <3> (ø) :arrow_down:

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 b7b4d48...16f7714. Read the comment docs.

sappenin commented 7 years ago

Overall, Looks good. I'd like to prevent users from manually adding headers with the name Encryption, None or Key, although that makes testing a bit more complicated.

I think for testing purposes, we could create a test-only extension of PskMessageBuilder and overwrite the addPublicHeader methods to not validate against the header-names above. We would only use that extension-builder when trying to create a Message for assertion purposes, but otherwise the tests would use the standard builders, which would enforce the header name restrictions. Thougths?

adrianhopebailie commented 7 years ago

Maybe that needs to be prevented too?

I agree but given the discussion around PSK is still open let's not waste cycles on that. I suggest merging and logging an issue to address this which we might close if the PSK design changes.