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

[account] Fix pagination on AccountModules and AccountResources #581

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gregnazario
Copy link
Collaborator

Description

Resolves #576

Test Plan

Added pagination tests for both calls

Related Links

Checklist

  • Have you ran pnpm fmt?
  • Have you updated the CHANGELOG.md?

@gregnazario gregnazario requested a review from a team as a code owner November 14, 2024 21:54
@@ -159,3 +159,45 @@ export async function paginateWithCursor<Req extends Record<string, any>, Res ex
} while (cursor !== null && cursor !== undefined);
return out as Res;
}

/// This function is a helper for paginating using a function wrapping an API using offset instead of start
export async function paginateWithObfuscatedCursor<Req extends Record<string, any>, Res extends Array<{}>>(
Copy link
Collaborator

@0xmaayan 0xmaayan Nov 20, 2024

Choose a reason for hiding this comment

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

iiuc, the only 2 differences is the start vs offset param and the re-evaluate length calculation - any reason not to simply use the existing paginateWithCursor but just provide a flag for the re-evaluate length calculation part? requestParams are already being passed as an argument to the paginateWithCursor function.

Copy link
Collaborator Author

@gregnazario gregnazario Nov 20, 2024

Choose a reason for hiding this comment

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

I wanted to be more explicit because the start vs offset is actually more of the problem that the one with the obfuscated cursor is not useful / a user shouldn't provide it directly. You would have to get it from a previous function call.

Also, I think the cursor header isn't always set, so that whole logic probably needs to be checked.

Basically there are 2 code paths on the serverside for pagination, and I'm trying to separate them in the event we have other errors with it.

I kinda wish the API just used the same system for both.

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.

aptos.getAccountModules()
2 participants