Skip to content

feat(eth): update Ethereum API methods to return pointers for consistent nil handling #13150

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

virajbhartiya
Copy link
Member

This PR partially addresses issue #13043 by updating several Ethereum API methods to return pointers instead of values, enabling proper nil handling when blocks or transactions are not found. This change aligns with go-ethereum patterns and improves error handling for not found scenarios. Methods updated: EthGetBalance, EthGetBlockByHash, EthGetBlockByNumber, EthGetBlockTransactionCountByHash, EthGetBlockTransactionCountByNumber, EthGetTransactionCount, and EthFeeHistory. Note: This is a partial implementation - additional methods will be addressed in follow-up PRs.

@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz May 30, 2025
@virajbhartiya
Copy link
Member Author

Hey @rvagg
I've implemented:

  • EthGetBlockTransactionCountByNumber/Hash*ethtypes.EthUint64
  • EthGetBlockByHash/Number*ethtypes.EthBlock
  • EthGetTransactionCount*ethtypes.EthUint64
  • EthGetBalance*ethtypes.EthBigInt
  • EthFeeHistory*ethtypes.EthFeeHistory

Just wanted to check if this is heading in the right direction before I continue with EthEstimateGas and the "ignore not found errors" logic. Does the approach look good to you?

@virajbhartiya virajbhartiya requested a review from rvagg May 30, 2025 05:29
@rjan90 rjan90 moved this from 📌 Triage to 🔎 Awaiting Review in FilOz May 30, 2025
@rjan90 rjan90 requested a review from masih June 3, 2025 11:49
@masih
Copy link
Member

masih commented Jun 3, 2025

@virajbhartiya Please review the failing CI workflows. Looks like we are missing a make gen.

@virajbhartiya
Copy link
Member Author

Hey @masih , I've fixed the make gen issue, but I am not sure if CI is failing because of the changes made in this PR

@masih
Copy link
Member

masih commented Jun 5, 2025

I am not sure if CI is failing because of the changes made in this PR

At the very least, some CI workflows are failing because of the changes made in this PR. I would:

  • start by looking at lint issues.
  • then move on to looking at eth test failures. This one is a good one to start with.

Make sure tests pass locally before investigating any further false negative CI workflow failures.

@BigLep
Copy link
Member

BigLep commented Jun 10, 2025

@virajbhartiya : there were some itest fixes that checked into master. Can you update your fork to pull those in? I tried rerunning the failed jobs, but I'm still seeing a couple that I believe have been fixed.

@BigLep BigLep moved this from 🔎 Awaiting Review to ⌨️ In Progress in FilOz Jun 23, 2025
@BigLep
Copy link
Member

BigLep commented Jun 23, 2025

@virajbhartiya : it looks like there are a couple of tests consistently failing even after you updated from filecoin-project:master: https://github.com/filecoin-project/lotus/actions/runs/15552300377/job/44641706836?pr=13150

Are you able to look into this?

Comment on lines +105 to +110
// According to go-ethereum patterns, "not found" errors should lead to (nil, nil).
// Other errors should result in (nil, err).
if errors.Is(err, &api.ErrNullRound{}) || ipld.IsNotFound(err) {
return nil, nil
}
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

nice, do we happen to have a test for this?

Copy link
Member

Choose a reason for hiding this comment

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

actually I see this pattern is the same one you're using in most places - but I do think basic tests for these would be good, maybe in eth_api_test.go, or fevm_test.go even, let me know if you need guidance in setting something up, it should be relatively straightforward once you establish a pattern

@rvagg
Copy link
Member

rvagg commented Jun 25, 2025

Sorry for the delay in getting to look at this. It's looking fairly good @virajbhartiya. I think the next steps before landing this would be:

  • Look at that one failing CI test above and see if it's related; I think it might be: go test ./itests/eth_api_f3_test.go to reproduce it locally and verify
  • Rebase on master (or merge master into this)
  • Let's figure out how to get some testing in. There's API tests in eth_api_test.go that we should be able to logically squeeze this into, we just need to have good call patterns that trigger these nil values. I haven't looked in detail but please let me know if you find it a challenge to get some tests in and I'll jump in and figure something out.
  • Reviewer (likely me) just needs to cross-reference and double-check that we're doing the go-ethereum compatible thing on all of these API methods 🤞 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ⌨️ In Progress
Development

Successfully merging this pull request may close these issues.

4 participants