Skip to content
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

Derive Serialize/Deserialize if possible #611

Closed
wants to merge 1 commit into from

Conversation

wtdcode
Copy link
Contributor

@wtdcode wtdcode commented Apr 27, 2024

Motivation

Derive Serialize/Deserialize if possible (with serde dep).

Solution

See diff.

PR Checklist

  • [ x ] Added Tests
    Not needed
  • [ x ] Added Documentation
    Not needed
  • [ x ] Breaking changes
    No

@wtdcode wtdcode requested a review from prestwich as a code owner April 27, 2024 15:38
@prestwich
Copy link
Member

We do not intend these values to be (de)serialized, as they do not correspond to any standard data forma. We aren't really interested in pushing a new standard. For now, all of these types should be in-memory only

Closing as not planned

@prestwich prestwich closed this Apr 27, 2024
@wtdcode
Copy link
Contributor Author

wtdcode commented Apr 27, 2024

We do not intend these values to be (de)serialized, as they do not correspond to any standard data forma. We aren't really interested in pushing a new standard. For now, all of these types should be in-memory only

Closing as not planned

I can't understand. It's mostly harmless.

@wtdcode
Copy link
Contributor Author

wtdcode commented Apr 27, 2024

https://github.com/rust-ethereum/ethabi/blob/b1710adc18f5b771d2d2519c87248b1ba9430778/ethabi/src/token/token.rs#L21

Note ethabi also has these. If alloy is intended as an alternative to ethers-rs, it should also enable these derives, no? See gakonst/ethers-rs#2667. Not having these derives prevents migration from ethers-rs to alloy/core.

Even just "in memory`, this enables it to represent in other format.

@wtdcode
Copy link
Contributor Author

wtdcode commented Apr 27, 2024

For tracking pupose, https://serde.rs/remote-derive.html also doesn't work:

error[E0277]: the trait bound `DynSolValue: Deserialize<'_>` is not satisfied
    --> -:21:15
     |
21   |         Tuple(Vec<DynSolValue>),
     |               ^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `DynSolValue`, which is required by `Vec<DynSolValue>: Deserialize<'_>`
     |
     = help: the following other types implement trait `Deserialize<'de>`:
               <bool as Deserialize<'de>>
               <char as Deserialize<'de>>
               <isize as Deserialize<'de>>
               <i8 as Deserialize<'de>>
               <i16 as Deserialize<'de>>
               <i32 as Deserialize<'de>>
               <i64 as Deserialize<'de>>
               <i128 as Deserialize<'de>>
             and 493 others
     = note: required for `Vec<DynSolValue>` to implement `Deserialize<'_>`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants