search

coinbase / mesh-ethereum

Ethereum Mesh API Implementation
Apache License 2.0
102 stars 70 forks source link

feat(call): Add eth_getBlockByNumber support to /call endpoint #69

Closed akramhussein closed 2 years ago

akramhussein commented 2 years ago

Motivation

This PR adds support for eth_getBlockByNumber being called via the /call endpoint.

Solution

Open questions

None

akramhussein commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

shrimalmadhur commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

I just realized that the other method in Call function calls a separate function rather than directly calling CallContext like on line 1224 ec.transactionReceipt(ctx, common.HexToHash(input.TxHash)) - Can me make it consistent. And also we might want to add a test for Invalid Argument like TestCall_InvalidArgs one. I think then we are good to go.

shrimalmadhur commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

Also all the commits will need to be GPG signed and verified before I can merge that it.

akramhussein commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

Also all the commits will need to be GPG signed and verified before I can merge that it.

This should be done now - squashed the commits into 1 and using gpg now. Please can you confirm?

shrimalmadhur commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

I just realized that the other method in Call function calls a separate function rather than directly calling CallContext like on line 1224 ec.transactionReceipt(ctx, common.HexToHash(input.TxHash)) - Can me make it consistent. And also we might want to add a test for Invalid Argument like TestCall_InvalidArgs one. I think then we are good to go.

@akramhussein Can you also refactor and add a test for Invalid args?

akramhussein commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

I just realized that the other method in Call function calls a separate function rather than directly calling CallContext like on line 1224 ec.transactionReceipt(ctx, common.HexToHash(input.TxHash)) - Can me make it consistent. And also we might want to add a test for Invalid Argument like TestCall_InvalidArgs one. I think then we are good to go.

Working on this now. When I started the PR I tried to use existing functions like:

https://github.com/coinbase/rosetta-ethereum/blob/9a3846484a33d9354159cb2d59eb0b10f3ace793/ethereum/client.go#L212

This parses the response into a different structure with additional calls.

This drops some data fields - if you look at the Header struct from go-ethereum it's missing some fields found in the eth_getBlockByNumber response - see this test data file for example of the response

The first isn't right since call is meant to be a pass-through function. The second would require me to re-factor it to allow the parameter value for show_transaction_details be passed in and use a generic map which may break other parts of the code.

Do you have any thoughts on best way forward?

shrimalmadhur commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

I just realized that the other method in Call function calls a separate function rather than directly calling CallContext like on line 1224 ec.transactionReceipt(ctx, common.HexToHash(input.TxHash)) - Can me make it consistent. And also we might want to add a test for Invalid Argument like TestCall_InvalidArgs one. I think then we are good to go.

Working on this now. When I started the PR I tried to use existing functions like:

https://github.com/coinbase/rosetta-ethereum/blob/9a3846484a33d9354159cb2d59eb0b10f3ace793/ethereum/client.go#L212

This parses the response into a different structure with additional calls.

This drops some data fields - if you look at the Header struct from go-ethereum it's missing some fields found in the eth_getBlockByNumber response - see this test data file for example of the response

The first isn't right since call is meant to be a pass-through function. The second would require me to re-factor it to allow the parameter value for show_transaction_details be passed in and use a generic map which may break other parts of the code.

Do you have any thoughts on best way forward?

Maybe a best way is to have a separate function for getBlockByNumber which returns map[string]interface{} and don't touch the existing function. Eventually Block function might use this function.

akramhussein commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

I just realized that the other method in Call function calls a separate function rather than directly calling CallContext like on line 1224 ec.transactionReceipt(ctx, common.HexToHash(input.TxHash)) - Can me make it consistent. And also we might want to add a test for Invalid Argument like TestCall_InvalidArgs one. I think then we are good to go.

Working on this now. When I started the PR I tried to use existing functions like: https://github.com/coinbase/rosetta-ethereum/blob/9a3846484a33d9354159cb2d59eb0b10f3ace793/ethereum/client.go#L212

This parses the response into a different structure with additional calls.

This drops some data fields - if you look at the Header struct from go-ethereum it's missing some fields found in the eth_getBlockByNumber response - see this test data file for example of the response The first isn't right since call is meant to be a pass-through function. The second would require me to re-factor it to allow the parameter value for show_transaction_details be passed in and use a generic map which may break other parts of the code. Do you have any thoughts on best way forward?

Maybe a best way is to have a separate function for getBlockByNumber which returns map[string]interface{} and don't touch the existing function. Eventually Block function might use this function.

Ok - will do that. Thanks!

For the invalid args test - should it be a test per call function i.e.:

TestCall_InvalidArgs_GetTransactionReceipt and TestCall_InvalidArgs_GetBlockByNumber?

Or should we just add another call and assert in TestCall_InvalidArgs?

shrimalmadhur commented 2 years ago

Updated - all passing now. Do you think it's worth adding any more tests?

I just realized that the other method in Call function calls a separate function rather than directly calling CallContext like on line 1224 ec.transactionReceipt(ctx, common.HexToHash(input.TxHash)) - Can me make it consistent. And also we might want to add a test for Invalid Argument like TestCall_InvalidArgs one. I think then we are good to go.

Working on this now. When I started the PR I tried to use existing functions like: https://github.com/coinbase/rosetta-ethereum/blob/9a3846484a33d9354159cb2d59eb0b10f3ace793/ethereum/client.go#L212

This parses the response into a different structure with additional calls.

This drops some data fields - if you look at the Header struct from go-ethereum it's missing some fields found in the eth_getBlockByNumber response - see this test data file for example of the response The first isn't right since call is meant to be a pass-through function. The second would require me to re-factor it to allow the parameter value for show_transaction_details be passed in and use a generic map which may break other parts of the code. Do you have any thoughts on best way forward?

Maybe a best way is to have a separate function for getBlockByNumber which returns map[string]interface{} and don't touch the existing function. Eventually Block function might use this function.

Ok - will do that. Thanks!

For the invalid args test - should it be a test per call function i.e.:

TestCall_InvalidArgs_GetTransactionReceipt and TestCall_InvalidArgs_GetBlockByNumber?

Or should we just add another call and assert in TestCall_InvalidArgs?

Let's make them separate. Easy to maintain and if we add more methods in call, it's easy to write.

akramhussein commented 2 years ago

@shrimalmadhur I've added the test and split out the function. Also not that experienced with go so do let me know if I did anything wrong or can improve it.