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

Add default returns for eth_coinbase, eth_hashrate, eth_gasPrice JSON RPC calls #112

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

Bhargavasomu
Copy link
Contributor

@Bhargavasomu Bhargavasomu commented Jan 5, 2019

What was wrong?

Link to Old PR: ethereum/py-evm#1651

As part of Issue #20 .
Implements eth_coinbase and eth_hashrate functions. The return values are the default values as mining isn't supported by trinity as of now.
eth_gasPrice currently returns the average of the gasprices of all the transactions in a block.

How was it fixed?

The functions were modified/added to trinity/rpc/eth.py

Cute Animal Picture

put a cute animal picture link inside the parentheses

@Bhargavasomu
Copy link
Contributor Author

@pipermerriam review please.

block_overall_gas_price = 0
for transaction in canonical_block.transactions:
block_overall_gas_price += transaction.gas_price
block_average_gas_price = block_overall_gas_price // len(canonical_block.transactions)
Copy link
Member

Choose a reason for hiding this comment

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

This needs special handling for the case where len(transactions) == 0.

@Bhargavasomu Bhargavasomu force-pushed the json_rpc_eth_leftover branch 2 times, most recently from 37c2ef7 to 16c3872 Compare January 7, 2019 18:57
@Bhargavasomu
Copy link
Contributor Author

@pipermerriam please take a look now. I have modified the logic so that if the canonical block has no transactions, we go to the parent block and check the number of transactions that parent block has. If that is also 0, then we keep on going to the parent, till we reach a block which has non-zero transactions.
(This approach should work because, as far as I know, the probability of a block having 0 transactions is quite low. Please correct me if wrong.)

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

@Bhargavasomu sorry to ask you to toss the work you did, but I'm inclined to just drop all the intelligence from the gasPrice for now since we know it's something that will ultimately be pluggable allowing different engines for computing gas prices.

num_transactions_latest_block = len(latest_block_having_transactions.transactions)
block_average_gas_price = block_overall_gas_price // num_transactions_latest_block

return hex(block_average_gas_price)
Copy link
Member

Choose a reason for hiding this comment

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

Given that even this approach has edge cases (like the genesis block when there are no transactions) I'm now inclined to drop this whole implementation in favor of a dead simple approach.

def gasPrice(self):
    return int(os.environ.get('TRINITY_GAS_PRICE', eth_utils.to_wei(1, 'gwei')))

This allows the user to set the price if they need via an environment variable and defaults to 1 gwei. In a subsequent PR we can make this pluggable.

@Bhargavasomu Bhargavasomu force-pushed the json_rpc_eth_leftover branch 2 times, most recently from 7e02e6e to 083c8db Compare January 8, 2019 11:44
@Bhargavasomu Bhargavasomu force-pushed the json_rpc_eth_leftover branch from 083c8db to c41df03 Compare January 8, 2019 12:05
@Bhargavasomu
Copy link
Contributor Author

@pipermerriam I understand the problem, and have made the code in a simple way. Please review. Thankyou.

@pipermerriam pipermerriam merged commit 3f80c22 into ethereum:master Jan 8, 2019
@Bhargavasomu Bhargavasomu deleted the json_rpc_eth_leftover branch January 10, 2019 07:56
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants