-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
feat: add optional root_version
to Query.owner
#18486
feat: add optional root_version
to Query.owner
#18486
Conversation
@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. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Thanks for this @unmaykr-aftermath !
Note though that the initial idea was to add rootVersion
as a parameter to Query.owner
-- i.e. you could make a query for an Owner
and it would have an optional concept of a root_version
(if it's an object, this represents the version of that object, if it's a child object or a wrapped object it's the version of the root object that owns that object).
We would then propagate that through to all these dynamic field queries, but we can also use it as the object's version if a call to asMoveObject
is made, etc.
Okay, I will update the PR then |
…ies" This reverts commit 5237a4b.
root_version
to Owner
DF queriesroot_version
to Query.owner
Hey @amnn, I implemented the suggested changes and used But for the For instance, recall the example breaking up the query "Parent(6) -> Child1 (5) -> Child2 (6)" in parts:
If in 2 above the user also uses What do you think? That probably means implementing a new SQL query, but I have some experience with |
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.
Thanks for this change, this looks great!
I see what you're saying about how to interpret Owner.root_version
in asObject
-- I think you're right. Luckily you don't need to write any more SQL/diesel -- this query already exists to support dynamic field lookups, and is exposed via a DataLoader:
sui/crates/sui-graphql-rpc/src/types/object.rs
Lines 830 to 834 in afc3dc1
pub(crate) async fn query( | |
ctx: &Context<'_>, | |
id: SuiAddress, | |
key: ObjectLookup, | |
) -> Result<Option<Self>, Error> { |
You can construct an ObjectLookup::LatestAt
with Owner.root_version
passed in as the parent_version
.
When we do this, we should also generalise the docs (thanks for improving these by the way!) so that it's clear that root version also influences the version of the object that gets returned as well as the versions of dynamic fields that are fetched.
@@ -552,6 +552,7 @@ impl ObjectImpl<'_> { | |||
owner: Some(Owner { | |||
address, | |||
checkpoint_viewed_at: self.0.checkpoint_viewed_at, | |||
root_version: None, |
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.
Should this be the Object's root_version
, actually?
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.
That's a tricky one, because if this object's owner is an address and not an object, then I don't see the use in propagating the root version (which is gonna be the object's version itself) upwards.
That SuiAddress is not going to have dynamic fields and is not an object ofc, so root_version
doesn't seem to serve any purpose
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.
Yes, you're right -- originally, I was thinking about the case where the object was transferred to another object (so its owner is an AddressOwner
, but that address points to an object, not an account address), but in that case, the rules around parent versions and bounding do not apply.
Looking at the rest of this function, the case that this comment is relevant for is actually the Parent
owner case (this is the case where we're travelling back up the dynamic field hierarchy) -- we should be propagating the root version there, but it's kind of orthogonal to this PR, we can do that in a follow-up.
Thanks @amnn, I had completely forgotten about |
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 great, thanks again for contributing @unmaykr-aftermath! Let me see if either I or @wlmyng can find some time to write tests for this new feature, either today or tomorrow, and we can get it in.
## Description Updating E2E and snapshot tests for MystenLabs#18486. ## Test plan ``` sui-graphql-rpc$ cargo nextest run sui-graphql-e2e-tests$ cargo nextest run --features pg_integration ```
Sorry for the delay @unmaykr-aftermath -- the PR with tests is up here: AftermathFinance#3 |
[GraphQL] Tests for Owner.rootVersion
Thanks for the collaboration! |
## 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
This adds an optional
root_version
argument toQuery.owner
as discussed in PR #17934.In summary:
Test plan
Introduced new E2E tests:
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.
rootVersion
parameter toQuery.owner
. This can be used to do versioned lookups when reading dynamic fields rooted on a wrapped object or another dynamic object field.