Skip to content

Conversation

@LesnyRumcajs
Copy link
Member

@LesnyRumcajs LesnyRumcajs commented Jan 23, 2026

Summary of changes

Changes introduced in this pull request:

  • simplify some code with derive_more::FromStr

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced error messaging for invalid transaction hash inputs, providing clearer context when parsing fails.

✏️ Tip: You can customize this high-level summary in your review settings.

@LesnyRumcajs LesnyRumcajs requested a review from a team as a code owner January 23, 2026 17:45
@LesnyRumcajs LesnyRumcajs requested review from hanabi1224 and sudo-shashank and removed request for a team January 23, 2026 17:45
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 23, 2026

Walkthrough

The PR refactors FromStr trait implementations for EthAddress and EthHash by switching from manual implementations to derived implementations using derive_more::FromStr. Error handling in EthTraceTransaction is enhanced with contextual error messaging for invalid transaction hashes.

Changes

Cohort / File(s) Summary
FromStr derivation
src/rpc/methods/eth/types.rs
Added FromStr derive macro from derive_more for both EthAddress and EthHash; removed manual impl FromStr for EthAddress
FromStr cleanup
src/rpc/methods/eth.rs
Removed impl FromStr for EthHash block; updated EthTraceTransaction to wrap string parsing with .context("invalid transaction hash") for improved error reporting

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • hanabi1224
  • sudo-shashank
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: replacing manual FromStr implementations with derive_more::FromStr across the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Jan 23, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.70%. Comparing base (b1b1a2c) to head (b411b25).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/rpc/methods/eth.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/rpc/methods/eth/types.rs 67.03% <ø> (+<0.01%) ⬆️
src/rpc/methods/eth.rs 68.76% <0.00%> (+<0.01%) ⬆️

... and 36 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1b1a2c...b411b25. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Jan 23, 2026
Merged via the queue into main with commit 237583f Jan 23, 2026
34 checks passed
@LesnyRumcajs LesnyRumcajs deleted the derive-from-str branch January 23, 2026 22:37
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