-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implement the eth_getFilterLogs
endpoint
#217
Conversation
WalkthroughThe update focuses on enhancing the Changes
Assessment against linked issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
5bce7fa
to
bb2e508
Compare
|
||
filter, ok := api.filters[id] | ||
if !ok { | ||
return nil, errors.Join( |
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.
according to the above description it should return empty array if not found, I suggest adding a test where the filter is not found as well
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.
Good point regarding the test case, I will add it right away.
Good catch as well regarding the description. This is actually a wrong description copied from geth, See https://github.com/ethereum/go-ethereum/blob/8d42e115b1cae4f09fd02b71c06ec9c85f22ad4f/eth/filters/api.go#L383-L417 😅 I will update the description, the geth behavior is to always return nil
, instead of an empty array, at least for this specific endpoint.
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.
Updated in d2952b8
bb2e508
to
d2952b8
Compare
let contractAddress = deployed.receipt.contractAddress | ||
|
||
// create logs filter on the address of the deployed contract | ||
let response = await callRPCMethod('eth_newFilter', [{ 'address': contractAddress }]) |
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.
why we don't use the web3.js client 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.
Unfortunately web3.eth does not expose any methods for working with filters. So the only way around was to simply create a POST HTTP call.
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.
Actionable comments posted: 3
const abi = require(`../fixtures/${name}ABI.json`) | ||
const code = await fs.promises.readFile(`${__dirname}/../fixtures/${name}.byte`, 'utf8') | ||
const contractABI = new web3.eth.Contract(abi) | ||
|
||
let data = contractABI | ||
.deploy({ data: `0x${code}` }) | ||
.encodeABI() | ||
|
||
let signed = await conf.eoa.signTransaction({ | ||
from: conf.eoa.address, | ||
data: data, | ||
value: '0', | ||
gasPrice: '0', | ||
}) | ||
|
||
let receipt = await web3.eth.sendSignedTransaction(signed.rawTransaction) | ||
let receipt = await web3.eth.sendSignedTransaction(signed.rawTransaction) | ||
|
||
return { | ||
contract: new web3.eth.Contract(abi, receipt.contractAddress), | ||
receipt: receipt | ||
} | ||
return { | ||
contract: new web3.eth.Contract(abi, receipt.contractAddress), | ||
receipt: receipt | ||
} |
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.
Consider adding explicit error handling for operations like file reading and contract deployment to enhance robustness.
const signedTx = await conf.eoa.signTransaction(tx) | ||
// send transaction and make sure interaction was success | ||
const receipt = await web3.eth.sendSignedTransaction(signedTx.rawTransaction) | ||
|
||
return { | ||
hash: signedTx.transactionHash, | ||
receipt: receipt, | ||
} |
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.
Add explicit error handling for the network operations to ensure robustness, especially in case of network failures.
// callRPCMethod accepts a method name and its params and | ||
// makes a POST request to the JSON-RPC API server. | ||
// Returns a promise for the response. | ||
async function callRPCMethod(methodName, params) { | ||
return chai.request('http://127.0.0.1:8545') | ||
.post('/') | ||
.set('Content-Type', 'application/json') | ||
.set('Accept', 'application/json') | ||
.send({ | ||
'jsonrpc': '2.0', | ||
'method': methodName, | ||
'id': 1, | ||
'params': params | ||
}) |
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.
Consider adding error handling for the HTTP requests to handle potential issues like network errors or invalid responses.
Closes: #214
Description
Example call:
Example result:
For contributor use:
master
branchFiles changed
in the Github PR explorerSummary by CodeRabbit
New Features
eth_getFilterLogs
, deploy contracts, make contract function calls, and verify logs in a testing environment.Bug Fixes
Tests
eth_getFilterLogs
to verify functionality and robustness.Documentation