Update deprecated vstorage queries #55
Conversation
samsiegart
force-pushed
the
remove-batching
branch
from
18eef83
to
7aca2bf
Compare
samsiegart
force-pushed
the
remove-batching
branch
from
7aca2bf
to
371f7f8
Compare
packages/rpc/src/batchQuery.ts
Outdated
| .then(responseDatas => { | ||
| return Promise.all(responseDatas.map(res => res.json())); | ||
| }) |
There was a problem hiding this comment.
nit, optional
| .then(responseDatas => { | |
| return Promise.all(responseDatas.map(res => res.json())); | |
| }) | |
| .then(responseDatas => Promise.all(responseDatas.map(res => res.json()) |
| return Promise.all(responseDatas.map(res => res.json())); | ||
| }) | ||
| .then(responses => | ||
| responses.map((res, index) => { |
There was a problem hiding this comment.
this handler is quite complex at this depth.
consider factoring it out into a named function and adding a unit test for it.
There was a problem hiding this comment.
The logic could be pulled out into some other functions, but the code-paths are well tested, and I prefer having the unit tests be more encompassing of the entire logic with chainStorageWatcher.ts, I'll just leave as-is for now
| @@ -12,8 +12,8 @@ type Subscriber<T> = { | |||
|
|
|||
| const defaults = { | |||
| newPathQueryDelayMs: 20, | |||
| refreshLowerBoundMs: 2000, | |||
| refreshUpperBoundMs: 4000, | |||
| refreshLowerBoundMs: 4000, | |||
There was a problem hiding this comment.
is Product good with this doubling of minimal refresh time?
There was a problem hiding this comment.
Good question, I think since these are just defaults and configurable, erring on the side of less spam would be safer, but we can always pass in smaller values for certain queries in certain dapps
refs: #53
For the chain storage watcher, no longer use the deprecated RPC method to query vstorage. Instead, use the JSON API. There's a lot of ways to handle the query logic, but for now this just keeps the same internal logic and makes a shotgun of requests instead of a batch query. Bandwidth consumption seems roughly similar after increasing the polling period a bit, and the increased request count should have minimal impact with connection coalescing.
Also, updates the wallet connection component because it was relying on the watcher having an RPC node.
Updated unit tests and validated with Agoric/dapp-inter#209 locally using yarn link