Skip to content
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

Raise exceptions rather than return None in all jsonrpc calls #1218

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tests/core/eth-module/test_transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ def test_unmined_transaction_wait_for_receipt(web3):
'to': '0xd3CdA913deB6f67967B99D67aCDFa1712C293601',
'value': 123457
})
assert web3.eth.getTransactionReceipt(txn_hash) is None
with pytest.raises(ValueError):
web3.eth.getTransactionReceipt(txn_hash)

txn_receipt = web3.eth.waitForTransactionReceipt(txn_hash)
assert txn_receipt['transactionHash'] == txn_hash
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ def test_latest_block_based_cache_middleware_busts_cache(w3, mocker):
result = w3.manager.request_blocking('fake_endpoint', [])

assert w3.manager.request_blocking('fake_endpoint', []) == result
w3.testing.mine()
with pytest.raises(ValueError):
w3.testing.mine()

# should still be cached for at least 1 second. This also verifies that
# the middleware caches the latest block based on the block time.
Expand Down Expand Up @@ -211,8 +212,10 @@ def result_cb(method, params):
}))
w3.middleware_onion.add(latest_block_based_cache_middleware)

w3.manager.request_blocking('fake_endpoint', [])
w3.manager.request_blocking('fake_endpoint', [])
with pytest.raises(ValueError):
w3.manager.request_blocking('fake_endpoint', [])
with pytest.raises(ValueError):
w3.manager.request_blocking('fake_endpoint', [])

assert next(counter) == 2

Expand Down
6 changes: 4 additions & 2 deletions tests/core/middleware/test_simple_cache_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,10 @@ def result_cb(method, params):
rpc_whitelist={'fake_endpoint'},
))

w3.manager.request_blocking('fake_endpoint', [])
w3.manager.request_blocking('fake_endpoint', [])
with pytest.raises(ValueError):
w3.manager.request_blocking('fake_endpoint', [])
with pytest.raises(ValueError):
w3.manager.request_blocking('fake_endpoint', [])

assert next(counter) == 2

Expand Down
6 changes: 4 additions & 2 deletions tests/core/middleware/test_time_based_cache_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,10 @@ def mk_result(method, params):
w3.middleware_onion.add(construct_result_generator_middleware({'fake_endpoint': mk_result}))
w3.middleware_onion.add(time_cache_middleware)

w3.manager.request_blocking('fake_endpoint', [])
w3.manager.request_blocking('fake_endpoint', [])
with pytest.raises(ValueError):
w3.manager.request_blocking('fake_endpoint', [])
with pytest.raises(ValueError):
w3.manager.request_blocking('fake_endpoint', [])

assert next(counter) == 2

Expand Down
9 changes: 7 additions & 2 deletions tests/core/testing-module/test_testing_snapshot_and_revert.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import pytest


def test_snapshot_revert_to_latest_snapshot(web3):
web3.testing.mine(5)

Expand All @@ -11,7 +14,8 @@ def test_snapshot_revert_to_latest_snapshot(web3):

block_after_mining = web3.eth.getBlock("latest")

web3.testing.revert()
with pytest.raises(ValueError):
web3.testing.revert()

block_after_revert = web3.eth.getBlock("latest")

Expand All @@ -38,7 +42,8 @@ def test_snapshot_revert_to_specific(web3):

block_after_mining = web3.eth.getBlock("latest")

web3.testing.revert(snapshot_idx)
with pytest.raises(ValueError):
web3.testing.revert(snapshot_idx)

block_after_revert = web3.eth.getBlock("latest")

Expand Down
6 changes: 5 additions & 1 deletion tests/core/testing-module/test_testing_timeTravel.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import pytest


def test_time_traveling(web3):
current_block_time = web3.eth.getBlock("pending")['timestamp']

time_travel_to = current_block_time + 12345

web3.testing.timeTravel(time_travel_to)
with pytest.raises(ValueError):
web3.testing.timeTravel(time_travel_to)

latest_block_time = web3.eth.getBlock("pending")['timestamp']
assert latest_block_time >= time_travel_to
12 changes: 6 additions & 6 deletions web3/_utils/module_testing/eth_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -485,8 +485,8 @@ def test_eth_getBlockByHash(self, web3, empty_block):
assert block['hash'] == empty_block['hash']

def test_eth_getBlockByHash_not_found(self, web3, empty_block):
block = web3.eth.getBlock(UNKNOWN_HASH)
assert block is None
with pytest.raises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

I realize that I'm late to the party on this one, but I think that this should be a custom exception for each object type.

  • BlockNotFound
  • TransactionNotFound (use this for receipt fetching too)
  • anything else?

cc @njgheorghita

web3.eth.getBlock(UNKNOWN_HASH)

def test_eth_getBlockByNumber_with_integer(self, web3, empty_block):
block = web3.eth.getBlock(empty_block['number'])
Expand All @@ -498,8 +498,8 @@ def test_eth_getBlockByNumber_latest(self, web3, empty_block):
assert block['number'] == current_block_number

def test_eth_getBlockByNumber_not_found(self, web3, empty_block):
block = web3.eth.getBlock(12345)
assert block is None
with pytest.raises(ValueError):
web3.eth.getBlock(12345)

def test_eth_getBlockByNumber_pending(self, web3, empty_block):
current_block_number = web3.eth.blockNumber
Expand Down Expand Up @@ -565,8 +565,8 @@ def test_eth_getTransactionReceipt_unmined(self, web3, unlocked_account_dual_typ
'gas': 21000,
'gasPrice': web3.eth.gasPrice,
})
receipt = web3.eth.getTransactionReceipt(txn_hash)
assert receipt is None
with pytest.raises(ValueError):
web3.eth.getTransactionReceipt(txn_hash)

def test_eth_getTransactionReceipt_with_log_entry(self,
web3,
Expand Down
4 changes: 2 additions & 2 deletions web3/_utils/module_testing/parity_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
class ParityModuleTest:

def test_list_storage_keys_no_support(self, web3, emitter_contract_address):
keys = web3.parity.listStorageKeys(emitter_contract_address, 10, None)
assert keys is None
with pytest.raises(ValueError):
web3.parity.listStorageKeys(emitter_contract_address, 10, None)

def test_trace_replay_transaction(self, web3, parity_fixture_data):
trace = web3.parity.traceReplayTransaction(parity_fixture_data['mined_txn_hash'])
Expand Down
5 changes: 4 additions & 1 deletion web3/_utils/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ def fill_transaction_defaults(web3, transaction):
def wait_for_transaction_receipt(web3, txn_hash, timeout=120, poll_latency=0.1):
with Timeout(timeout) as _timeout:
while True:
txn_receipt = web3.eth.getTransactionReceipt(txn_hash)
try:
txn_receipt = web3.eth.getTransactionReceipt(txn_hash)
except ValueError:
txn_receipt = None
# FIXME: The check for a null `blockHash` is due to parity's
# non-standard implementation of the JSON-RPC API and should
# be removed once the formal spec for the JSON-RPC endpoints
Expand Down
6 changes: 6 additions & 0 deletions web3/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ def request_blocking(self, method, params):
if "error" in response:
raise ValueError(response["error"])

if response['result'] is None:
raise ValueError(f"The call to {method} did not return a value.")

return response['result']

async def coro_request(self, method, params):
Expand All @@ -107,6 +110,9 @@ async def coro_request(self, method, params):
if "error" in response:
raise ValueError(response["error"])

if response['result'] is None:
raise ValueError(f"The call to {method} did not return a value.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kclowes @dylanjw I'm not greatly familiar with the async gameplan for web3 - but it occurred to me that if we are raising exceptions for jsonrpc calls that return None in synchronous requests, then we should have the same behavior in the async requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me.


return response['result']

@deprecated_for("coro_request")
Expand Down