Add support for deserialization of owned values#8
Open
thomaseizinger wants to merge 3 commits intofspmarshall:masterfrom
Open
Add support for deserialization of owned values#8thomaseizinger wants to merge 3 commits intofspmarshall:masterfrom
thomaseizinger wants to merge 3 commits intofspmarshall:masterfrom
Conversation
bors bot
added a commit
to comit-network/comit-rs
that referenced
this pull request
Apr 7, 2020
2405: Remove dependency on `primitive_types` for Ethereum types such as Address and TransactionHash r=da-kami a=da-kami `H256` (Hash) and `H160` (Address) are converted to our own types. Note that for deserialization we always have to pass the Hash/Address by reference, otherwise we don't reach the right code-path in serde hex. @thomaseizinger already [tackled that problem](fspmarshall/serde-hex#8). Unfortunately, the `U256` [value in Ethereum is not fixed hex](https://github.com/ethereum/wiki/wiki/JSON-RPC#hex-value-encoding) (although it actually is of fixed size, namely `[u8, 32]`). This leads to the problem, that we would have to implement our own `Serializer` and `Deserializer` because serde-hex does not implement `Compact` for `[u8, 32]`. Implementing a custom `Serializer` and `Deserializer` is, however, not trivial, because our hex library does not support compacted hex neither. I started pulling in the [serialization and deserialization code from the parity codebase](https://github.com/paritytech/parity-common/blob/master/primitive-types/impls/serde/src/serialize.rs), but it is quite a lot of code to be pulled in, that is rather complicated. Given, that this ticket was under the premise that we (most likely...) can just use serde-hex or our hex library for encoding/decoding the value, this is getting a rabbit hole. Since I already wasted quite some time on this ticket due to a tricky [serde-hex bug](fspmarshall/serde-hex#8) I would put the implementation of `U256` aside now and keep the dependency on `primitive_types` for it. We can decide how/if we want to deal with `U256` later. I am not sure we want to own the custom serialization/deserialization code, or if it is not better to use `primitive_types::U256`. Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>
|
Many thanks for this patch! I have spend a lot of time trying to figure out why this toy example: use serde::{Deserialize};
use serde_hex::{SerHex, CompactPfx};
#[derive(Debug, Deserialize)]
struct Foo {
#[serde(with = "SerHex::<CompactPfx>")]
bar: u32
}
const INI: &str = r#"bar = 0x42"#;
fn main() {
let foo: Foo = serde_ini::from_str(INI).unwrap();
println!("{:?}", foo);
}Was providing the following result: Cheers! |
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 18, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 18, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 18, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 19, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 20, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 20, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 20, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 21, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
thomaseizinger
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 21, 2020
This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405
bors bot
added a commit
to comit-network/comit-rs
that referenced
this pull request
Aug 21, 2020
3071: Infer DAI token from Ethereum chain ID r=mergify[bot] a=thomaseizinger We are going to use this to avoid including the token contract address in the orders that we are sending around. This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405 Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
nklhtv
approved these changes
Sep 13, 2022
|
Awesome! Exactly what i needed! |
This was referenced Sep 13, 2022
thomaseizinger
force-pushed
the
support-deserialize-owned-strings
branch
from
890413b to
0edfd91
Compare
6 tasks
Contributor
|
ping @fspmarshall |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Resolves #7.
There is some duplication between the visitors so there is potential for follow-up work. The feature works however and is tested with 2 additional tests I added.
Let me know what you think!