Support Prysm and Ethdo Keystores (Fixes #4107) #4149
Conversation
| writer.writeField("version", JsonString( | ||
| Base10.toString(uint64(value.version)))) | ||
| writer.endRecord() | ||
| error: "keystores must be converted to json with Json.encode(keystore). " & |
There was a problem hiding this comment.
we use the "default" json serialization for logging - we should use a dedicated standard serializer for the keystore standard, which may or may not match our logging json serializer
There was a problem hiding this comment.
ie the correct way to do this is to introduce a KeystoreJson that implements the keystore standard, whatever that may be - we cannot keep on using Json, this prevents migrating nim-json-rpc to json-ser for example
There was a problem hiding this comment.
While this would be a theoretical improvement, the current state of affairs is that Json.encode(keystore) is a correct implementation of the standard, while the removed functions here were not.
There was a problem hiding this comment.
well, architecturally, this PR is a regression because it moves further away from dedicated serializers - we could do the right thing here and introduce the KeystoreJson already - otherwise, it deserves documenting at least, because it's a fragile setup causing easily preventable bugs
There was a problem hiding this comment.
in particular, the outright recommendation to use Json.encode is simply poor advice - it's one hack/workaround replacing another
There was a problem hiding this comment.
In case that's not clear, we have been using Json.decode for keystores all along. The encoding/decoding routines were defined before the support for flavours were added to the JSON library and they are tested in the test_keystore.nim module. This PR is certainly not a regression as it removes an unnecessary duplicated definition that should not have been created at all. It also addresses a very specific issue, encountered by our end users.
I don't disagree regarding introducing KeystoreJson, but this can happen in another PR.
No description provided.