[x/programs] Serialize params on the Rust side #979
Conversation
iFrostizz
requested review from
samliok,
dboehm-avalabs and
richardpringle
as code owners
| case float64: | ||
| // json unmarshal converts to float64 | ||
| cp = append(cp, Parameter{Value: uint64(v), Type: param.Type}) | ||
| case int: | ||
| if v < 0 { | ||
| return nil, fmt.Errorf("%w: %s", errors.New("negative value"), param.Type) | ||
| } | ||
| cp = append(cp, Parameter{Value: uint64(v), Type: param.Type}) | ||
| case string: | ||
| number, err := strconv.ParseUint(v, 10, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: %s", ErrFailedParamTypeCast, param.Type) | ||
| } | ||
| cp = append(cp, Parameter{Value: number, Type: param.Type}) |
There was a problem hiding this comment.
we were not using this
There was a problem hiding this comment.
I'm sure there's more unused code lurking too
iFrostizz
force-pushed
the
rust_params_serialization
branch
from
f5d9746
to
e398f9a
Compare
x/programs/cmd/simulator/src/lib.rs
Outdated
| impl Serialize for Param { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| let mut val = serializer.serialize_struct("param", 2)?; | ||
| match self { | ||
| Param::U64(num) => { | ||
| val.serialize_field("type", "u64")?; | ||
| val.serialize_field("value", &b64.encode(num.to_le_bytes()))?; | ||
| } | ||
| Param::String(text) => { | ||
| val.serialize_field("type", "string")?; | ||
| val.serialize_field("value", &b64.encode(text))?; | ||
| } | ||
| Param::Id(id) => { | ||
| val.serialize_field("type", "id")?; | ||
| let id = serde_json::to_vec(&id).map_err(serde::ser::Error::custom)?; | ||
| let id = &id[1..id.len() - 1]; // remove quotes | ||
| val.serialize_field("value", &b64.encode(id))?; | ||
| } | ||
| Param::Key(key) => { | ||
| let (key, algo) = match key { | ||
| Key::Ed25519(key) => (key, "ed25519"), | ||
| Key::Secp256r1(key) => (key, "secp256r1"), | ||
| }; | ||
| val.serialize_field("type", algo)?; | ||
| val.serialize_field("value", &b64.encode(key))?; | ||
| } | ||
| } | ||
| val.end() | ||
| } | ||
| } |
There was a problem hiding this comment.
I could not find a way to do all of this with the serde attribute macros in a nice way. It would probably be more verbose if we would have used #[serde(serialize_with = "...")] in order to support all of the variants, and it's more readable that way.
There was a problem hiding this comment.
I don't quite understand why everything needs to be base64 encoded. If I had to guess, it's because it's easier to decode everything on the Go side, but you should just be able to switch on the type tag and only base64 decode where necessary. But IMO, it's an extra step that's unnecessary (and adds a little complexity).
Also, I think it would just be beneficial to use an intermediate type here:
#[derive(Serialize)]
#[serde(rename_all = "lowercase", tag = "type", content = "value")]
enum StringParam {
U64(String),
String(String),
Id(String),
#[serde(untagged)]
Key(Key),
}If you want, you can define the type inside the Serialize implementation, do a From conversion, then just serialize the intermediate type. This should all get optimized away by the compiler, especially when the type is only used in one place.
There was a problem hiding this comment.
It's Go making choices for us when dealing with the []byte type, see https://pkg.go.dev/encoding/json#Marshal
Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string, and a nil slice encodes as the null JSON value.
I found online that people were forking the package to strip out the base64 part but I don't think that we should do that.
iFrostizz
force-pushed
the
rust_params_serialization
branch
from
ee4c844
to
f64e564
Compare
| case float64: | ||
| // json unmarshal converts to float64 | ||
| cp = append(cp, Parameter{Value: uint64(v), Type: param.Type}) | ||
| case int: | ||
| if v < 0 { | ||
| return nil, fmt.Errorf("%w: %s", errors.New("negative value"), param.Type) | ||
| } | ||
| cp = append(cp, Parameter{Value: uint64(v), Type: param.Type}) | ||
| case string: | ||
| number, err := strconv.ParseUint(v, 10, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: %s", ErrFailedParamTypeCast, param.Type) | ||
| } | ||
| cp = append(cp, Parameter{Value: number, Type: param.Type}) |
There was a problem hiding this comment.
I'm sure there's more unused code lurking too
x/programs/cmd/simulator/src/lib.rs
Outdated
| impl Serialize for Param { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| let mut val = serializer.serialize_struct("param", 2)?; | ||
| match self { | ||
| Param::U64(num) => { | ||
| val.serialize_field("type", "u64")?; | ||
| val.serialize_field("value", &b64.encode(num.to_le_bytes()))?; | ||
| } | ||
| Param::String(text) => { | ||
| val.serialize_field("type", "string")?; | ||
| val.serialize_field("value", &b64.encode(text))?; | ||
| } | ||
| Param::Id(id) => { | ||
| val.serialize_field("type", "id")?; | ||
| let id = serde_json::to_vec(&id).map_err(serde::ser::Error::custom)?; | ||
| let id = &id[1..id.len() - 1]; // remove quotes | ||
| val.serialize_field("value", &b64.encode(id))?; | ||
| } | ||
| Param::Key(key) => { | ||
| let (key, algo) = match key { | ||
| Key::Ed25519(key) => (key, "ed25519"), | ||
| Key::Secp256r1(key) => (key, "secp256r1"), | ||
| }; | ||
| val.serialize_field("type", algo)?; | ||
| val.serialize_field("value", &b64.encode(key))?; | ||
| } | ||
| } | ||
| val.end() | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't quite understand why everything needs to be base64 encoded. If I had to guess, it's because it's easier to decode everything on the Go side, but you should just be able to switch on the type tag and only base64 decode where necessary. But IMO, it's an extra step that's unnecessary (and adds a little complexity).
Also, I think it would just be beneficial to use an intermediate type here:
#[derive(Serialize)]
#[serde(rename_all = "lowercase", tag = "type", content = "value")]
enum StringParam {
U64(String),
String(String),
Id(String),
#[serde(untagged)]
Key(Key),
}If you want, you can define the type inside the Serialize implementation, do a From conversion, then just serialize the intermediate type. This should all get optimized away by the compiler, especially when the type is only used in one place.
iFrostizz
force-pushed
the
rust_params_serialization
branch
from
6684c5b
to
34e99d2
Compare
iFrostizz
force-pushed
the
rust_params_serialization
branch
from
2883a5e
to
3e31404
Compare
| fn from(val: &'a Id) -> Self { | ||
| &val.0 | ||
| } | ||
| } |
| { | ||
| match self { | ||
| Param::U64(num) => { | ||
| Serialize::serialize(&StringParam::U64(b64.encode(num.to_le_bytes())), serializer) | ||
| } | ||
| Param::String(text) => { | ||
| Serialize::serialize(&StringParam::String(b64.encode(text)), serializer) | ||
| } | ||
| Param::Id(id) => { | ||
| let num: &usize = id.into(); | ||
| let id = format!("step_{}", num); | ||
| Serialize::serialize(&StringParam::Id(b64.encode(id)), serializer) | ||
| } | ||
| Param::Key(key) => Serialize::serialize(key, serializer), | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a little more complex than what I had suggested. Also, you really don't need the step_. In json, the "type" field will be "id", right? that means that step_ is redundant.
Closes #927