Json serializers refactoring #1979
Merged
Conversation
This file contains 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
Having custom serializers depend on external format would introduce an infinite recursion at runtime if not careful. Thankfully, none of our serializers use it, so we may as well remove the possibility entirely.
We don't need to type the serializers: this is required for deserializing, not serializing, and we are not using it. The fact that be had a type mismatch here shows it: ```scala object TransactionSerializer extends MinimalSerializer[TransactionWithInputInfo] ```
Instead of providing a `MyClass => JValue` conversion method, we provide a `MyClass => MyClassJson` method, with the assumption that `MyClassJson` is serializable using the base serializers. The rationale is that it's easier to define the structure with types rather than by building json objects. This also means that the serialization of attributes of class C is out of the scope when defining the serializer for class C. See for example how `DirectedHtlcSerializer` doesn't need anymore to bring in lower level serializers. It also has the advantage of removing recursion from custom serializers which sometimes generated weird stack overflows.
pm47
commented
Oct 1, 2021
Codecov Report
@@ Coverage Diff @@
## master #1979 +/- ##
==========================================
+ Coverage 87.63% 87.67% +0.04%
==========================================
Files 158 158
Lines 12416 12410 -6
Branches 530 518 -12
==========================================
Hits 10881 10881
+ Misses 1535 1529 -6
|
t-bast
reviewed
Oct 1, 2021
eclair-core/src/main/scala/fr/acinq/eclair/json/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
t-bast
previously approved these changes
Oct 1, 2021
t-bast
approved these changes
Oct 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Best reviewed commit-by-commit.