Conversation
Scodec has integrated that feature already.
Codecs will be namespaced, hence the use of child traits of the Tlv trait.
Move common codecs to their own file. Harmonize use of val vs def for fields.
Add minimal encoding enforcement in varInt codec.
Codecov Report
@@ 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
|
pm47
reviewed
Jun 22, 2019
eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageCodecs.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/wire/LightningMessageCodecsSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageCodecs.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageCodecs.scala
Outdated
Show resolved
Hide resolved
pm47
reviewed
Jun 22, 2019
eclair-core/src/main/scala/fr/acinq/eclair/wire/TlvCodecs.scala
Outdated
Show resolved
Hide resolved
pm47
reviewed
Jun 23, 2019
This was referenced Jun 24, 2019
Member
Author
|
Yesterday's meeting confirmed that the spec PR (lightning/bolts#607) is accepted, we're only waiting for test vectors to be added. |
pm47
reviewed
Jun 27, 2019
eclair-core/src/main/scala/fr/acinq/eclair/wire/CommonCodecs.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/CommonCodecs.scala
Outdated
Show resolved
Hide resolved
sstone
reviewed
Jun 27, 2019
Rename uint64 codecs to harmonize.
Validation during case class `apply`. Simplify codec using the built-in `list`.
Member
Author
|
Allright this should be ready for final review now (thanks for @pm47's love for |
pm47
approved these changes
Jul 2, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
LightningMessageCodecstoCommonCodecs.