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

Add TLV and TLV stream codec support #1045

Merged
merged 12 commits into from Jul 2, 2019

Conversation

Projects
None yet
4 participants
@t-bast
Copy link
Member

commented Jun 21, 2019

Implement the latest TLV/TLV stream proposal (on which we all agreed during the last IRC meeting, spec PR is only waiting for final language edits and test vectors).

I took this opportunity to refactor the wire codecs (move stuff around and harmonize) and update/remove some obsolete code (features that were missing in scodec and that are now available there).

I recommend reviewing commit by commit, this will make it easier to ignore the noise generated by moving many codec from LightningMessageCodecs to CommonCodecs.

t-bast added some commits Jun 20, 2019

Remove unused FixedSizeStrictCodec.
Scodec has integrated that feature already.
Add generic TLV codec.
Codecs will be namespaced, hence the use of child traits of the Tlv trait.
Codecs refactoring.
Move common codecs to their own file.
Harmonize use of val vs def for fields.
Add tlv stream codec.
Add minimal encoding enforcement in varInt codec.

@t-bast t-bast requested review from pm47 and sstone Jun 21, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 21, 2019

Codecov Report

Merging #1045 into master will increase coverage by 0.46%.
The diff coverage is 98.71%.

@@            Coverage Diff             @@
##           master    #1045      +/-   ##
==========================================
+ Coverage   80.35%   80.81%   +0.46%     
==========================================
  Files          98      100       +2     
  Lines        7630     8039     +409     
  Branches      303      316      +13     
==========================================
+ Hits         6131     6497     +366     
- Misses       1499     1542      +43
Impacted Files Coverage Δ
...ain/scala/fr/acinq/eclair/wire/CommandCodecs.scala 100% <ø> (ø) ⬆️
...blockchain/electrum/db/sqlite/SqliteWalletDb.scala 84.52% <ø> (ø) ⬆️
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala 100% <ø> (ø) ⬆️
...scala/fr/acinq/eclair/payment/PaymentRequest.scala 92% <ø> (ø) ⬆️
.../fr/acinq/eclair/wire/LightningMessageCodecs.scala 100% <100%> (+0.86%) ⬆️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala 100% <100%> (ø) ⬆️
...rc/main/scala/fr/acinq/eclair/wire/TlvCodecs.scala 100% <100%> (ø)
...c/main/scala/fr/acinq/eclair/crypto/ShaChain.scala 94.87% <100%> (ø) ⬆️
...in/scala/fr/acinq/eclair/wire/FailureMessage.scala 56.86% <100%> (-0.83%) ⬇️
...cala/fr/acinq/eclair/db/sqlite/SqlitePeersDb.scala 96% <100%> (ø) ⬆️
... and 17 more
@pm47
Copy link
Member

left a comment

Looking pretty good so far

@t-bast t-bast self-assigned this Jun 24, 2019

@t-bast

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

Yesterday's meeting confirmed that the spec PR (lightningnetwork/lightning-rfc#607) is accepted, we're only waiting for test vectors to be added.
You can thus safely review this PR and rebase on it if you need TLV.

t-bast added some commits Jun 28, 2019

Move TlvStream errors to companion object.
Rename uint64 codecs to harmonize.
Clean up Tlv stream.
Validation during case class `apply`.
Simplify codec using the built-in `list`.
@t-bast

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Allright this should be ready for final review now (thanks for @pm47's love for scodec ;))

@pm47

pm47 approved these changes Jul 2, 2019

@t-bast t-bast merged commit 1cc14ae into master Jul 2, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@t-bast t-bast deleted the tlv branch Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.