-
Notifications
You must be signed in to change notification settings - Fork 279
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.
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
refactor!: shift from JsonString
to JsonValue
#5012
Conversation
Signed-off-by: 0x009922 <[email protected]>
Signed-off-by: 0x009922 <[email protected]>
Signed-off-by: 0x009922 <[email protected]>
fed2414
to
3652169
Compare
@@ -930,7 +930,7 @@ mod transparent { | |||
/// Id of a trigger to execute | |||
pub trigger: TriggerId, | |||
/// Arguments to trigger execution | |||
pub args: Option<JsonString>, | |||
pub args: Option<JsonValueWrap>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use just JsonValue
? and absence of arguments could be just "null"
@@ -7,170 +7,281 @@ use alloc::{ | |||
string::{String, ToString}, | |||
vec::Vec, | |||
}; | |||
use core::str::FromStr; | |||
// use core::str::FromStr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code.
@@ -80,7 +80,8 @@ fn mutlisig() -> Result<()> { | |||
.map(Register::account), | |||
)?; | |||
|
|||
let call_trigger = ExecuteTrigger::new(multisig_register_trigger_id).with_args(&args); | |||
let call_trigger = ExecuteTrigger::new(multisig_register_trigger_id) | |||
.with_args(serde_json::to_value(&args).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like a step back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed implicit conversions on purpose.
Before there was From<T: Serialize> for JsonValue
, and was able to panic implicitly if for some reason T
fails to serialize as JSON. Now it is explicit, at least.
I tried to make emphasis on relying on the "standard" way of working with JSON in Rust, i.e. on serde_json
.
Actually, this serde_json::to_value(&args).unwrap()
could be shortcut to just json!(&args)
. And I would strongly prefer that rather than adding some methods to JsonValue
, making it unintuitive to use.
Temporarily suspended in favor of #5024 |
Context
JsonString
is a type in the schema for arbitrary JSON values. In SCALE it is encoded as a string (hence the name), while in JSON it is inlined as any JSON value.There are a few issues:
Option<JsonString>
in JSON: it is impossible to distinguish betweenNone
andSome("null")
, since both serialize asnull
in actual JSON.JsonString
representation in JSON. In JSON it is not a string, which caused confusion for SDK developers.This PR fixes #4900 by disallowing
Option<JsonString>
and essentially forcing SDKs to handle JSON values as a special case and account for its differences in SCALE vs JSON.Solution
by these changes:
JsonString
toJsonValue
and make stronger emphasis on using the native Rust JSON type, i.e.serde_json::Value
. Remove ambiguous conversions and delegate it to theserde_json::Value
itself.struct JsonValueWrap { value: JsonValue }
for use withOption
JsonValue
as a special type, i.e. not as an alias toString
, and forbid use ofOption<JsonValue>
.Migration Guide
There is a breaking change for SDK developers. Apart from
JsonString
being renamed toJsonValue
, it is also no longer an alias toString
type, but instead is a special typeJsonValue
. SDK should treat it as a special case in a way that is suitable for their platform.Binary and JSON serialisation compatibility is not broken. However, JSON form of
ExecuteTrigger
instruction andExecuteTriggerEvent
changed: itsargs
field was anOption<JsonString>
with ambiguousnull
value. Now it isOption<JsonValueWrap>
, meaning that you need to use it this way:Review notes
primitives/json.rs
, then schema, then the restJsonValue
method names, especiallyget(&self) -> &Value
.Checklist
CONTRIBUTING.md
.