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.

Sign up for GitHub

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] Add modified_at_versions to SuiTransactionEffects #7237

Closed
wants to merge 1 commit into from
Closed

[json] Add modified_at_versions to SuiTransactionEffects #7237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/sui-json-rpc-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1870,6 +1870,10 @@ pub struct SuiTransactionEffects {
// The status of the execution
pub status: SuiExecutionStatus,
pub gas_used: SuiGasCostSummary,
// The version that every modified (mutated or deleted) object had before it was modified by
// this transaction.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub modified_at_versions: Vec<(ObjectID, SequenceNumber)>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This style, the pair (ObjectID, SequenceNumber), does not seem in line with the rest of this struct, but I was unsure if I should make a type just for this case.

Any advice @patrickkuo?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use custom type (e.g. SuiObjectRef) to give the values names before serialisation to make it more readable.

IIRC this is preferred in typescript? CC @666lcz for comments

// The object references of the shared objects used in this transaction. Empty if no shared objects were used.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub shared_objects: Vec<SuiObjectRef>,
Expand Down Expand Up @@ -1916,6 +1920,7 @@ impl SuiTransactionEffects {
Ok(Self {
status: effect.status.into(),
gas_used: effect.gas_used.into(),
modified_at_versions: effect.modified_at_versions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate issue, but wondering why the existing modified_at_versions doesn't include the input object digest. This is important info if you want to replay the tx by reading the input values from an untrusted store.

Copy link
Member

Choose a reason for hiding this comment

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

Originally modified_at_versions was included to support reverting state updates, which assume a trusted store (the validator's own), so that's the reason for the omission. We chatted about this question in a recent meeting, and while this isn't necessary to support querying object history, it will be for replaying a transaction against an untrusted store -- if that's something that we support, we should add the digest.

shared_objects: to_sui_object_ref(effect.shared_objects),
transaction_digest: effect.transaction_digest,
created: to_owned_ref(effect.created),
Expand Down
1 change: 1 addition & 0 deletions crates/sui-open-rpc/src/examples.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ impl RpcExampleProvider {
storage_cost: 100,
storage_rebate: 10,
},
modified_at_versions: vec![],
shared_objects: vec![],
transaction_digest: TransactionDigest::new(self.rng.gen()),
created: vec![],
Expand Down