Skip to content
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

fix(js): scan fetch record limit #137

Merged
merged 1 commit into from
May 10, 2023

Conversation

genaris
Copy link
Contributor

@genaris genaris commented May 5, 2023

There is an unnecessary chunk variable (inherited from previous Indy SDK implementation in AFJ) that is preventing fetchAll to retrieve all records when they are more than PAGE_SIZE. As a result, a given Scan cannot return more than 32 entries.

Simply checking for listHandle (scanNext return value) validity seems to be enough to determine if there are no more matching records.

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the 256 here to prevent you from retrieving too many records and messing stuff up? Or do you we want to make that a responsiblity of the caller?

@genaris
Copy link
Contributor Author

genaris commented May 9, 2023

Isn't the 256 here to prevent you from retrieving too many records and messing stuff up? Or do you we want to make that a responsiblity of the caller?

I think in this case is a bit different from Indy SDK because scanNext does not receive a chunk size: instead, it is retrieving at most 32 records per call (in Indy we used fetchWalletSearchNextRecords, which received a count parameter). So the loop will be executed until specified limit is reached or a scanNext call returns 0 records.

@TimoGlastra TimoGlastra merged commit 0d687c3 into hyperledger:main May 10, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants