-
Notifications
You must be signed in to change notification settings - Fork 21
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
use query storage helper #1051
use query storage helper #1051
Conversation
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 don't seem to see the changes related to #1052
yeah #1052 is not related, it might be some docker issue, let's keep that issue open for now |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1051 +/- ##
==========================================
+ Coverage 74.51% 78.15% +3.63%
==========================================
Files 55 56 +1
Lines 3178 2737 -441
Branches 808 624 -184
==========================================
- Hits 2368 2139 -229
+ Misses 786 585 -201
+ Partials 24 13 -11
|
Change
Using api to query storage still has the memory issue polkadot-js/api#5981, so we switch back to the original query storage helper, but removed all cache for storage query. This is because there was a bug in the cache, and as we mentioned in #1039, caching storage won't be too useful.
We still keep the
blockHash => { blockNumber, Header, BlockData }
cache introduced in #1039 , which are commonly used. We keep the apiAt cache as well, which can speed up runtime calls.fix #1059
Although there is a small inconsistency when querying
system.account
compared toapiAt.query
, it does not affect our use case (we have been using this for a long time anyways). But in the future it would be nice to make it better, maybe @ntduan can take a look at #1060 when you got a chance?Test
all tests still pass (except the skipped one for storage query helper)