-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
feat(eth): update Ethereum API methods to return pointers for consistent nil handling #13150
Conversation
…dling - addresses issue 13043 (partial)
Hey @rvagg
Just wanted to check if this is heading in the right direction before I continue with |
@virajbhartiya Please review the failing CI workflows. Looks like we are missing a |
…turn pointers for nil handling
Hey @masih , I've fixed the |
At the very least, some CI workflows are failing because of the changes made in this PR. I would:
Make sure tests pass locally before investigating any further false negative CI workflow failures. |
…dereference pointers for correct comparisons
@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. |
@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? |
// 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 |
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.
nice, do we happen to have a test for this?
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.
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
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:
|
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.