Numeric type audit: network, consensus, provider, rpc-types #454

yash-atreya · 2024-04-03T18:01:41Z

Motivation

Solution

gas_limit -> u128
gas -> u128
gas_price -> u128
max_fee_per_blob_gas -> u128
transaction_type -> u8
max_fee_per_gas -> u128
max_priority_fee_per_gas -> u128
Consequently, changed fn args and return values accordingly.
Satisfies clippy, tests will fail.

PR Checklist

Added Tests
Added Documentation
Breaking changes

* `gas_limit` -> `u128` * `gas` -> `u128` * `gas_price` -> `u128` * `max_fee_per_blob_gas` -> `u128` * `transaction_type` -> `u8` * `max_fee_per_gas` -> `u128` * `max_priority_fee_per_gas` -> `u128` * Consequently, changed fn args and return values accordingly. * Satisfies clippy, tests will fail.

* add u128_hex_or_decimal_opt serde

mattsse

I think all of these make sense, primitive types are preferable imo.
gas being u128 should make some operations easier but should not affect math for any call arguments etc

Evalir

This mostly looks good to me, I'd just like to ensure we get the TODOs taken care of and add enough tests to be 100% sure we're not breaking serde

crates/provider/src/layers/gas.rs

crates/provider/src/provider.rs

crates/serde/src/num.rs

crates/rpc-types/src/eth/transaction/tx_type.rs

crates/provider/src/utils.rs

…d methods * add u128_vec serde methods

prestwich

just style nit for @mattsse. several more instances that i didn't explicitly mark

prestwich · 2024-04-04T17:52:51Z

crates/consensus/src/transaction/eip2930.rs

@@ -29,8 +29,8 @@ pub struct TxEip2930 {
    /// this transaction. This is paid up-front, before any
    /// computation is done and may not be increased
    /// later; formally Tg.
-    #[cfg_attr(feature = "serde", serde(with = "alloy_serde::u64_hex_or_decimal"))]
-    pub gas_limit: u64,
+    #[cfg_attr(feature = "serde", serde(with = "alloy_serde::u128_hex_or_decimal"))]


do any endpoints use decimal here? in other PRs we've avoided allowing decimal when nobody actually uses it

prestwich · 2024-04-04T17:58:23Z

crates/rpc-types/src/eth/transaction/mod.rs

+    #[serde(
+        default,
+        skip_serializing_if = "Option::is_none",
+        with = "alloy_serde::num::u128_hex_or_decimal_opt"


same question. pretty sure decimal would be illegal rpc response here? I don't mind tolerating it, but @mattsse asked for removal of decimal in other PRs

+ 1 to removing decimal support

makes sense, let's enforce strict quantity rules everywhere for core eth types

side note, the U*** type deser DO accept decimal. so this is technically a breaking behavior change

prestwich · 2024-04-05T03:36:18Z

followup work: #465

…#454) * num_type_audit: network, consensus, provider, rpc-types * `gas_limit` -> `u128` * `gas` -> `u128` * `gas_price` -> `u128` * `max_fee_per_blob_gas` -> `u128` * `transaction_type` -> `u8` * `max_fee_per_gas` -> `u128` * `max_priority_fee_per_gas` -> `u128` * Consequently, changed fn args and return values accordingly. * Satisfies clippy, tests will fail. * fix(rpc-type): transaction requests tests * fix(rpc-types): tests - transaction type * add u128_hex_or_decimal_opt serde * fix(providers): provider::tests * fix(providers): layers::gas::tests * fix: docs ci * fix(provider): estimate_gas should return u128 * add(serde): unit test for u128_hex_or_decimal_opt * fix(rpc-types, provider): use primitives in FeeHistory and its related methods * add u128_vec serde methods * ci nits * nit

yash-atreya changed the title ~~num_type_audit: network, consensus, provider, rpc-types~~ Numeric type audit: network, consensus, provider, rpc-types Apr 3, 2024

yash-atreya added 5 commits April 3, 2024 15:07

fix(rpc-type): transaction requests tests

431dba1

fix(rpc-types): tests - transaction type

d1da42d

* add u128_hex_or_decimal_opt serde

fix(providers): provider::tests

e2093a9

fix(providers): layers::gas::tests

3be25b2

fix: docs ci

3e1d4d5

yash-atreya marked this pull request as ready for review April 3, 2024 20:36

yash-atreya requested review from DaniPopes, gakonst, mattsse, onbjerg, prestwich and Evalir as code owners April 3, 2024 20:36

mattsse approved these changes Apr 4, 2024

View reviewed changes

yash-atreya added 2 commits April 4, 2024 09:43

Merge branch 'main' into num_type_audit

824f7be

Merge branch 'main' into num_type_audit

d4cb5aa

Evalir reviewed Apr 4, 2024

View reviewed changes

crates/provider/src/layers/gas.rs Outdated Show resolved Hide resolved

crates/provider/src/provider.rs Outdated Show resolved Hide resolved

crates/serde/src/num.rs Outdated Show resolved Hide resolved

yash-atreya added 2 commits April 4, 2024 12:15

fix(provider): estimate_gas should return u128

4766a49

add(serde): unit test for u128_hex_or_decimal_opt

e77a4ff

gakonst requested changes Apr 4, 2024

View reviewed changes

crates/rpc-types/src/eth/transaction/tx_type.rs Outdated Show resolved Hide resolved

crates/provider/src/utils.rs Outdated Show resolved Hide resolved

yash-atreya added 3 commits April 4, 2024 13:33

fix(rpc-types, provider): use primitives in FeeHistory and its relate…

3bcbfda

…d methods * add u128_vec serde methods

ci nits

889e57a

nit

eb43370

prestwich approved these changes Apr 4, 2024

View reviewed changes

prestwich mentioned this pull request Apr 5, 2024

[Refactor] Remove _or_decimal serde encoding from integers in rpc-types #465

Closed

prestwich merged commit 5c7e246 into alloy-rs:main Apr 5, 2024
18 checks passed

prestwich mentioned this pull request Apr 5, 2024

feat: std feature flag for alloy-consensus #461

Merged

yash-atreya mentioned this pull request Apr 6, 2024

num-audit: extension #474

Merged

3 tasks

klkvr mentioned this pull request Aug 30, 2024

eip2930: use alloy TxEip2930 paradigmxyz/reth#10623

Merged

yash-atreya mentioned this pull request Sep 9, 2024

[Feature] Make some gas fields u64 for Header #1241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Numeric type audit: network, consensus, provider, rpc-types #454

Numeric type audit: network, consensus, provider, rpc-types #454

yash-atreya commented Apr 3, 2024 •

edited

Loading

mattsse left a comment

Evalir left a comment

prestwich left a comment •

edited

Loading

prestwich Apr 4, 2024

prestwich Apr 4, 2024

Evalir Apr 4, 2024 •

edited

Loading

mattsse Apr 4, 2024

prestwich Apr 4, 2024

prestwich commented Apr 5, 2024

Numeric type audit: network, consensus, provider, rpc-types #454

Numeric type audit: network, consensus, provider, rpc-types #454

Conversation

yash-atreya commented Apr 3, 2024 • edited Loading

Motivation

Solution

PR Checklist

mattsse left a comment

Choose a reason for hiding this comment

Evalir left a comment

Choose a reason for hiding this comment

prestwich left a comment • edited Loading

Choose a reason for hiding this comment

prestwich Apr 4, 2024

Choose a reason for hiding this comment

prestwich Apr 4, 2024

Choose a reason for hiding this comment

Evalir Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse Apr 4, 2024

Choose a reason for hiding this comment

prestwich Apr 4, 2024

Choose a reason for hiding this comment

prestwich commented Apr 5, 2024

yash-atreya commented Apr 3, 2024 •

edited

Loading

prestwich left a comment •

edited

Loading

Evalir Apr 4, 2024 •

edited

Loading