-
Notifications
You must be signed in to change notification settings - Fork 293
Fix fetch_positions_for_owner() to perform batch calls #805
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
Conversation
6ba9f21 to
0454f07
Compare
wjthieme
left a comment
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.
Can you also add this for the ts sdk?
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@orca-so/whirlpools-rust": patch | |||
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.
Also need a changeset for the TS sdk
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.
Fixed.
rust-sdk/whirlpool/src/utils.rs
Outdated
| ) -> Result<Vec<Option<Account>>, Box<dyn std::error::Error>> { | ||
| let mut results = vec![]; | ||
|
|
||
| for chunk in pubkeys.chunks(chunk_size.unwrap_or(DEFAULT_CHUNK_SIZE)) { |
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.
nit: Clamp the chunk_size to (0, MAX_CHUNK_SIZE)?
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.
An iterator over a slice in (non-overlapping) chunks (chunk_size elements at a time), starting at the beginning of the slice.
When the slice len is not evenly divided by the chunk size, the last slice of the iteration will be the remainder.
Clamp is necessary here?
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.
Anything less than 1 and more than 100 is invalid
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.
Fixed.
ts-sdk/whirlpool/src/position.ts
Outdated
| const chunkSize = 100; | ||
|
|
||
| let promises = []; | ||
| for (let i = 0; i < positionAddresses.length; i += chunkSize) { |
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.
Would be nice if you can make this a separate function as well (like in rust). Something like:
export async function fetchMultipleAccountsBatched<T extends object>(
rpc: Rpc<GetMultipleAccountsApi>,
addresses: Address[],
decoder: (account: MaybeEncodedAccount) => MaybeAccount<T>,
): Promise<MaybeAccount<T>[]> {
const numChunks = Math.ceil(addresses.length / MAX_CHUNK_SIZE);
const chunks = [...Array(numChunks).keys()].map((i) =>
addresses.slice(i * MAX_CHUNK_SIZE, (i + 1) * MAX_CHUNK_SIZE),
);
const results: MaybeAccount<T>[] = [];
for (const chunk of chunks) {
const chunkResult = await fetchEncodedAccounts(rpc, chunk);
chunkResult.forEach((account, i) => {
const data = decoder(account);
results.push(data);
});
}
return results;
}
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.
Fixed.
This reverts commit 781b829.
No description provided.