-
Notifications
You must be signed in to change notification settings - Fork 12
[WIP] Common objects for Signing #113
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 30.05% 32.70% +2.64%
==========================================
Files 18 19 +1
Lines 2349 3605 +1256
==========================================
+ Hits 706 1179 +473
- Misses 1643 2426 +783 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
68bb669
to
c5c1cc7
Compare
#[serde(with = "serde_bytes::as_base64")] | ||
pub call_data: Box<[u8]>, | ||
/// Gas limit for the call | ||
pub call_gas_limit: u128, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KPrasch , do we have some mismatched types here?
I thought this value and others are supposed to be uint256
(based on the spec - https://eips.ethereum.org/EIPS/eip-4337) but are only represented as u128? My understanding is that Rust built-in types only go up to u128.
- Do we need some other library that provides uint256 functionality, or create a type ourselves? eg. https://docs.rs/primitive-types/0.13.1/primitive_types/
- Did you ever look into whether there are existing Rust libraries that already have a representation for
UserOperation
that we can leverage, as opposed to writing our own? For eg. https://docs.rs/alloy-rpc-types-eth/latest/alloy_rpc_types_eth/ or https://github.com/alchemyplatform/rundler. - I haven't looked into it, just noticed it in a search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ @KPrasch any thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2¹²⁸ ≈ 3.4 × 10³⁸ is an astronomically large number :) Ethereum mainnet block gas limit (as of 2025) is on the order of 30 million gas per block (~3 × 10⁷). So even if gas limits grew a trillion-fold, you’d still be nowhere near 2¹²⁸.
From a practical standpoint, u128 is vastly more than enough to represent gas limits or call gas. However, the EIP-4337 spec does defines them as uint256.
That being said, alloy does look like a promising tool here if we want to accept a new dependency.
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: