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.

Sign up for GitHub

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

Jump to bottom

Fix json marshalling #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix json marshalling #8

wants to merge 5 commits into from

Conversation

canndrew
Copy link
Member

This PR implements json marshalling in DNL where it belongs. This allows geewalet tests to pass on stockmono.

It also redefines single-variant discriminated union types as regular non-discriminated-union types.

knocte and others added 2 commits August 18, 2020 13:46
@knocte
Revert "bump NBitcoin version 5.0.41 -> 5.0.49"
48f7e60 
This reverts commit 21fb15e,
because:

- There was no motivation for this upgrade (nothing specified
in the commit message).
- It might introduce bugs that we don't want to deal with, at
the moment.
- We might revert this revert shortly after finishing our
milestone4 (closing channels).
@canndrew @knocte
Mono-hop unidirectional payments
cae6b93
@canndrew canndrew force-pushed the nblockchain-master-with-json-marshalling branch 2 times, most recently from 78dc243 to 6234c40 Compare August 18, 2020 07:23
knocte
knocte reviewed Aug 18, 2020
View reviewed changes
src/DotNetLightning.Core/Utils/Primitives.fs
@@ -333,23 +335,24 @@ module Primitives =
static member (*) (a: FeeRatePerKw, b: uint32) =
(a.Value * b) |> FeeRatePerKw
/// Block Hash
type BlockId = | BlockId of uint256 with
member x.Value = let (BlockId v) = x in v
[<Struct>]
Copy link
Member

@knocte knocte Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

how about adding [<StructuralComparison;StructuralEquality>] instead of [<Struct>] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an advantage to that? My understanding is that [<Struct>] makes the type a thin wrapper around uint256, which seems to be what we want.

Copy link
Member

Choose a reason for hiding this comment

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

@canndrew Define thin wrapper? There are many differences between classes and structs (e.g. allocated on stack instead of heap, AFAIR). But if what we care about here is only comparison, then I'd like to know if using [<StructuralComparison;StructuralEquality>] works, because if it does:
a) why not do this instead? the change would be more readable because it would declare intent by itself;
b) some cases I see which are now [<Struct;CustomComparison;CustomEquality>] may not need to change at all.

Copy link
Member Author

@canndrew canndrew Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

To me "thin wrapper" means, at least in this case, that the outer layer of the value is allocated on the stack (so there aren't two pointer-indirections) and that the type forwards equality to the wrapped type.

I agree that [<StructuralComparison;...>] arguably expresses the intent more directly, however I had to add [<Struct;...>] to some of those types to get it to compile in the first place. I only added it to 3 more types when fixing the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically, (since I just tried to build without Struct to see what the errors are), I get the error: Only record, union, exception and struct types may be augmented with the 'ReferenceEquality', 'StructuralEquality' and 'StructuralComparison' attributes.

This is the error I get if I, for instance, replace the [<Struct;StructuralComparison;StructuralEquality>] on TxId with just [<StructuralComparison;StructuralEquality>].

Copy link
Member

Choose a reason for hiding this comment

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

interesting... but in the case of this hunk:

-    [<CustomEquality;CustomComparison;StructuredFormatDisplay("{AsString}")>]
+    [<Struct;CustomEquality;CustomComparison;StructuredFormatDisplay("{AsString}")>]

why is Struct needed? if the equality and comparison are still custom! do tests pass if you remove this hunk from the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error in this case is: This type uses an invalid mix of the attributes 'NoEquality', 'ReferenceEquality', 'StructuralEquality', 'NoComparison' and 'StructuralComparison'

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand, if that was the case master branch wouldn't compile at all, because it currently has [<CustomEquality;CustomComparison;StructuredFormatDisplay("{AsString}")>] before the changes from your PR.

@canndrew @knocte
Fix chain hashes being serialized in reverse
a0ea6e1 
This is a fix for changes introduced in
joemphilips#118 which caused
chain hashes to get serialized in reverse order.
@knocte
Copy link
Member

knocte commented Aug 19, 2020

@canndrew please rebase this PR (and rename title from "Implement" to "Fix").

@canndrew
Redefine single-variant discriminated union types
1820f86 
A lot of types in DotNetLightning are defined as discriminated unions
with a single constructor with the same name as the type. eg:

    type NodeId = | NodeId PubKey

This commit redefines these types as:

    type NodeId(id: PubKey) = ...

The main motivation for this is that having types and constructors with
the same name breaks the json serialization library on older mono
versions. It can also lead to confusing error messages. Besides that,
it's also somewhat pointless to define a type as a discriminated union
but then only give it one variant.
@canndrew
Add JsonMarshalling module
afd7f3a 
This commit adds the JsonMarshalling module which defines json
serialization converters for a bunch of types exported by
DotNetLightning. This allows the consumer of DotNetLightning to
serialize the channel state to a wallet file.
@canndrew canndrew force-pushed the nblockchain-master-with-json-marshalling branch from 537f6e3 to afd7f3a Compare August 19, 2020 10:54
@canndrew canndrew changed the title Implement json marshalling Fix json marshalling Aug 19, 2020
@knocte knocte force-pushed the master branch 6 times, most recently from 135c9bb to 02564ea Compare October 21, 2020 17:02
@knocte knocte force-pushed the master branch 3 times, most recently from dd99cda to 08262fb Compare November 6, 2020 05:16
@knocte knocte force-pushed the master branch 8 times, most recently from 48f2a9f to 06581b9 Compare November 13, 2020 06:21
@knocte knocte force-pushed the master branch 3 times, most recently from f9839d5 to 6281acc Compare April 2, 2021 09:52
@knocte knocte force-pushed the master branch 3 times, most recently from 9f9f73e to 2883c78 Compare June 17, 2021 09:03
@canndrew canndrew force-pushed the master branch 4 times, most recently from f817da9 to 0340b0b Compare June 18, 2021 07:38
@knocte knocte force-pushed the master branch 3 times, most recently from 0389f6a to 60d2401 Compare June 22, 2021 13:06
@knocte knocte force-pushed the master branch 4 times, most recently from 419cd8d to f0461b4 Compare November 17, 2021 06:50
@knocte knocte force-pushed the master branch 4 times, most recently from 37fe5af to 1f418fe Compare December 2, 2021 21:08
@knocte knocte force-pushed the master branch 5 times, most recently from bd34034 to b67e13f Compare August 23, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knocte knocte knocte left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@canndrew @knocte