-
-
Notifications
You must be signed in to change notification settings - Fork 172
feat(query): add partial query solver engine #2536
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
base: main
Are you sure you want to change the base?
Changes from all commits
a5043eb
4c2d4d6
6b6fd97
e6106ee
0606f79
680e142
1991671
cdbbfa0
2785105
a7a75a5
598dac6
0c62c09
5ec2bae
7382881
aa649b0
0a78906
f262f25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,6 @@ | |
| from eth_utils import to_hex | ||
| from pydantic import Field, computed_field, field_serializer, model_validator | ||
|
|
||
| from ape.api.networks import NetworkAPI | ||
| from ape.api.query import BlockTransactionQuery | ||
| from ape.api.transactions import ReceiptAPI, TransactionAPI | ||
| from ape.exceptions import ( | ||
| APINotImplementedError, | ||
| ProviderError, | ||
|
|
@@ -44,6 +41,9 @@ | |
| from ape.utils.process import JoinableQueue, spawn | ||
| from ape.utils.rpc import RPCHeaders | ||
|
|
||
| from .networks import NetworkAPI | ||
| from .transactions import ReceiptAPI, TransactionAPI | ||
|
|
||
| if TYPE_CHECKING: | ||
| from eth_pydantic_types import HexBytes | ||
| from ethpm_types.abi import EventABI | ||
|
|
@@ -151,12 +151,21 @@ def transactions(self) -> list[TransactionAPI]: | |
| """ | ||
| All transactions in a block. | ||
| """ | ||
| from ape.api.query import BlockTransactionQuery | ||
|
|
||
| if self.hash is None: | ||
| # Unable to query transactions. | ||
| # NOTE: Only "unsealed" blocks do not have a hash | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not every provider Ape talks to is a blockchain. Made up dev blocks from providers like boa don't necessarily need hashes even though they have associated transactions.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an aside, boa could also not use a real hash for block hash (e.g. block 1 is hash 0x1, etc)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we already made this change, but it was during development that led to the discovery of the bug leading to the test. I think I am good with adjusting the test to catch an exception (instead of just completely deleting the test), as it was found with purpose. |
||
| raise ProviderError("Unable to find block transactions: not sealed yet") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be bad to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if there's a use case to work with that, but potentially thatmigjt be better |
||
|
|
||
| elif self.num_transactions == 0: | ||
| return [] | ||
|
|
||
| try: | ||
| query = BlockTransactionQuery(columns=["*"], block_id=self.hash) | ||
| query = BlockTransactionQuery( | ||
| columns=["*"], | ||
| num_transactions=self.num_transactions, | ||
| block_id=self.hash, | ||
| ) | ||
| return cast(list[TransactionAPI], list(self.query_manager.query(query))) | ||
| except QueryEngineError as err: | ||
| # NOTE: Re-raising a better error here because was confusing | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if happens is the query manager gets used w/o Pandas installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, narwhals is an abstraction package that has code to translate operations on your choice of dataframe implementation, so if you don't have pandas installed but pick pandas then it will raise (hence the config option)
Many people prefer polars over pandas, and this would allow us to support both without having to ship with either package (meaning you would install your preferred package and set the config to use it as default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on option, we could have a dynamic default that prefers polars if it is installed but uses pandas otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can do that, because it's unlikely that someone would have 2 or more installed, and if they do they can just set it via config (or kwarg)