-
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
[JSON RPC API] - New APIs for getting object's dynamic field #5882
Conversation
33c3dea
to
65c7427
Compare
@@ -97,6 +131,21 @@ impl RpcReadApiServer for ReadApi { | |||
.try_into()?) | |||
} | |||
|
|||
async fn get_dynamic_field_object( |
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.
In https://docs.sui.io/devnet/build/programming-with-objects/ch5-dynamic-fields, we use dynamic field
and dynamic object field
to refer to two different types of dynamic fields and mentioned that an object stored in this kind of field will be considered wrapped and will not be accessible via its ID by external tools (explorers, wallets, etc) accessing storage.
. Does this endpoint support both types?
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 both are supported.
sui/crates/sui-types/src/dynamic_field.rs
Line 19 in 3735554
pub type_: DynamicFieldType, |
sui/crates/sui-types/src/dynamic_field.rs
Lines 27 to 33 in 3735554
pub enum DynamicFieldType { | |
#[serde(rename_all = "camelCase")] | |
DynamicField { | |
wrapped_object_id: ObjectID, | |
}, | |
DynamicObject, | |
} |
The DynamicFieldType
enum is used to indicate whether it is dynamic field or dynamic field object.
One question I have is do we still support object owning objects in Move? i.e. transferring ownership to an object without using dynamic field. This PR assumes an object can only own another object using DF, is this the right assumption? @tnowacki ? |
3735554
to
76fd4c0
Compare
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.
Noted a few things in what I believe is the output to documentation. Ignore me if I'm wrong.
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.
I'm thinking we should move owner indexer and the new dynamic field tables into indexes.rs.
It does not seem the owner info has to be in the critical path, or even live in validator's storage. After some quick look, the only place that validators may look at the owner table is make_account_info
which will be called by handle_account_info_request
. Today the only callsite is get_all_owned_objects
and sync_all_owned_objects
, which iiuc, is now deprecated with gateway gone. @lxfind wdyt?
The long term plan is to move all indices to indexer. cc @mystenmark
8470c07
to
4ce6a8e
Compare
@ronny-mysten thanks for all the doc fixes! |
I think we will still need I will make that changes. |
Correct. owner_index should not belong to validator store. |
d90a602
to
89b9fd9
Compare
Sorry for the late reply!
Correct. Within Move, you can only make an object own another object through dynamic fields (in particular dynamic object fields) |
Iiuc, |
89b9fd9
to
1d64f8e
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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.
Still going through things! But wanted to leave the first set of comments I had so far (I will continue to look more later today)
}; | ||
match self.get_owner_at_version(id, *old_version)? { | ||
Owner::AddressOwner(addr) => deleted_owners.push((addr, *id)), | ||
Owner::ObjectOwner(object_id) => { |
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.
You probably want to filter for the type here too. I don't think you want this event/index for the secondary object in the dynamic object field case.
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.
In other words, we need to check that the object's type is sui::dynamic_field::Field
, otherwise it could be some other object being used with sui::dynamic_object_field
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.
Still reviewing but leaving these comments here as I go: Do we need a type filter here if we are filtering on creation?
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.
good catch! this will remove unnecessary seek and delete ops on db
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.
Actually after looking at the code (this was done quite a while ago 😅), it will require an extra DB read to retrieve the object type because TransactionEffects only contains ObjectRef, which will be more expensive because it will deserialise the whole Object
from storage. We could do this without the db read after this PR which adds ObjectType to the effects.
I will add a TODO for now.
deleted_owners.push((addr, *id)); | ||
} | ||
Owner::ObjectOwner(object_id) => { | ||
deleted_dynamic_fields.push((ObjectID::from(object_id), *id)) |
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.
same type filtering issue as above
@@ -1209,6 +1211,10 @@ impl AuthorityState { | |||
effects: &SignedTransactionEffects, | |||
timestamp_ms: u64, | |||
) -> SuiResult { | |||
let changes = self |
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.
I'm just learning here, when does index_tx
get run? This is some separate service or something right?
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.
Currently this runs in fullnode (@gegaowp 's indexer work will probably move this to the indexer), there are 2 code path leading to this method, one is from the node_sync when the fullnode receive new transactions; another path is from the TransactiondOrchestrator which updates the local store after successful tx execution.
)), | ||
Owner::ObjectOwner(object_id) => { | ||
let id = o.id(); | ||
let Some(info) = self.try_create_dynamic_field_info(o)? else{ |
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.
Not important, but I've been noticing on my PRs too, it doesn't seem like rustfmt likes let-else
. There isn't a space in else{
Just noting and summoning my normal person for Rust questions, @bmwill. Do we need to do something else to configure rustfmt to handle let-else?
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.
Nope, unfortunately the let-else
feature was stabilized without the corresponding formatting being added to rustfmt
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.
I reviewed the RPC, Rust SDK and sui-storage parts, which LGTM.
I do not enough knowledge to review files below and will leave it to other folks
- authority.rs
- object_basics.move
- sui-types/src/dynamic_field.rs
crates/sui-json-rpc/src/api.rs
Outdated
parent_object_id: ObjectID, | ||
/// Optional paging cursor | ||
cursor: Option<ObjectID>, | ||
/// Maximum item returned per page |
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.
nit: looks like otherwise QUERY_MAX_RESULT_LIMIT will be returned, let's add a comment for that?
Err(anyhow!("Page result limit must be larger then 0."))?; | ||
} | ||
Ok(if limit > QUERY_MAX_RESULT_LIMIT { | ||
pub fn cap_page_limit(limit: Option<usize>) -> usize { |
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.
good move!
303e607
to
0c0601d
Compare
Anyone still reviewing this PR? I would like to merge soon. |
0c0601d
to
527a78a
Compare
added dynamic_field index
parse and process move object data for dynamic objects added test
Co-authored-by: ronny-mysten <[email protected]>
527a78a
to
6013e6f
Compare
Summary
This PR adds dynamic field support to Sui's JSON-RPC APIs.
Notable changes
dynamic_field_index
andowner_index
toIndexStore
storing all object owned dynamic field object infoowner_index
inAuthorityPerpetualTables
is deprecated and will be removed in subsequence PRget_dynamic_field_object
method for retrieving object using parent object id and dynamic field name.Examples
the first object is
nfts::marketplace::Marketplace
, second object isnfts::geniteam::Player
Requests
Response
Remaining Tasks:
TODOs:
get_objects_owned_by_object
from JSON-RPC APIs #6372get_object_owned_by_address
and add type query and pagination #6370