Skip to content
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

feat: implement methods for eth_getTransactionByBlockNumberAndIndex and eth_getTransactionByBlockHashAndIndex #12618

Merged
merged 13 commits into from
Oct 25, 2024

Conversation

virajbhartiya
Copy link
Member

Closes #10640

Proposed Changes

Implement methods for eth_getTransactionByBlockNumberAndIndex and `eth_getTransactionByBlockHashAndIndex

Checklist

Before you mark the PR ready for review, please make sure that:

@virajbhartiya virajbhartiya changed the title feat: implement methods for eth_getTransactionByBlockNumberAndIndex and `eth_getTransactionByBlockHashAndIndex feat: implement methods for eth_getTransactionByBlockNumberAndIndex and eth_getTransactionByBlockHashAndIndex Oct 20, 2024
node/impl/full/eth.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Oct 21, 2024

These should go in api_gateway.go along with the other Eth methods in there - they are reasonable to expose publicly. Once you do that you'll get some compile errors. Go to gateway/proxy_eth.go and have a look at how the methods that take a blkNum argument handle it - you want to checkTipsetHeight at some point, either directly or via checkEthBlockParam--I think we only need the latter if we're supporting the string predefined variants ('latest' etc.) (are we with these?). The main point is that users of the gateway need to be able to set an epoch lookback limit on queires and queries that resolve to tipsets older than that limit should error.

itests/fevm_test.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
@aarshkshah1992 aarshkshah1992 self-requested a review October 21, 2024 06:33
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

@virajbhartiya Reviewed. good stuff.

@virajbhartiya
Copy link
Member Author

@rvagg @aarshkshah1992 I have addressed the above reviews. Both the tests have been merged into one. The EthGetTransactionByBlockNumberAndIndex can now accept either hexadecimal values or "latest", "pending", etc. I have also made the changes in getTransactionByTipsetAndIndex by removing the MsgLookup since the tipset is already there in the list of messages. Thanks for pointing me out to the resources!

CHANGELOG.md Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
@virajbhartiya
Copy link
Member Author

@rvagg @aarshkshah1992 I have addressed the comments above. used Xerrors isntead of fmt. Added rate and lookback limits for both the methods, and added tests for both - SigTypeSecp256k1 and SigTypeBLS messages.

@virajbhartiya virajbhartiya requested a review from rvagg October 22, 2024 12:18
itests/fevm_test.go Outdated Show resolved Hide resolved
@virajbhartiya virajbhartiya requested a review from rvagg October 23, 2024 05:19
node/impl/full/eth.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

looking good! almost there, just some minor suggestions left

@virajbhartiya virajbhartiya requested a review from rvagg October 24, 2024 04:45
node/impl/full/eth.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

all good except for those ErrNullRound inclusions, remove those and we'll merge this

@virajbhartiya
Copy link
Member Author

@rvagg Removed the null round checks for both thee functions

@virajbhartiya virajbhartiya requested a review from rvagg October 24, 2024 12:22
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

Support eth_getTransactionByBlockNumberAndIndex and eth_getTransactionByBlockHashAndIndex
3 participants