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(graphql): add root object version for dynamic field queries #17934

Merged

Conversation

unmaykr-aftermath
Copy link
Contributor

@unmaykr-aftermath unmaykr-aftermath commented May 25, 2024

Description

Adds the following attribute to sui_graphql_rpc::types::object::Object:

    /// Optional root parent object version if this is a dynamic field.
    ///
    /// This enables consistent dynamic field reads in the case of chained dynamic object fields,
    /// e.g., `Parent -> DOF1 -> DOF2`. In such cases, the object versions may end up like
    /// `Parent >= DOF1, DOF2` but `DOF1 < DOF2`. Thus, database queries for dynamic fields must
    /// bound the object versions by the version of the root object of the tree.
    ///
    /// Essentially, lamport timestamps of objects are updated for all top-level mutable objects
    /// provided as inputs to a transaction as well as any mutated dynamic child objects. However,
    /// any dynamic child objects that were loaded but not actually mutated don't end up having
    /// their versions updated.
    root_version: Option<u64>,

I tried making sure that this attribute is set correctly wherever an Object is created.

There's one scenario which still isn't covered: if the GQL query is rooted at a DOF, something like

query DynamicFields($parent: SuiAddress!, $version: Int, $after: String) {
  object(address: $parent, version: $version) {
    dynamicFields(/* ... */) {
        // ...
    }
  }
}

where $parent is a DOF's object id, then the server has no way to know the root object's version, therefore it will only use checkpoint_viewed_at to bound the objects query, which of course varies with the current checkpoint number since it can't be set via the query. This can give incorrect results.

Test plan

How did you test the new or updated feature?

I need help with this. At Aftermath, we tested it on Testnet against a protocol and verified that queries rooted at the root parent object successfully returned two levels of DOFs even though the first level didn't change version because its contents didn't change in a transaction. However the state of that protocol has since evolved and the root object version we tested is outside the available range of the RPC.


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):
  • Indexer:
  • JSON-RPC:
  • GraphQL: When dynamic fields are selected under the object field, if it is a dynamic object field, then its contents will be from the latest version that is at most equal to its parent object's version.
  • CLI:
  • Rust SDK:

Copy link

vercel bot commented May 25, 2024

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 Jun 21, 2024 6:27am

Copy link

vercel bot commented May 25, 2024

@unmaykr-aftermath is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

@unmaykr-aftermath
Copy link
Contributor Author

@wlmyng
Copy link
Contributor

wlmyng commented May 28, 2024

Hey @unmaykr-aftermath , greatly appreciate you opening this PR! I've just returned from PTO and will get to reviewing this later today

Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

This looks good, thank you for tackling this bug. I just have two comments:

  1. I believe we do not need DynamicField.parent_version, since it is set on super_.super_.root_version() per the ways we can instantiate a DynamicField
  2. It would be great if we can introduce some comments on the functions that accept root_version, similar to what we have for checkpoint_viewed_at

As for testing, I should be able to branch off this and set up an e2e test

crates/sui-graphql-rpc/src/types/dynamic_field.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/dynamic_field.rs Outdated Show resolved Hide resolved
@unmaykr-aftermath
Copy link
Contributor Author

This looks good, thank you for tackling this bug. I just have two comments:

  1. I believe we do not need DynamicField.parent_version, since it is set on super_.super_.root_version() per the ways we can instantiate a DynamicField
  2. It would be great if we can introduce some comments on the functions that accept root_version, similar to what we have for checkpoint_viewed_at

As for testing, I should be able to branch off this and set up an e2e test

Thanks. I'll be able to tackle these next week

@unmaykr-aftermath
Copy link
Contributor Author

@wlmyng I think I addressed all your comments in the last two commits. Let me know

@unmaykr-aftermath
Copy link
Contributor Author

I wonder if we should error if the user roots a dynamic field query on a DOF.

The issue in that scenario is that Object::root_version will return None. So imagine:

  • the current checkpoint number is 100
  • the user queries an object(...), which is a DOF of another object, at version 10, which was created at checkpoint 80
  • the user asks for one or more dynamic fields (under object(...) { ... })

The user would likely expect the dynamic fields to be the ones at the time version 10 of the DOF was created, i.e., at checkpoint 80. However, since Object::root_version will be set to None, dynamic field queries will search up to the current checkpoint, 100

@wlmyng
Copy link
Contributor

wlmyng commented Jun 4, 2024

I wonder if we should error if the user roots a dynamic field query on a DOF.

The issue in that scenario is that Object::root_version will return None. So imagine:

  • the current checkpoint number is 100
  • the user queries an object(...), which is a DOF of another object, at version 10, which was created at checkpoint 80
  • the user asks for one or more dynamic fields (under object(...) { ... })

The user would likely expect the dynamic fields to be the ones at the time version 10 of the DOF was created, i.e., at checkpoint 80. However, since Object::root_version will be set to None, dynamic field queries will search up to the current checkpoint, 100

If I'm understanding your example correctly, then we actually want to root on the top-level object regardless of whether the object is a DOF/ child object or not? If so, I think we can address this by relaxing Object::root_version's logic. If root_version is already stamped, we'll continue to use it. Otherwise, if it's not set, use the root object's version as the root_version, even if it is a child object.

Locally, I've set up an e2e test as follows:

  1. create Parent1 (2)
  2. create Child1 (3)
  3. create Child2 (4)
  4. add Child1 to Parent1 -> Parent1 (5), Child1 (5)
  5. add Child2 as a nested child object by borrowing Parent1.Child1 -> Parent1 (6), Child1 (5), Child2 (6)
  6. mutate child1 -> Parent1 (7), Child1 (7), Child2 (6)
  7. mutate child2 through parent -> Parent1 (8), Child1 (7), Child2 (8)

The only change I made was to infer_root_version, to return Some(native.version().into())

With a graphql query like object(Parent, version).dynamicFields.dynamicFields, the results will be as follows:
Parent(5) -> Child1 (5) -> None // add child1 as child to parent1
Parent(6) -> Child1 (5) -> Child2 (6) // add child2 as a child to child1 by borrowing child1 from parent
Parent(7) -> Child1 (7) -> Child2 (6) // mutate child1
Parent(8) -> Child1 (7) -> Child2 (8) // mutate nested child2 by borrowing from child1

And then, if you were to query with Child1 as the root:
Child1 (5) -> None
Child1 (7) -> Child2 (6)

In comparison, with the existing infer_root_version implementation, the second request will always return Child2 (8)

@unmaykr-aftermath
Copy link
Contributor Author

And then, if you were to query with Child1 as the root:
Child1 (5) -> None
Child1 (7) -> Child2 (6)

In comparison, with the existing infer_root_version implementation, the second request will always return Child2 (8)

Yeah, this seems to be as good as we can get without changing the implementation too much. There should be a caveat in some part of the documentation (not sure where) explaining that queries with child objects as root will view the state of that object version at the time of its creation.

Otherwise, a user could do the following queries

  1. Parent(6) -> Child1 (5) // add child2 as a child to child1 by borrowing child1 from parent
  2. Child1 (5) -> None

expecting that they can break the query "Parent(6) -> Child1 (5) -> Child2 (6)" in parts. We actually ran into this scenario ourselves because we wanted to:

  1. Get the contents of the Parent and a particular dynamic object (Child1)
  2. Paginate over the dynamic fields of the dynamic object

However, that wasn't correct as we later found out. Unfortunately it's a very subtle, silent error that we could have overlooked and gone to production with

Ultimately, a better solution IMO would be do expose the checkpoint_viewed_at in the GQL parameters; quoting @amnn from Telegram:

Eventually we want to expose this via an extra parameter to these queries, like so:

query ($objectId: SuiAddress!, $checkpoint: Int) {
  object(address: $objectId, latestAt: $checkpoint) {
    # ...
  }
}
query ($tableId: SuiAddress!, $checkpoint: Int) {
  owner(address: $tableId, latestAt: $checkpoint) {
    dynamicFields { 
      # ...
    }
  }
}

@wlmyng
Copy link
Contributor

wlmyng commented Jun 5, 2024

There should be a caveat in some part of the documentation (not sure where) explaining that queries with child objects as root will view the state of that object version at the time of its creation.

I think you're right in suggesting that we ought to return an error if someone is using the object field rooted on a DOF, due to the inconsistency between a caller expecting that they can break the query "Parent(6) -> Child1 (5) -> Child2 (6)" in parts and what they are actually likely to get back

The error can point the caller to use the owner field instead.

Meanwhile, on the Query.owner field, we can introduce an additional optional root_version parameter, so that someone can provide the address of some nested DOF that owns another DOF, and the proper root_version to fix to a point in time. I think that will also more accurately bound the history than setting a checkpoint

@unmaykr-aftermath
Copy link
Contributor Author

I think you're right in suggesting that we ought to return an error if someone is using the object field rooted on a DOF, due to the inconsistency between a caller expecting that they can break the query "Parent(6) -> Child1 (5) -> Child2 (6)" in parts and what they are actually likely to get back

The error can point the caller to use the owner field instead.

Meanwhile, on the Query.owner field, we can introduce an additional optional root_version parameter, so that someone can provide the address of some nested DOF that owns another DOF, and the proper root_version to fix to a point in time. I think that will also more accurately bound the history than setting a checkpoint

I like the ideas. The question is what is in scope for this PR. Should I add the error here and leave the Query.owner extension for later?

@wlmyng
Copy link
Contributor

wlmyng commented Jun 6, 2024

I like the ideas. The question is what is in scope for this PR. Should I add the error here and leave the Query.owner extension for later?

I suppose until we allow someone to provide a root_version, we should disallow someone from being able to root dynamic fields on a DOF on either the object or owner field; as in, it would only be possible to root on a DOF if a root_version is also provided, to disambiguate the possibility of returning historical dat at dof's mutation vs from the perspective of breaking up a nested query.

Would you be interested in tackling the Query.owner extension? I think it can be done in a follow-up PR

@unmaykr-aftermath
Copy link
Contributor Author

I suppose until we allow someone to provide a root_version, we should disallow someone from being able to root dynamic fields on a DOF on either the object or owner field; as in, it would only be possible to root on a DOF if a root_version is also provided, to disambiguate the possibility of returning historical dat at dof's mutation vs from the perspective of breaking up a nested query.

Would you be interested in tackling the Query.owner extension? I think it can be done in a follow-up PR

I'm interested, yes. It would solve some problems for us as well.

So I'll add some sort of error when someone requests dynamic fields from a child object without a set root_version

@wlmyng
Copy link
Contributor

wlmyng commented Jun 18, 2024

@unmaykr-aftermath I'm not that good with github foo, this is what I came up with to land the commits in this branch 😅 AftermathFinance#1

@unmaykr-aftermath
Copy link
Contributor Author

@wlmyng Merged your PR. There are conflicts here now that I don't know how to resolve. Can you solve them using the GitHub UI?

@amnn
Copy link
Contributor

amnn commented Jun 19, 2024

@wlmyng, you can push directly onto the branch for this PR to add commits (add the AftermathFinance version of the repo as a remote, and then push to the branch there). Github allows this because the branch is associated with a PR for a repo owned by an org that you are a member of.

@wlmyng
Copy link
Contributor

wlmyng commented Jun 20, 2024

Hey @unmaykr-aftermath , truly sorry for all the back and forth. I believe what @amnn proposed I should do is untenable due to this issue https://github.com/orgs/community/discussions/5634 (come on github!!!) so it looks like I'll have to create another PR pointed against this branch. Apologies for the churn! I can't use the web editor directly to resolve this conflict since it'll leave behind some unwanted artifacts. Instead I'll need to rebase the branch on main and resolve any conflicts there. It'll be up in a bit!

@unmaykr-aftermath
Copy link
Contributor Author

Hey @unmaykr-aftermath , truly sorry for all the back and forth. I believe what @amnn proposed I should do is untenable due to this issue https://github.com/orgs/community/discussions/5634 (come on github!!!) so it looks like I'll have to create another PR pointed against this branch. Apologies for the churn! I can't use the web editor directly to resolve this conflict since it'll leave behind some unwanted artifacts. Instead I'll need to rebase the branch on main and resolve any conflicts there. It'll be up in a bit!

All good. Just lmk

@wlmyng
Copy link
Contributor

wlmyng commented Jun 20, 2024

@unmaykr-aftermath while I have something in this PR AftermathFinance#2 I think it makes a mess of the commit history, and I'm not quite sure what this PR will look like when we merge that one ... Wondering if instead you'd be able to sync the fork to MystenLabs:main and rebase, or if that is not desirable, to rebase this branch onto latest main from MystenLabs?

You'll likely run into some merge conflicts - feel free to accept incoming changes or resolve them however you'd like. It'll also complain about query_latest_dynamic_field since we need to update the call to let gql_object = Object::try_from_stored_history_object(history_object, checkpoint_viewed_at, None)?;, but I can pick this up along with any changes to .exp files needed from rerunning sui-graphql-e2e-tests.

But if you do have things set up locally, you can also rerun the e2e tests on your end with env UB=1 cargo nextest run --package sui-graphql-e2e-tests --features pg_integration

@unmaykr-aftermath
Copy link
Contributor Author

But if you do have things set up locally, you can also rerun the e2e tests on your end with env UB=1 cargo nextest run --package sui-graphql-e2e-tests --features pg_integration

Hey @wlmyng I wasn't able to run tests properly since I ran into errors like this:

---- run_test::epoch/system_state.move ----
test panicked: Timeout waiting for graphql server to start: Elapsed(())


failures:
    run_test::epoch/system_state.move

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 62 filtered out; finished in 15.81s

I decided then to merge main into this branch and manually cherry-pick your changes. I checked that the e2e tests and graphql bin match using

git remote add -f wlmyng git@github.com:wlmyng/sui-public.git
git remote update
git diff wlmyng/resolve-conflicts-unmaykr-fix-dof crates/sui-graphql-e2e-tests/ crates/sui-graphql-rpc

Can you check that all tests pass?

@wlmyng
Copy link
Contributor

wlmyng commented Jun 21, 2024

@unmaykr-aftermath - looks like we are good to go. 🚢 it!

@unmaykr-aftermath
Copy link
Contributor Author

unmaykr-aftermath commented Jun 21, 2024

Who can look into the failing checks and eventually merge it? 👀

@wlmyng
Copy link
Contributor

wlmyng commented Jun 21, 2024

I think we can ignore the Vercel checks, they are not required. You should have the ability to squash and merge. But if not, I'm happy to press the button for you

@unmaykr-aftermath
Copy link
Contributor Author

I think we can ignore the Vercel checks, they are not required. You should have the ability to squash and merge. But if not, I'm happy to press the button for you

I don't have that ability unfortunately. You can do the honors!

@wlmyng wlmyng merged commit 25335da into MystenLabs:main Jun 21, 2024
40 of 43 checks passed
wlmyng added a commit that referenced this pull request Jun 24, 2024
Adds the following attribute to
`sui_graphql_rpc::types::object::Object`:
```rust
    /// Optional root parent object version if this is a dynamic field.
    ///
    /// This enables consistent dynamic field reads in the case of chained dynamic object fields,
    /// e.g., `Parent -> DOF1 -> DOF2`. In such cases, the object versions may end up like
    /// `Parent >= DOF1, DOF2` but `DOF1 < DOF2`. Thus, database queries for dynamic fields must
    /// bound the object versions by the version of the root object of the tree.
    ///
    /// Essentially, lamport timestamps of objects are updated for all top-level mutable objects
    /// provided as inputs to a transaction as well as any mutated dynamic child objects. However,
    /// any dynamic child objects that were loaded but not actually mutated don't end up having
    /// their versions updated.
    root_version: Option<u64>,
```

I tried making sure that this attribute is set correctly wherever an
`Object` is created.

There's one scenario which still isn't covered: if the GQL query is
rooted at a DOF, something like
```graphql
query DynamicFields($parent: SuiAddress!, $version: Int, $after: String) {
  object(address: $parent, version: $version) {
    dynamicFields(/* ... */) {
        // ...
    }
  }
}
```
where `$parent` is a DOF's object id, then the server has no way to know
the root object's version. Instead, we will use the rooting object's version, even if it is a nested child object.

New tests `immutable_dof.move` and `nested_dof.move`.

---

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):
- [ ] Indexer:
- [ ] JSON-RPC:
- [x] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:

---------

Co-authored-by: Ashok Menon <amenon94@gmail.com>
Co-authored-by: Will Yang <willyang@mystenlabs.com>
wlmyng added a commit that referenced this pull request Jun 24, 2024
Adds the following attribute to
`sui_graphql_rpc::types::object::Object`:
```rust
    /// Optional root parent object version if this is a dynamic field.
    ///
    /// This enables consistent dynamic field reads in the case of chained dynamic object fields,
    /// e.g., `Parent -> DOF1 -> DOF2`. In such cases, the object versions may end up like
    /// `Parent >= DOF1, DOF2` but `DOF1 < DOF2`. Thus, database queries for dynamic fields must
    /// bound the object versions by the version of the root object of the tree.
    ///
    /// Essentially, lamport timestamps of objects are updated for all top-level mutable objects
    /// provided as inputs to a transaction as well as any mutated dynamic child objects. However,
    /// any dynamic child objects that were loaded but not actually mutated don't end up having
    /// their versions updated.
    root_version: Option<u64>,
```

I tried making sure that this attribute is set correctly wherever an
`Object` is created.

There's one scenario which still isn't covered: if the GQL query is
rooted at a DOF, something like
```graphql
query DynamicFields($parent: SuiAddress!, $version: Int, $after: String) {
  object(address: $parent, version: $version) {
    dynamicFields(/* ... */) {
        // ...
    }
  }
}
```
where `$parent` is a DOF's object id, then the server has no way to know
the root object's version. Instead, we will use the rooting object's version, even if it is a nested child object.

New tests `immutable_dof.move` and `nested_dof.move`.

---

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):
- [ ] Indexer:
- [ ] JSON-RPC:
- [x] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:

---------

Co-authored-by: Ashok Menon <amenon94@gmail.com>
Co-authored-by: Will Yang <willyang@mystenlabs.com>
wlmyng added a commit that referenced this pull request Jun 24, 2024
## Description 

Cherry-pick #17934 which fixes dof implementation of nested dof

## Test plan 

How did you test the new or updated feature?

---

## 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): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:

---------

Co-authored-by: UnMaykr <98741738+unmaykr-aftermath@users.noreply.github.com>
Co-authored-by: Ashok Menon <amenon94@gmail.com>
wlmyng added a commit that referenced this pull request Jun 25, 2024
Adds the following attribute to
`sui_graphql_rpc::types::object::Object`:
```rust
    /// Optional root parent object version if this is a dynamic field.
    ///
    /// This enables consistent dynamic field reads in the case of chained dynamic object fields,
    /// e.g., `Parent -> DOF1 -> DOF2`. In such cases, the object versions may end up like
    /// `Parent >= DOF1, DOF2` but `DOF1 < DOF2`. Thus, database queries for dynamic fields must
    /// bound the object versions by the version of the root object of the tree.
    ///
    /// Essentially, lamport timestamps of objects are updated for all top-level mutable objects
    /// provided as inputs to a transaction as well as any mutated dynamic child objects. However,
    /// any dynamic child objects that were loaded but not actually mutated don't end up having
    /// their versions updated.
    root_version: Option<u64>,
```

I tried making sure that this attribute is set correctly wherever an
`Object` is created.

There's one scenario which still isn't covered: if the GQL query is
rooted at a DOF, something like
```graphql
query DynamicFields($parent: SuiAddress!, $version: Int, $after: String) {
  object(address: $parent, version: $version) {
    dynamicFields(/* ... */) {
        // ...
    }
  }
}
```
where `$parent` is a DOF's object id, then the server has no way to know
the root object's version. Instead, we will use the rooting object's version, even if it is a nested child object.

New tests `immutable_dof.move` and `nested_dof.move`.

---

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):
- [ ] Indexer:
- [ ] JSON-RPC:
- [x] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:

---------

Co-authored-by: Ashok Menon <amenon94@gmail.com>
Co-authored-by: Will Yang <willyang@mystenlabs.com>
wlmyng added a commit that referenced this pull request Jun 25, 2024
## Description 

Cherry-pick #17934 to the legacy branch

## Test plan 

How did you test the new or updated feature?

---

## 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): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:

---------

Co-authored-by: UnMaykr <98741738+unmaykr-aftermath@users.noreply.github.com>
Co-authored-by: Ashok Menon <amenon94@gmail.com>
@unmaykr-aftermath unmaykr-aftermath deleted the unmaykr/fix-graphql-nested-dof-queries branch July 2, 2024 11:53
amnn added a commit that referenced this pull request Jul 15, 2024
## Description 

This adds an optional `root_version` argument to `Query.owner` as
discussed in PR #17934.

In summary:
```
`root_version` represents the version of the root object in some nested chain of dynamic    
fields. It allows historical queries for the case of wrapped objects, which don't have a    
version. For example, if querying the dynamic fields of a table wrapped in a parent object, 
passing the parent object's version here will ensure we get the dynamic fields' state at the
moment that parent's version was created.                                                   
                                                                                            
If `root_version` is left null, the dynamic fields will be from a consistent snapshot of the
Sui state at the latest checkpoint known to the GraphQL RPC.                                
```

## Test plan 

Introduced new E2E tests:

```
sui-graphql-e2e-tests$ cargo nextest run --features pg_integration
```

---

## 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): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [x] GraphQL: Introduces an optional `rootVersion` parameter to
`Query.owner`. This can be used to do versioned lookups when reading
dynamic fields rooted on a wrapped object or another dynamic object
field.
- [ ] CLI: 
- [ ] Rust SDK:

---------

Co-authored-by: Ashok Menon <ashok@mystenlabs.com>
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this pull request Jul 29, 2024
…enLabs#17934)

## Description 

Adds the following attribute to
`sui_graphql_rpc::types::object::Object`:
```rust
    /// Optional root parent object version if this is a dynamic field.
    ///
    /// This enables consistent dynamic field reads in the case of chained dynamic object fields,
    /// e.g., `Parent -> DOF1 -> DOF2`. In such cases, the object versions may end up like
    /// `Parent >= DOF1, DOF2` but `DOF1 < DOF2`. Thus, database queries for dynamic fields must
    /// bound the object versions by the version of the root object of the tree.
    ///
    /// Essentially, lamport timestamps of objects are updated for all top-level mutable objects
    /// provided as inputs to a transaction as well as any mutated dynamic child objects. However,
    /// any dynamic child objects that were loaded but not actually mutated don't end up having
    /// their versions updated.
    root_version: Option<u64>,
```

I tried making sure that this attribute is set correctly wherever an
`Object` is created.

There's one scenario which still isn't covered: if the GQL query is
rooted at a DOF, something like
```graphql
query DynamicFields($parent: SuiAddress!, $version: Int, $after: String) {
  object(address: $parent, version: $version) {
    dynamicFields(/* ... */) {
        // ...
    }
  }
}
```
where `$parent` is a DOF's object id, then the server has no way to know
the root object's version. Instead, we will use the rooting object's version, even if it is a nested child object.

## Test plan 

New tests `immutable_dof.move` and `nested_dof.move`.

---

## 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): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [x] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:

---------

Co-authored-by: Ashok Menon <amenon94@gmail.com>
Co-authored-by: Will Yang <willyang@mystenlabs.com>
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this pull request Jul 29, 2024
## Description 

This adds an optional `root_version` argument to `Query.owner` as
discussed in PR MystenLabs#17934.

In summary:
```
`root_version` represents the version of the root object in some nested chain of dynamic    
fields. It allows historical queries for the case of wrapped objects, which don't have a    
version. For example, if querying the dynamic fields of a table wrapped in a parent object, 
passing the parent object's version here will ensure we get the dynamic fields' state at the
moment that parent's version was created.                                                   
                                                                                            
If `root_version` is left null, the dynamic fields will be from a consistent snapshot of the
Sui state at the latest checkpoint known to the GraphQL RPC.                                
```

## Test plan 

Introduced new E2E tests:

```
sui-graphql-e2e-tests$ cargo nextest run --features pg_integration
```

---

## 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): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [x] GraphQL: Introduces an optional `rootVersion` parameter to
`Query.owner`. This can be used to do versioned lookups when reading
dynamic fields rooted on a wrapped object or another dynamic object
field.
- [ ] CLI: 
- [ ] Rust SDK:

---------

Co-authored-by: Ashok Menon <ashok@mystenlabs.com>
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.

3 participants