Add System.Text.Json converters for our Newtonsoft JSON converters + add tests

[Szpoti]

OP changed by Kimi:

The goal of this PR is to introduce STJ versions of NewtonSoft.JSON converters that behave the same (as much as possible). This PR adds tests to show behavior of STJ and NSJ converters. NSJ converters were not functionally changed.

STJ has the following properties:

• Pro: Actively developed and written with performance, correctness, etc. in mind¹.
• Pro: STJ is IMO better documented than Newtonsoft.json at this point
• Pro: Performance-wise it is about twice as fast. The precise speedup is not that important imo. Also it uses less memory, so less GCs.
• Pro: STJ is a part of .NET SDK so no dependency on "external" code is needed. Though, Newtonsoft is now under Microsoft wings though I believe.
• Pro: STJ is more AOT friendly than Newtonsoft.

The short term goal is to save some time in reading Client config JSON file.

Changes in behaviour

1. InvalidCastException became JsonException at in:
   • NetworkJsonConverter
   • MoneySatoshiJsonConverter
   • FeeRateJsonConverter
2. EndPointJsonConverterNg accepts null value for JsonWriter to write. The same is true for NetworkJsonConverterNg here. The reason being that EndPointJsonConverter and NetworkJsonConverter skips the WriteJson call when the value to write is null, so the exceptions here and here are not thrown.

Resources

• https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-7-0

Footnotes

1. Newtonsoft.json was for a long time the number one library for JSON support but it certainly does not have the level of design work behind. ↩

[Szpoti]

By the latest commit, deserializing:

• Constants.MaximumNumberOfBitcoins + 1 sat (209999999.97690001) -> Can deserialize✅
• 21 million bitcoin -> Can deserialize✅
• 1. (no digit after decimal point) -> Can deserialize (1.00000000)✅
• 1e6 value -> Can't deserialize❌
• 1,0 (decimal comma) -> Can't deserialize❌
• 1,000.00 (thousands separator comma) -> Can't deserialize❌
• 0.00000000000000000000000000000000000000000000001 -> Can deserialize (0.00000000)✅
• 00000000000000000000000 -> Can deserialize (0.00000000)✅

In these cases, either both succeded or both failed, meaning their behaviour is the same.

[Szpoti]

I moved AssertDeserializeJsonExceptionFailure(token) into it's own test, as the exception itself comes from the missing " quotes from around the value. It doesn't make sense to check it in every value case.

[Szpoti]

@kiminuo I don't know if this "typo" was intentional or not, but the missing } that I fixed in
809538b caused the test to pass, as it was an invalid format after all, but not because of the numeric value.
Fixed the typo, failed the test.
Now Newtonsoft gives back null at

	public override Money? ReadJson(JsonReader reader, Type objectType, Money? existingValue, bool hasExistingValue, JsonSerializer serializer)
	{
		var stringValue = reader.Value as string;
		return Parse(stringValue);
	}

and System.Text.Json throws json exception at

	public override Money? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
	{
		if (reader.TokenType != JsonTokenType.String)
		{
			throw new JsonException("Expected a JSON number value.");
		}

[Szpoti]

if (reader.TokenType != JsonTokenType.String)
		{
			throw new JsonException("Expected a JSON number value.");
		}

This is weird, based on the if, we don't expect a number value here, it's the opposite. We expect string value, and throw if it's not.

[Szpoti]

Also FYI, there are big inconsistency in how we handle different values in json. Some numbers are numeric, some or strings, and I can't really see the pattern, why its like this. For example here, MoneyBtcJsonConverter works with string values like "Money":"10.0", but FeeRateJsonConverter works with numeric values like "Rate":10.0.

Also if you look at WabiSabiConfig.json (default state):

• "ConfirmationTarget": 108
• "Rate": 0.003
• "MinInputCountByRoundMultiplier": 0.5
• "DoSSeverity": "0.10"
• "MaxSuggestedAmountBase": "0.10"
• "MaxRegistrableAmount": "43000.00"

Do you know why?

[kiminuo]

I don't know if change in behavior is a problem but I'd like to know the reason behind.

The goal is not to change behavior, if possible. Changes in behavior should be IMHO documented in the post here.

[kiminuo]

@turbolay @adamPetho I think this is quite close to being done. Could you review it one more time please?

[adamPetho]

Overally LGTM.

Questions and nitpicks below.

[adamPetho]

We can get rid of the nullability of the return type if at the end we do a check like this.

Network network = Network.GetNetwork(networkString);
if (network is null) => throw Exception
return network

[kiminuo]

I think that we can do:

networkString = reader.GetString()?.Trim();

if (networkString is null)
{
	throw new ArgumentNullException(nameof(networkString));
}

->

networkString = reader.GetString()!.Trim();

That makes sense to me because null is checked before.

[adamPetho]

I figured out that changing the nullability breaks the test.
If we don't allow to return null, I think we change the behavior, too. Which goes against the goal of this PR.

It's up to you guys, if you want to deal with this in this PR or in another PR or never.

[kiminuo]

I applied my patch and it does not break tests.

[adamPetho]

What's wrong with TimeSpan.Parse(stringValue)?
Does that behave so differently that we need our own parser function?

[kiminuo]

What's wrong with TimeSpan.Parse(stringValue)?

Nothing IMO.

Does that behave so differently that we need our own parser function?

Dunno, it just looks like somebody prefered different format at that time. TimeSpan.Parse(s) parses different format: [ws][-]{ d | [d.]hh:mm[:ss[.ff]] }[ws] according to https://learn.microsoft.com/en-us/dotnet/api/system.timespan.parse?view=net-7.0#system-timespan-parse(system-string).

[turbolay]

we should look into this in another PR, it's weird

[kiminuo]

But what do you guys want to achieve really?

If people like the format <days>d <hours>h <minutes>m <seconds>s, then it does what it is supposed to do. It's implemented in an inefficient way but AFAIK there are not that many TimeSpans to convert, or not?

edit: I believe that a compiled regex would be pretty fast here.

[turbolay]

With https://github.com/zkSNACKs/WalletWasabi/pull/11840/files#r1394254325 LGTM

[turbolay]

When is this the case?

[kiminuo]

if you read null as a JSON token. Not "null" but simply null - e.g. { "Network": null }.

[kiminuo]

I merged the PR. I think it's finished. Feel free to create any follow-up PR if you have an idea what to improve. But I think everything was addressed.

Thanks for the help guys.