[GraphQL] Try to remove objectKeys #21032

stefan-mysten · 2025-01-31T06:12:19Z

Description

This PR removes the objectKeys field from ObjectFilter as per the previous deprecation notice. Use multiGetObjects instead.

Test plan

Updated tests accordingly and ensured all existing tests pass.

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

Protocol:
Nodes (Validators and Full nodes):
gRPC:
JSON-RPC:
GraphQL: Removed the objectKeys field from ObjectFilter as per the previous deprecation notice. Use multiGetObjects instead to fetch multiple objects.
CLI:
Rust SDK:

vercel · 2025-01-31T06:12:25Z

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

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 12, 2025 1:03am

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Feb 12, 2025 1:03am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Feb 12, 2025 1:03am

amnn

I think you're mostly there, but can just delete some more stuff...

amnn · 2025-01-31T11:41:18Z

crates/sui-graphql-rpc/src/types/gas.rs

@@ -71,7 +71,6 @@ impl GasInput {
        let page = Page::from_params(ctx.data_unchecked(), first, after, last, before)?;

        let filter = ObjectFilter {


This will need to be replaced with a call to multi-get (hopefully a test failed because of this change 🤞 ?)

Maybe in e2e tests, will have to double check.

This fix is not correct as indicated by the E2E tests above -- you are now fetching the latest version of each gas payment object rather than the version specified by the input. As mentioned previously, please use the multi-get API you just introduced.

amnn · 2025-01-31T11:44:34Z

crates/sui-graphql-rpc/src/types/object.rs

        // If both object IDs and object keys are specified, then we need to query in
        // both historical and consistent views, and then union the results.


Judging by this comment, this whole first branch should be removed, because it only applies if both object keys and object IDs are provided, which cannot happen any longer.

In fact this whole function becomes:

build_objects_query( if !filter.has_filters() { View::Historical } else { View::Consistent }, range, page, move |query| filter.apply(query), move |newer| newer, )

I was initially confused why we use this "Historical" view if there are no filters. I think I've figured out why, but generally it's pretty confusing, so let me write it out, and then @wlmyng and @emmazzz can correct me:

View::Historical makes it so that after we find candidate objects that meet our filter criteria, we don't look for newer versions of those objects (which would cause us to discard the candidate).

Previously, when we supported ID filters, key filters and pagination, we had the following semantics:

If you supply object keys and other filters, you are asking: "Find these versions of these objects, and tell me if they meet this criteria".

If you supply object IDs and other filters, you are asking: "Find the latest versions of these objects, and then tell me if they meet this criteria".

If you supply both IDs and Keys, you are really doing two distinct queries.

This should hopefully make it clear why we don't discard objects that meet our criteria if we supply keys (we were interested in those specific versions to begin with).

The reason we also do this when we don't supply any filter is because then we know that there can't be any newer objects (because we didn't apply any filter, the first query will just always find the latest objects). Could you add this explanation to the function's doc comment?

TL;DR -- I'm looking forward to separating all multi-get APIs from all query APIs.

I don't really follow as I don't know how the objects query is constructed.

@wlmyng could you verify this logic + let me know what I should update the doc comment to?

crates/sui-graphql-rpc/src/types/object.rs

amnn

Could you take another look at the changes to gas payment and filter intersection? They seem incorrect to me.

amnn · 2025-02-07T12:27:36Z

crates/sui-graphql-e2e-tests/tests/stable/consistency/objects_pagination.move

nit: these are no longer related to pagination -- it's probably worth putting them in their own test for multi_get?

amnn · 2025-02-07T12:29:07Z

crates/sui-graphql-e2e-tests/tests/stable/consistency/objects_pagination.move

@@ -118,20 +118,18 @@ module Test::M1 {

 //# run-graphql
 {
-  objects_at_version: address(address: "@{A}") {


Previously, this query also imposed an ownership constraint -- was that relevant for the test? It seems like from the results, version 3 didn't used to exist in the output and now it does.

I don't follow what is an ownership constraint.

The outer query on address(...) means that the inner query only returns objects owned by that address.

amnn · 2025-02-07T12:30:48Z

crates/sui-graphql-e2e-tests/tests/stable/consistency/tx_address_objects.snap

The fact that these results have changed is concerning -- before, the numbers were all different, and now they are not. Seems related to the changes to how gas payment lookups work.

I fixed the code to call Object::query_many which takes the Vec<ObjectKey> and checkpoint_viewed_at, but the result is the same. Something fishy.

amnn · 2025-02-07T12:34:47Z

crates/sui-graphql-rpc/src/types/gas.rs

@@ -71,7 +71,6 @@ impl GasInput {
        let page = Page::from_params(ctx.data_unchecked(), first, after, last, before)?;

        let filter = ObjectFilter {


This fix is not correct as indicated by the E2E tests above -- you are now fetching the latest version of each gas payment object rather than the version specified by the input. As mentioned previously, please use the multi-get API you just introduced.

amnn · 2025-02-07T13:05:51Z

crates/sui-graphql-rpc/src/types/object.rs

-            .map(|key| PointLookupKey {
-                id: key.object_id,
-                version: key.version.into(),
+        // Keep track of input keys' indices to ensure same output object order


This can be done more efficiently -- data is already a HashMap, so you should be able to construct your desired results by iterating over keys, creating point lookup keys, and then fetching them from that map.

let keys: Vec<_> = keys .into_iter() .map(|key| PointLookupKey { id: key.object_id, version: key.version.into(), }) .collect(); let data = loader.load_many(keys.clone()).await; Ok(keys.into_iter().map(|k| data.get(&k).cloned()).collect())

Writing this also made me realise that if ordering is relevant, then if we don't find an object key, then we should probably return null for that (so that other results line up). The snippet above does that, but it needs to be propagated to the actual implementation, schema, docs and tests.

Added this but it looks like the gas related numbers in E2E tests stayed the same, which is strange.

amnn · 2025-02-07T13:11:32Z

crates/sui-graphql-rpc/src/types/object.rs

-                .flatten()
-                .filter_map(|(id, v)| v.is_none().then_some(*id))
-                .collect();
+            if let (Some(obj_ids), Some(other_obj_ids)) = (&self.object_ids, &other.object_ids) {


We are missing a couple of cases here:

If only one of the filters have object ids, then I think we should be preserving those IDs in the intersection. I suspect using the intersect::field helper would achieve that.

If the resulting intersection is empty, then we need to return None from this function to indicate the fact that we know in advance that the result is inconsistent.

Think I fixed this.

crates/sui-graphql-rpc/src/types/object.rs

stefan-mysten requested review from amnn, wlmyng, emmazzz and suiwombat as code owners January 31, 2025 06:12

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env January 31, 2025 06:12 — with GitHub Actions Inactive

amnn reviewed Jan 31, 2025

View reviewed changes

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env January 31, 2025 22:17 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 31, 2025 22:20 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 18:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 18:48 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 20:22 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 20:23 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 22:08 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 22:09 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 4, 2025 18:51 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 4, 2025 18:55 View deployment

stefan-mysten requested a review from amnn February 6, 2025 03:50

stefan-mysten added 9 commits February 5, 2025 19:52

Try to remove objectKeys

b91770c

Fix e2 tests

084544e

Fixes

5f613e7

Fix staging tests

8f822c0

Fix tests

0f27cc2

Fix tests

eb7cba6

Fix tests

e0dd9b4

Fix tests by sorting the output of multi_get_query

c3b7430

Improve the sorting code

2ad5d2a

stefan-mysten force-pushed the gql_remove_objectKeys branch from e14e86c to 2ad5d2a Compare February 6, 2025 04:00

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 6, 2025 04:01 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 6, 2025 04:02 View deployment

amnn requested changes Feb 7, 2025

View reviewed changes

stefan-mysten added 4 commits February 10, 2025 21:25

Fix gas to use query many to retrieve coins at the specified versions

10be979

Simplify query many function

8b39bdd

Fix intersection of ObjectFilter

f7376b5

Fix tests

b34232d

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 11, 2025 05:51 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 11, 2025 05:53 View deployment

stefan-mysten added 2 commits February 11, 2025 16:07

Fix fmt and clippy

ae202d6

Move multi get objects in their own tests

d5b5f9f

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env February 12, 2025 01:01 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 12, 2025 01:03 View deployment

stefan-mysten requested a review from amnn February 12, 2025 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GraphQL] Try to remove objectKeys #21032

[GraphQL] Try to remove objectKeys #21032

stefan-mysten commented Jan 31, 2025 •

edited

Loading

vercel bot commented Jan 31, 2025 •

edited

Loading

amnn left a comment

amnn Jan 31, 2025

stefan-mysten Jan 31, 2025

stefan-mysten Feb 3, 2025

amnn Feb 7, 2025

amnn Jan 31, 2025

stefan-mysten Feb 4, 2025

amnn left a comment

amnn Feb 7, 2025

stefan-mysten Feb 13, 2025

amnn Feb 7, 2025

stefan-mysten Feb 12, 2025

amnn Feb 14, 2025

amnn Feb 7, 2025

stefan-mysten Feb 12, 2025

amnn Feb 7, 2025

amnn Feb 7, 2025

stefan-mysten Feb 12, 2025

amnn Feb 7, 2025

stefan-mysten Feb 12, 2025

		@@ -71,7 +71,6 @@ impl GasInput {
		let page = Page::from_params(ctx.data_unchecked(), first, after, last, before)?;

		let filter = ObjectFilter {

		// If both object IDs and object keys are specified, then we need to query in
		// both historical and consistent views, and then union the results.

[GraphQL] Try to remove objectKeys #21032

Are you sure you want to change the base?

[GraphQL] Try to remove objectKeys #21032

Conversation

stefan-mysten commented Jan 31, 2025 • edited Loading

Description

Test plan

Release notes

vercel bot commented Jan 31, 2025 • edited Loading

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-mysten commented Jan 31, 2025 •

edited

Loading

vercel bot commented Jan 31, 2025 •

edited

Loading