NomicFoundation / edr

An Ethereum development runtime implementation that can be reused to build new developer tools.
MIT License
52 stars 8 forks source link

fix: fix `eth_feeHistory` result deserialization #484

Closed agostbiro closed 4 months ago

agostbiro commented 4 months ago

The JSON-RPC response struct in edr_eth contains the response in the data field which is flattened for serialization to allow handling the result/error field as an enum:

/// Represents a JSON-RPC 2.0 response.
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
pub struct Response<T> {
    /// A String specifying the version of the JSON-RPC protocol.
    pub jsonrpc: Version,
    //
    /// Correlation id.
    ///
    /// It **MUST** be the same as the value of the id member in the Request
    /// Object.
    pub id: Id,
    /// Response data.
    #[serde(flatten)]
    pub data: ResponseData<T>,
}

Turns out this doesn't play well with the arbitrary_precision feature of serde_json which causes the response from eth_feeHistory fail to parse. The reason only eth_feeHistory is failing, because this is the only RPC response with a number in it, all other methods supported by the client return numbers as hex strings.

The EDR crates don't need the arbitrary_precision feature of serde_json, but it's needed by the parseJson Foundry cheatcode. The reason it's causing problems in edr_eth is feature unification which kicks in when both the EDR and Foundry crates are included in edr_napi.

To fix the problem I added custom deserialization logic for the numerical field of the eth_feeHistory response. Parsing the numbers first into serde_json::Number and then into the desired number type makes flatten work with arbitrary_precision.

I also made all EDR crates refer to the workspace version of serde and serde_json. This was the case before as well (as evidenced by the lack of Cargo.lock change), but this way it's clearer to the reader.

I have taken the approach of not having per-crate feature definitions for serde and serde_json, but having all the required ones in the workspace's Cargo.toml. This way there are no surprises when code under test behave one way in a crate, but behaves differently when used as a dependency in another trait.

changeset-bot[bot] commented 4 months ago

⚠️ No Changeset found

Latest commit: cae0e7b08d2399aa0c0311bacb61c7c4185f97e6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR