refactor: encode JsonString as object in scale #5084
Closed
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
mversic
requested changes
Sep 18, 2024
Comment on lines
+2395
to
+2427
| "JsonString": { | ||
| "Enum": [ | ||
| { | ||
| "tag": "Null", | ||
| "discriminant": 0 | ||
| }, | ||
| { | ||
| "tag": "String", | ||
| "discriminant": 1, | ||
| "type": "String" | ||
| }, | ||
| { | ||
| "tag": "Bool", | ||
| "discriminant": 2, | ||
| "type": "bool" | ||
| }, | ||
| { | ||
| "tag": "Array", | ||
| "discriminant": 3, | ||
| "type": "Vec<JsonString>" | ||
| }, | ||
| { | ||
| "tag": "Number", | ||
| "discriminant": 4, | ||
| "type": "String" | ||
| }, | ||
| { | ||
| "tag": "Object", | ||
| "discriminant": 5, | ||
| "type": "SortedMap<String, JsonString>" | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Suggested change
| "JsonString": { | |
| "Enum": [ | |
| { | |
| "tag": "Null", | |
| "discriminant": 0 | |
| }, | |
| { | |
| "tag": "String", | |
| "discriminant": 1, | |
| "type": "String" | |
| }, | |
| { | |
| "tag": "Bool", | |
| "discriminant": 2, | |
| "type": "bool" | |
| }, | |
| { | |
| "tag": "Array", | |
| "discriminant": 3, | |
| "type": "Vec<JsonString>" | |
| }, | |
| { | |
| "tag": "Number", | |
| "discriminant": 4, | |
| "type": "String" | |
| }, | |
| { | |
| "tag": "Object", | |
| "discriminant": 5, | |
| "type": "SortedMap<String, JsonString>" | |
| } | |
| ] | |
| }, | |
| "JsonValue: "JsonValue" |
There was an idea that we can introduce a new json type into schema. It would be up to the sdk to know how to handle this type like it is for Option<T> or Vec<T>. What happened with this suggestion?
There was a problem hiding this comment.
I'm not aware of this idea.
I think the main difference is that Vec and Option has native representation in scale and json doesn't.
| #[display(fmt = "{_0}")] | ||
| pub struct JsonString(String); | ||
| #[serde(transparent)] | ||
| pub struct JsonString(Value); |
There was a problem hiding this comment.
Suggested change
| pub struct JsonString(Value); | |
| pub struct JsonValue(Value); |
if we're to proceed with this change we'll have to rename it
There was a problem hiding this comment.
Yes, made a draft with minimal changes
Erigara
force-pushed
the
json_value_to_scale
branch
from
2e95196
to
f64ca4e
Compare
Signed-off-by: Shanin Roman <[email protected]>
Erigara
force-pushed
the
json_value_to_scale
branch
from
f64ca4e
to
5e6935c
Compare
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.
Context
Aim to close differences between
scaleandjsonby encodingJsonStringas enum in scale and not justString.Closes #5024
Solution
Approach is to manually implement encode and decode for
JsonStringand describe it in schema.Review notes
I am not sure that this will help since there is anyway not 1 to 1 mapping between
scaleandjson.And in the
jsonwe still expect arbitrary json object and not smt encoded using schema...