ava-labs / avalanche-types-rs

Avalanche primitive types in Rust (experimental)
Other
32 stars 10 forks source link

jsonrpc module update #36

Closed Nuttymoon closed 1 year ago

Nuttymoon commented 1 year ago

Description

This PR is the first effort to update jsonrpc structs to match the recent API updates. See #33 for more details.

Note: I only focused on ApiPrimaryValidator and ApiPrimaryDelegator for now.

Changes

Notes

I used the platform.getCurrentValidators docs as a reference to update the fields.

Tests from scripts/tests.unit.sh are passing which make me believe that my changes on the Ids deserializers have no impact

Nuttymoon commented 1 year ago

Rebased on main

Nuttymoon commented 1 year ago

@gyuho will you have a chance to take a look at the PR? Would be happy to implement any corrections if needed!

richardpringle commented 1 year ago

Hey @Nuttymoon, thanks for the contribution 🎉.

Could you provide a reproducible example(maybe on another branch) of how deserialization failed using ureq? We shouldn't need that extra allocation deserializing into a String first.

Thanks in advance!

Nuttymoon commented 1 year ago

Hey @Nuttymoon, thanks for the contribution 🎉.

Could you provide a reproducible example(maybe on another branch) of how deserialization failed using ureq? We shouldn't need that extra allocation deserializing into a String first.

Thanks in advance!

Hey @richardpringle ! Yes I guess I can create another branch here without the String deserialization and one on my repo (that uses ureq). I will ping you when I have done that!

Nuttymoon commented 1 year ago

Btw maybe my issue comes from the fact that I am using ureq for deserializing to a struct that contains String fields through serde. You can take a look at the workaround I had to implement here:

https://github.com/AshAvalanche/ash-rs/blob/df36348aab9fd6f1d9e10f5eb1af4f33b3882fe2/crates/ash_sdk/src/avalanche.rs#L42-L49

And how I deserialize:

https://github.com/AshAvalanche/ash-rs/blob/df36348aab9fd6f1d9e10f5eb1af4f33b3882fe2/crates/ash_sdk/src/avalanche/jsonrpc/platformvm.rs#L145-L152

(The deserialization process is currently being refactored but you get the idea)

gyuho commented 1 year ago

@Nuttymoon Thanks for the contribution! And apologies for the late response.

I cherry-picked your patch here https://github.com/ava-labs/avalanche-types-rs/pull/67.

And will cut a new cargo release, so you can try.

gyuho commented 1 year ago

Just published https://crates.io/crates/avalanche-types/0.0.379.

Please try and let us know if you find any issue.

richardpringle commented 1 year ago

@Nuttymoon, I see the issue with deserialization now. If you want to open a new PR, you can solve it with the following code (or something similar)

impl<'de> Deserialize<'de> for Id {
    fn deserialize<D>(deserializer: D) -> std::result::Result<Id, D::Error>
    where
        D: Deserializer<'de>,
    {
        struct StringOrStr;

        impl<'de> Visitor<'de> for StringOrStr {
            type Value = Cow<'de, str>;

            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
                formatter.write_str("borrowed or owned string")
            }

            fn visit_string<E>(self, v: String) -> std::result::Result<Self::Value, E>
            where
                E: serde::de::Error,
            {
                Ok(Cow::Owned(v))
            }

            fn visit_borrowed_str<E>(self, v: &'de str) -> std::result::Result<Self::Value, E>
            where
                E: serde::de::Error,
            {
                Ok(Cow::Borrowed(v))
            }
        }

        let s = deserializer.deserialize_any(StringOrStr)?;
        Id::from_str(&s).map_err(serde::de::Error::custom)
    }
}

The solution should also include a test using serde_json::from_value for deserialization.

The more complex solution is that we do not want to perform any unnecessary allocations. Just let me know if you would rather I write the fix and accompanying test, but I figured I would give you an opportunity to make the contribution since you're the one that found the limitation.

Nuttymoon commented 1 year ago

Hey, @richardpringle Thanks for your answer! I will open a new PR based on the code above. To be honest I am still learning Rust so I could not have come up with that on my own :sweat_smile: