-
-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat: query acct history #1277
feat: query acct history #1277
Conversation
b0aeca3
to
0126454
Compare
NOTE: Can't implement cache querying until #994 is resolved |
e4a6f01
to
a8a72a4
Compare
Note for reviewers: Some extra reasoning: this recursive algorithm is necessary in scenarios where there needs to be a backup in place for people to access these features, and push them towards using a better means to access history (explorer plugin, other query providers). From working on this PR, it became very apparently that there needs to be a refactor of both sessional history and the |
Just one of @ApeWorX/engineering needs to approve to merge |
fa2a7af
to
203ab41
Compare
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.
Testing issue: I connected to mainnet using Alchemy. I loaded my account with txns. And then I did this:
next(account.history.outgoing)
I got this error:
ChainError: 'stop=4' cannot be greater than account's current nonce (3).
(other feedback is small and nitpicky - mostly focused on testing)
203ab41
to
723ef84
Compare
believe I solved this with 723ef84 |
For some reason, EDIT: This is resolved |
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.
I ran some queries. Neat! What remains? This works a lot better than before 🙇🏻♀️ .
I do wonder how we can utilize a sessional cache to cache receipts from the slower HTTP-based caches (I saw your TODO). It could be opt-in.
Just this: #1277 (comment) I think this is just affecting the detection of nonce = 0, the rest works
I mean we have ape-cache. For permanently existing networks, the idea is that it pushes stuff to an SQLite DB, which is good for multi-session caching. Could implement a secondary cache provider (either extending the one under Also the query system needs a more general refactor of how data passes through it so that it only converts if/when necessary e.g. refactor all Lastly, a lot of the stuff relating to caching contracts is something I wanted to merge into the data system, as time goes on (fetching Manifests, maybe publishing them, fetch contract types via address, proxy info, etc.) |
7631c8b
to
a0bb800
Compare
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.
Ok I tested the heck out of this.
I think most of my feedback is related to the actual query system though.
Please let me know if there any other gaps you would like me to test.
Note, it is hard to ensure anything won't break in future releases without more tests.
self, | ||
*columns: List[str], | ||
start_nonce: int = 0, | ||
stop_nonce: Optional[int] = None, |
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.
I wish the resulting dataframe was indexed by nonce always.
When I do:
account.history.query("transaction", start_once=3, stop_nonce=6)
it shows the result like numeric indices 0 - 3, which threw me off at first.
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 I agree with this. Probably a breaking change though, issue to track for 0.7.0?
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.
i mean, you can't do anything with the data now.. it's just broken
Edit: nevermind, I thought this was something else.
ya this could be 0.7 or a later opportunity.
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.
Actually wait, we added this method here, so I think we could actually do it for account history (but not for block history or event history)
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.
nvm, it requires siginificant restructuring to how we handle the returns from the query manager in order to create an index column, so not handling this here
a0bb800
to
93f9b0a
Compare
NOTE: Special handling for `ReceiptAPI`
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.
Looks good, just a few semantic comments/questions
Co-authored-by: helloibis <[email protected]>
76926fd
to
3f60320
Compare
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.
Note: works great with latest |
What I did
BaseAddress.history
, which fetches the relevantAccountHistory
objectAccountHistory.outgoing
through the query managerProviderAPI
to search through chain history and fetch transactions by account and range of nonces, and implement inWeb3Provider
(brute force method, very expensive)AccountHistory
queries (by nonce) through eitherExplorerAPI
(if available) or via new brute-force methodNOTE: Change this functionality in
ape-etherscan
toQueryAPI
, and remove fromExplorerAPI
(non-breaking change)AccountHistory.query
for working with account history using a dataframe approach6. (WIP) Add support to the cache query engine for storing and fetchingAccountTransactionQuery
resultsfixes: #1237
fixes: APE-572
How I did it
I did this to take better advantage of the Ape query engine, and also provide more correct results in situations where an explorer wasn't available.
How to verify it
check the new feature
acct.history
on some local and public networks, with and without etherscanand the cache query engineChecklist
.query
- [ ] Add to cache provider