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

[akramhussein]

Motivation

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

Solution

• Add eth_getBlockByNumber to CallMethods in types.go
• Add a case block for eth_getBlockByNumber

Open questions

None

[akramhussein]

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

[shrimalmadhur]

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]

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]

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]

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]

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.

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

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]

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.

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

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]

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.

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

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]

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.

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

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]

@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.

[shrimalmadhur]

LGTM. Thanks!