Skip to content

Conversation

derekpierre
Copy link
Member

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:

High-level idea of the changes introduced in this PR.
List relevant API changes (if any), as well as related PRs and issues.

Issues fixed/closed:

  • Fixes #...

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 36.88784% with 799 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.70%. Comparing base (e080d33) to head (5490090).

Files with missing lines Patch % Lines
nucypher-core-python/src/lib.rs 0.00% 523 Missing ⚠️
nucypher-core/src/signature_request.rs 61.01% 276 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@derekpierre derekpierre marked this pull request as draft September 11, 2025 17:55
#[serde(with = "serde_bytes::as_base64")]
pub call_data: Box<[u8]>,
/// Gas limit for the call
pub call_gas_limit: u128,
Copy link
Member Author

@derekpierre derekpierre Sep 12, 2025

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

^ @KPrasch any thoughts here?

Copy link
Member

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.

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.

3 participants