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

Add custom serialization for Address #742

Merged

Conversation

jenpaff
Copy link
Contributor

@jenpaff jenpaff commented Sep 20, 2024

Motivation

Partially closes foundry-rs/foundry#8715
We want to serialize Address type in checksum format.

Solution

Solution is to implement a custom serialization function for Address type.

I ran into some issues:

  1. The impl_serde macro conflicts with impl Serialize for Address (Address is wrapped with the wrap_fixed_bytes macro which calls impl_serde), so I added a workaround in impl_serde to bypass Address type. Another solution would be to just move the impl directly into the macro- if preferred.

  2. There's another type EIP712Domain which uses the type Address, I wanted to double check that it is okay for this to also be serialized in checksum format, I added another unit test to make it explicit.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@zerosnacks zerosnacks self-requested a review September 20, 2024 11:50
@zerosnacks
Copy link
Member

zerosnacks commented Sep 20, 2024

  1. Implementation looks good, I don't think the NOOP behavior in the macro is a problem but documenting it would be good. Perhaps there is a better way of handling this.
  2. It doesn't look like EIP712 enforces EIP-55, and nothing specifically relating to the verifyingContract field. From preliminary tests it looks like checksumming was already handled here so the resulting hash after the proposed changes should stay the same - worth double checking.

cc @DaniPopes please correct me if I'm wrong

Tip: for CI fmt task failing run: cargo +nightly fmt --all --check

@jenpaff jenpaff force-pushed the jenpaff/use-EIP-55-checksummed-address branch 2 times, most recently from ba8dcb1 to bc55d26 Compare September 20, 2024 13:23
@jenpaff jenpaff force-pushed the jenpaff/use-EIP-55-checksummed-address branch from bc55d26 to 1352007 Compare September 20, 2024 13:57
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nits

Address override in impl_serde! is fine, I can't think of a better way to do this without having to restructure wrap_fixed_bytes!.

crates/primitives/src/bits/serde.rs Outdated Show resolved Hide resolved
crates/primitives/src/bits/serde.rs Outdated Show resolved Hide resolved
@mattsse mattsse marked this pull request as ready for review September 20, 2024 17:15
jenpaff and others added 2 commits September 20, 2024 19:59
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@DaniPopes DaniPopes merged commit 4b823f7 into alloy-rs:main Sep 20, 2024
31 checks passed
mattsse added a commit that referenced this pull request Oct 8, 2024
DaniPopes pushed a commit that referenced this pull request Oct 8, 2024
Revert "Add custom serialization for Address (#742)"

This reverts commit 4b823f7.
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.

feat(compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs
3 participants