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.

Sign up for GitHub

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

Jump to bottom

fix: deserialization of null storage keys in AccessListItem #955

Merged
merged 5 commits into from
Jun 21, 2024
Merged

fix: deserialization of null storage keys in AccessListItem #955

merged 5 commits into from
Jun 21, 2024

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented Jun 21, 2024

Motivation

We recently adopted the AccessList and AccessListItem types for EDR/Hardhat. This fixes an issue when deserializing an AccessListItem where the null value for storage keys was not accepted.

Solution

Added a custom serde deserialization handler.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@Wodann Wodann changed the title Fix/access list null storage keys fix: deserialization of null storage keys in AccessListItem Jun 21, 2024
mattsse
mattsse requested changes Jun 21, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

supportive, the spec does not properly specify this but I think it's reasonable to interpret null as [].

crates/serde/src/optional.rs Outdated Show resolved Hide resolved
crates/serde/src/optional.rs Outdated Show resolved Hide resolved
@Wodann Wodann requested a review from mattsse June 21, 2024 19:00
mattsse
mattsse requested changes Jun 21, 2024
View reviewed changes
crates/eips/src/eip2930.rs Outdated Show resolved Hide resolved
@mattsse mattsse added the enhancement New feature or request label Jun 21, 2024
@Wodann @mattsse
Update crates/eips/src/eip2930.rs
4af7f9f 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
DaniPopes
DaniPopes requested changes Jun 21, 2024
View reviewed changes
crates/serde/src/lib.rs Outdated Show resolved Hide resolved
@Wodann Wodann requested review from DaniPopes and mattsse June 21, 2024 21:26
mattsse
mattsse approved these changes Jun 21, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty

@Wodann
Copy link
Contributor Author

Wodann commented Jun 21, 2024

Thank you for your reviews!

What timeline do alloy releases normally follow?

@mattsse
Copy link
Member

mattsse commented Jun 21, 2024

likely when we have #950 in

@mattsse mattsse merged commit 194f266 into alloy-rs:main Jun 21, 2024
22 checks passed
@Wodann Wodann deleted the fix/access-list-null-storage-keys branch June 25, 2024 15:10
@jtguibas
Copy link
Contributor

jtguibas commented Jul 15, 2024 •
edited
Loading

This PR unfortunately makes it so all types inheriting AccessListItem cannot be serialized/deserialized with serializers like bincode.

I think a good solution could be to instead make the type Option<Vec<B256>> for storage keys, if we really want to support null deserialization.

I could make a PR to fix this if that sounds good @mattsse?

@mattsse
Copy link
Member

mattsse commented Jul 15, 2024 •
edited
Loading

hey @Wodann please remind me, why should a null value be supported? I believe a storagekey field should always be required:

https://github.com/ethereum/go-ethereum/blob/37590b2c5579c36d846c788c70861685b0ea240e/core/types/tx_access_list.go#L34-L35

jtguibas added a commit to jtguibas/alloy that referenced this pull request Jul 15, 2024
@jtguibas
fix: broken bincode serialization from alloy-rs#955
d138cd9
@jtguibas jtguibas mentioned this pull request Jul 15, 2024
Merged
3 tasks
mattsse added a commit that referenced this pull request Jul 16, 2024
@jtguibas @mattsse
fix: require storageKeys value broken bincode serialization from #955 (…
1d0c81d 
…#1058)

* fix: broken bincode serialization from #955

* remove optional in alloy-serde

* resolve matt's comments

* get rid of broken test

* fix cargo fmt

* enforce storage key

---------

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@Wodann
Copy link
Contributor Author

Wodann commented Jul 17, 2024

hey @Wodann please remind me, why should a null value be supported? I believe a storagekey field should always be required:

https://github.com/ethereum/go-ethereum/blob/37590b2c5579c36d846c788c70861685b0ea240e/core/types/tx_access_list.go#L34-L35

Historically, Hardhat allows specification of the null value in storage keys to signal the same thing as an empty array of storage keys.

It could very well be that the spec has changed since. If this change is causing issues, we're happy to perform a breaking change in Hardhat's JSON-RPC spec.

@Wodann
Copy link
Contributor Author

Wodann commented Jul 23, 2024

We decided to remove support for null lists to match geth and alloy.

ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
@Wodann @mattsse
fix: deserialization of null storage keys in AccessListItem (alloy-rs…
3dbc953 
…#955)

* fix: deserialization of null storage keys in AccessListItem

* test: add test

* misc: apply review suggestions

* Update crates/eips/src/eip2930.rs

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

* misc: apply review suggestion

---------

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
@jtguibas @mattsse
fix: require storageKeys value broken bincode serialization from allo…
8dba740 
…y-rs#955 (alloy-rs#1058)

* fix: broken bincode serialization from alloy-rs#955

* remove optional in alloy-serde

* resolve matt's comments

* get rid of broken test

* fix cargo fmt

* enforce storage key

---------

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
j75689 pushed a commit to bnb-chain/alloy that referenced this pull request Aug 1, 2024
@jtguibas @mattsse @j75689
fix: require storageKeys value broken bincode serialization from allo…
c7a9311 
…y-rs#955 (alloy-rs#1058)

* fix: broken bincode serialization from alloy-rs#955

* remove optional in alloy-serde

* resolve matt's comments

* get rid of broken test

* fix cargo fmt

* enforce storage key

---------

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaniPopes DaniPopes DaniPopes approved these changes

@mattsse mattsse mattsse approved these changes

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@prestwich prestwich Awaiting requested review from prestwich

@Evalir Evalir Awaiting requested review from Evalir

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Wodann @mattsse @jtguibas @DaniPopes