fix: fix out of gas problems with getClientDataSets #734
fix: fix out of gas problems with getClientDataSets #734hugomrdias wants to merge 2 commits intomasterfrom
getClientDataSets #734Conversation
…d use p-queue for `getPdpDataSets`
… update test cases
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 78c1674 | Commit Preview URL Branch Preview URL |
Apr 14 2026, 06:48 PM |
| for (const dataSet of data) { | ||
| yield dataSet | ||
| } |
There was a problem hiding this comment.
before yielding, you could start the next readContract.
There was a problem hiding this comment.
yeah, not a bad suggestion although it does complicate the code a little when you're promise juggling and potentially means more calls than necessary if you're iterating for a search but it would be quicker collection if the consumer is doing more than just appending to an array
up to you Hugo
There was a problem hiding this comment.
I think this would only save time if the p-queue is full so it's probably not worth it
| const queue = new PQueue({ | ||
| concurrency: 10, | ||
| interval: 1000, | ||
| intervalCap: 20, |
There was a problem hiding this comment.
20 seems low to me. In geth I could do 3k-12k calls per second. Are we enforcing someone's rate limit?
There was a problem hiding this comment.
yeah glif public nodes, which i just discover have a 100req/min token bucket type rate limit...
but to be honest, its mostly an experiment as this action is not used in the sdk and we want to introduce this style of handling in other places
| address: options.address, | ||
| offset: options.offset, | ||
| })) { | ||
| await queue.onSizeLessThan(20) |
There was a problem hiding this comment.
I don't think this line is necessary, and people might think it's enforcing the concurrency limit when it's actually limiting the size of the pending task queue
github-project-automation
bot
moved this from 📌 Triage
to ✔️ Approved by reviewer
in FOC
|
so you're pushing the complexity from the sdk into core? does this create inconsistencies in the patterns we've established so far where core is lower level? |
yeah but theres already other actions depending on this method in core that would need to implement the pagination and without the my rule here would be if its as low level as ran "out of gas" problems we should guard against in the core action. |
|
@rvagg whats do you think about the p-queue in |
3 participants
Should fix filecoin-project/filecoin-pin#362
Adds p-queue to
getPdpDataSetsits should only run into rate limits error now,getClientDataSetsWithDetailsin the sdk many need to use something like this.@rvagg the
hasMorepattern from pdp-verifier would have been better for these methods