-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
fix(graphql): add root object version for dynamic field queries #17934
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@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. |
Hey @unmaykr-aftermath , greatly appreciate you opening this PR! I've just returned from PTO and will get to reviewing this later today |
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.
This looks good, thank you for tackling this bug. I just have two comments:
- I believe we do not need
DynamicField.parent_version
, since it is set onsuper_.super_.root_version()
per the ways we can instantiate aDynamicField
- It would be great if we can introduce some comments on the functions that accept
root_version
, similar to what we have forcheckpoint_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 |
@wlmyng I think I addressed all your comments in the last two commits. Let me know |
I wonder if we should error if the user roots a dynamic field query on a DOF. The issue in that scenario is that
The user would likely expect the dynamic fields to be the ones at the time version |
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 Locally, I've set up an e2e test as follows:
The only change I made was to With a graphql query like And then, if you were to query with Child1 as the root: In comparison, with the existing |
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
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:
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
|
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? |
I suppose until we allow someone to provide a 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 |
@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 |
…ted-dof-queries Test fix from MystenLabs#17934
@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? |
@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. |
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 |
@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 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 But if you do have things set up locally, you can also rerun the e2e tests on your end with |
Hey @wlmyng I wasn't able to run tests properly since I ran into errors like this:
I decided then to merge
Can you check that all tests pass? |
@unmaykr-aftermath - looks like we are good to go. 🚢 it! |
Who can look into the failing checks and eventually merge it? 👀 |
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! |
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>
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>
## 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>
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>
## 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>
## 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>
…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>
## 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>
Description
Adds the following attribute to
sui_graphql_rpc::types::object::Object
: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
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 usecheckpoint_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.