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

num-audit: extension #474

Merged
merged 13 commits into from
Apr 8, 2024
Merged

num-audit: extension #474

merged 13 commits into from
Apr 8, 2024

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Apr 6, 2024 •
edited
Loading

Motivation

Extension of #454

Solution

  • gas related fields to u128
  • nonce and number to u64 in Header from B64 and U256 respectively

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@yash-atreya
num-audit: extension
6de3f42 
* gas related field to `u128`
* `nonce` and `number` to `u64` in `Header` from `B64` and `U256` respectively
@klkvr
Copy link
Member

klkvr commented Apr 6, 2024

rpc_types::TransactionReceipt also has several u64 gas price fields

@klkvr
Copy link
Member

klkvr commented Apr 6, 2024 •
edited
Loading

I am experiencing serde-rs/json#625 on latest alloy while deserializing WithOtherFields<TransactionRequest>, so I think we should ensure that all u128 fields have a with = u128_hex_or_decimal or similar alternative even if we only serialize/deserialize it as a decimal

mattsse
mattsse requested changes Apr 6, 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.

very supportive
unifying all of them is necessary, just need to ensure we keep hex support

crates/consensus/src/header.rs Outdated Show resolved Hide resolved
crates/eips/src/eip1559/basefee.rs Show resolved Hide resolved
crates/rpc-types/src/eth/block.rs Outdated Show resolved Hide resolved
crates/rpc-types/src/eth/block.rs Outdated Show resolved Hide resolved
@klkvr klkvr mentioned this pull request Apr 7, 2024
Merged
1 task
@yash-atreya yash-atreya requested review from prestwich and mattsse April 8, 2024 18:21
prestwich
prestwich approved these changes Apr 8, 2024
View reviewed changes
crates/serde/src/num.rs Outdated Show resolved Hide resolved
mattsse
mattsse requested changes Apr 8, 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.

last pedantic serde nits

crates/rpc-types/src/eth/block.rs Outdated Show resolved Hide resolved
crates/rpc-types/src/eth/block.rs Outdated Show resolved Hide resolved
klkvr
klkvr reviewed Apr 8, 2024
View reviewed changes
crates/genesis/src/lib.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya requested review from mattsse and prestwich April 8, 2024 20:31
@prestwich
Copy link
Member

#465 actually somewhat superseded by the newer issue wrt serde helpers redundant code

@yash-atreya
Copy link
Member Author

#465 actually somewhat superseded by the newer issue wrt serde helpers redundant code

Yes, the PR that addresses #479 will close #465

@yash-atreya yash-atreya requested review from mattsse and klkvr April 8, 2024 20:55
@prestwich
Copy link
Member

#465 actually somewhat superseded by the newer issue wrt serde helpers redundant code

Yes, the PR that addresses #479 will close #465

not exactly because moving to via_ruint canonizes the tolerant behavior, where #465 would require significant changes to reject the invalid-but-tolerated objects

Given how much surface area its add, my current preference is to leave everything tolerant

@mattsse
Copy link
Member

mattsse commented Apr 8, 2024

Given how much surface area its add, my current preference is to leave everything tolerant

hmm, for response types, being tolerant is actually beneficial because the user doesn't really care how the server formats it

mattsse
mattsse approved these changes Apr 8, 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.

per above,
also, supporting decimals on deserialize is okay and perhaps even preferable, in case servers don't follow QUANTITY

@mattsse mattsse merged commit cd5a2c7 into alloy-rs:main Apr 8, 2024
18 checks passed
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
@yash-atreya
num-audit: extension (alloy-rs#474)
0b9e4d3 
* num-audit: extension
* gas related field to `u128`
* `nonce` and `number` to `u64` in `Header` from `B64` and `U256` respectively

* num-audit: gas fields to `u128` in Genesis

* num-audit: use primitive types in rpc-types: common, optimism, receipt, request

* fix(rpc-types): request tests

* fix(rpc-types): optimism tests

* add serde attr and fix rpc-types tests

* timestamp -> `u64` in rpc-types `block`

* nit

* serde attr nits

* skip_serializing nits

* rpc-types: make `gas_used` required in receipt

* fix: gas filler tests
@yash-atreya yash-atreya mentioned this pull request Sep 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

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

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

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

@Evalir Evalir Awaiting requested review from Evalir

@prestwich prestwich Awaiting requested review from prestwich

@klkvr klkvr Awaiting requested review from klkvr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yash-atreya @klkvr @prestwich @mattsse