Skip to content

Conversation

@notmatical
Copy link

@notmatical notmatical commented Apr 28, 2025

Description

Deduplicates the passed object of key server object ID's so they are only fetched one time, and then returns the preserved array including the fetched content.

Test plan

unit tests


@notmatical notmatical requested a review from a team as a code owner April 28, 2025 08:25
@vercel
Copy link

vercel bot commented Apr 28, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview May 16, 2025 5:05pm

@hayes-mysten hayes-mysten requested a review from joyqvq April 28, 2025 19:10
@notmatical
Copy link
Author

notmatical commented Apr 29, 2025

looks like integration test is failing, i am doing some investigation as to what's causing it now.

@github-actions
Copy link
Contributor

⚠️ 🦋 Changesets Warning: This PR has changes to public npm packages, but does not contain a changeset. You can create a changeset easily by running pnpm changeset in the root of the Sui repo, and following the prompts. If your change does not need a changeset (e.g. a documentation-only change), you can ignore this message. This warning will be removed when a changeset is added to this pull request.

Learn more about Changesets.

Copy link
Contributor

@hayes-mysten hayes-mysten left a comment

Choose a reason for hiding this comment

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

I think this overall seems worth while, but we probably want to add some test coverage for these changes

@notmatical notmatical requested a review from benr-ml May 11, 2025 21:25
@jonas-lj
Copy link
Contributor

Duplicates is not an issue after #293 has been merged, since retrieveKeyServers is now called without any duplicates among the object ID's. Duplicates are instead handled by adding weights to key servers. So I don't think this PR is needed anymore.

@notmatical
Copy link
Author

Correct, your PR is a solid approach. However, this should still be viable in terms of batching the rpc requests for object data. Previously we used getObject for each passed ID, now we take all the passed ID's and make use of getObjects

@jonas-lj
Copy link
Contributor

Ah, yes. Then this still makes sense, but you don't need to use a Set to avoid duplicates.

@notmatical
Copy link
Author

@jonas-lj latest should reflect the changes upstream from #293.

@notmatical notmatical changed the title [seal] prevent duplicated object fetching [seal] batched key server requests May 16, 2025
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.

4 participants