Skip to content
This repository was archived by the owner on Jul 1, 2021. It is now read-only.

Implement eth_getTransactionByHash, eth_getTransactionReceipt JSON RPC function #122

Closed
wants to merge 3 commits into from
Closed

Implement eth_getTransactionByHash, eth_getTransactionReceipt JSON RPC function #122

wants to merge 3 commits into from

Conversation

Bhargavasomu
Copy link
Contributor

What was wrong?

As part of Issue #26 and also #20.

How was it fixed?

This spec was followed

Cute Animal Picture

put a cute animal picture link inside the parentheses

@Bhargavasomu
Copy link
Contributor Author

Implementing the eth_getTransactionReceipt needs ethereum/py-evm#1705 to be merged in one way or the other.

cburgdorf
cburgdorf approved these changes Jan 9, 2019
View reviewed changes
@Bhargavasomu Bhargavasomu force-pushed the json_eth_rpc_leftover_part2 branch from 34f913c to 96ce0ac Compare January 9, 2019 10:11
@Bhargavasomu Bhargavasomu force-pushed the json_eth_rpc_leftover_part2 branch from 96ce0ac to 9588f8b Compare January 9, 2019 19:24
@pipermerriam
Copy link
Member

I think CI failures are due to recent merge of #121 and you need to rename self._chain to self.chain

@cburgdorf
Copy link
Contributor

@Bhargavasomu the chain inside the RPC module moved from self._chain to self.chain. It should have never used _ (underscore) notation as it's inherit and hence not private by definition.

@Bhargavasomu Bhargavasomu force-pushed the json_eth_rpc_leftover_part2 branch from 9588f8b to 3d40010 Compare January 10, 2019 07:51
cburgdorf
cburgdorf suggested changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

To make this work for light clients as well we need to implement coro_get_canoncal_transaction on the BaseAsyncChain and all the implementing chains and then use that instead.

trinity/rpc/modules/eth.py Outdated
@@ -247,6 +247,11 @@ def name(self) -> str:
nonce = account_db.get_nonce(address)
return hex(nonce)

@format_params(decode_hex)
async def getTransactionByHash(self, transaction_hash: Hash32) -> Dict[str, str]:
transaction = self.chain.get_canonical_transaction(transaction_hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I just noticed that this may not be correct. The modules work on a BaseAsyncChain which is primary because of the light client support where certain type of data (including this) needs to be fetched from peers. So if this API is intended to work for light clients as well (which I think it should) then, this needs to be fetched via a coro_get_canonical_transaction API which also needs to be implemented first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cburgdorf can this function coro_get_canonical_transaction be also added to the AsyncChainMixin as async_method(get_canonical_transaction), or does it need an explicit implementation as far as the API goes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would need an abstract method on BaseAsyncChainAPI and then an implementation of AsyncChainMixin (see my comment here https://github.com/ethereum/trinity/pull/122/files#r252588196)

@Bhargavasomu Bhargavasomu force-pushed the json_eth_rpc_leftover_part2 branch from 3d40010 to d507533 Compare January 28, 2019 15:47
@Bhargavasomu
Copy link
Contributor Author

@pipermerriam @cburgdorf review please. Also based on the above arguments, I guess even getTransactionReceipt should use the coro_ method. Could you please confirm this too? Thankyou.

@pipermerriam
Copy link
Member

@cburgdorf can I hand this over to you to finish review.

@Bhargavasomu I believe you are correct that the other methods should also be using coro_ methods for chain interactions.

@cburgdorf
Copy link
Contributor

Sorry, I missed this. I'll take a look at it tomorrow.

cburgdorf
cburgdorf suggested changes Jan 31, 2019
View reviewed changes
trinity/rpc/modules/eth.py Outdated
async def getTransactionReceipt(
self,
transaction_hash: Hash32) -> Dict[str, Union[str, List[Log]]]:
transaction_block_number, transaction_index = self.chain.chaindb.get_transaction_index(
Copy link
Contributor

Choose a reason for hiding this comment

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

Digging into self.chain.chaindb is violating the expected encapsulation. All modules should only work on the public API of the BaseAsyncChain.

I propose to add get_transaction_receipt to the BaseChain and then also add a coro_get_transaction_receipt to the BaseAsyncChainAPI and add an implementation in AsyncChainMixin.

And then this code here would use self.chain.get_transaction_receipt

@Bhargavasomu Bhargavasomu mentioned this pull request Feb 12, 2019
Merged
@Bhargavasomu Bhargavasomu force-pushed the json_eth_rpc_leftover_part2 branch from d507533 to ba4ae5d Compare February 16, 2019 06:41
@Bhargavasomu Bhargavasomu force-pushed the json_eth_rpc_leftover_part2 branch from ba4ae5d to d752899 Compare February 16, 2019 09:36
@Bhargavasomu Bhargavasomu changed the title [WIP] Implement eth_getTransactionByHash, eth_getTransactionReceipt JSON RPC function Implement eth_getTransactionByHash, eth_getTransactionReceipt JSON RPC function Feb 16, 2019
@Bhargavasomu
Copy link
Contributor Author

@cburgdorf I have now added the coro_ functions. Please review. Thankyou.

cburgdorf
cburgdorf reviewed Feb 18, 2019
View reviewed changes
trinity/chains/light.py
def get_transaction_receipt(self, transaction_hash: Hash32) -> Receipt:
raise NotImplementedError("Chain classes must implement " + inspect.stack()[0][3])

async def coro_get_transaction_receipt(self, transaction_hash: Hash32) -> Receipt:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this provide an actual implementation?

cburgdorf
cburgdorf reviewed Feb 18, 2019
View reviewed changes
Copy link
Contributor

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

I think this is still missing implementations for the LightDispatchChain. It would be really cool if we had actual tests backing this. But I know this might be a bit too much to ask for this PR.

trinity/chains/light.py
@@ -203,6 +203,15 @@ def create_unsigned_transaction(cls,
def get_canonical_transaction(self, transaction_hash: Hash32) -> BaseTransaction:
raise NotImplementedError("Chain classes must implement " + inspect.stack()[0][3])

async def coro_get_canonical_transaction(self, transaction_hash: Hash32) -> BaseTransaction:
raise NotImplementedError("Chain classes must implement " + inspect.stack()[0][3])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also provide an implementation?

@Bhargavasomu Bhargavasomu deleted the json_eth_rpc_leftover_part2 branch March 15, 2019 18:53
@lithp lithp mentioned this pull request Aug 29, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cburgdorf cburgdorf cburgdorf requested changes

@pipermerriam pipermerriam pipermerriam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bhargavasomu @pipermerriam @cburgdorf