[SOD] Reshare objects at same initial_shared_version #17834
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
tzakian
changed the title
crates/sui-types/src/object.rs
Outdated
| } => { | ||
| write!( | ||
| f, | ||
| "Shared(initial_shared_version: {})", |
There was a problem hiding this comment.
this is used in sui-tool, where I think I'd prefer the more concise option of Shared(N), if its all the same to you
There was a problem hiding this comment.
All the same to me -- will update.
There was a problem hiding this comment.
Are we okay with the non-hex value here? We definitely want it for tests, I'm just not sure why hex was used in the display normally
crates/sui-types/src/execution.rs
Outdated
| initial_shared_version: previous_initial_shared_version, | ||
| }) = input_objects.get(id).map(|obj| &obj.owner) | ||
| { | ||
| assert!(!self.created_object_ids.contains(id)); |
There was a problem hiding this comment.
is it worth removing the assert and making this an if/else?
There was a problem hiding this comment.
I don't believe so? IMO this is just a sanity assert to make sure if we get into a situation where the object is showing as both created and an input object we should panic.
There was a problem hiding this comment.
true, but if we think that's worth an assertion shouldn't it apply to all owner types? that said, i'm not very worried about this either way
tzakian
force-pushed
the
tzakian/sod-fix-iso
branch
from
42befaa
to
d26f1aa
Compare
tzakian
force-pushed
the
tzakian/sod-fix-iso
branch
from
d26f1aa
to
8afdd7c
Compare
| debug_assert!(!self.deleted_object_ids.contains(id)); | ||
| debug_assert!( | ||
| *initial_shared_version == SequenceNumber::new() | ||
| || *initial_shared_version == *previous_initial_shared_version |
There was a problem hiding this comment.
why isn't it always == previous_initial_shared_version? i would think the only way it can be SequenceNumber::new() is if that is also what the previous version is?
There was a problem hiding this comment.
That's the bug here, that sometimes it is SequenceNumber::new()
There was a problem hiding this comment.
This is the issue today -- in the written_objects the initial_shared_version will be SequenceNumber::new() in this case, and if it's already equal to the initial_shared_version on input there's no issue.
crates/sui-types/src/object.rs
Outdated
| Self::Shared { | ||
| initial_shared_version, | ||
| } => { | ||
| write!(f, "Shared({})", initial_shared_version.value()) |
There was a problem hiding this comment.
nit. I know the formatting above isn't super consistent (like that : sitting after Object ID), but it might be better to do
| write!(f, "Shared({})", initial_shared_version.value()) | |
| write!(f, "Shared ( {} )", initial_shared_version.value()) |
Or maybe just change the other ones?
There was a problem hiding this comment.
I'd be more in favor of just updating this one to have the space -- will do that now :)
| debug_assert!(!self.deleted_object_ids.contains(id)); | ||
| debug_assert!( | ||
| *initial_shared_version == SequenceNumber::new() | ||
| || *initial_shared_version == *previous_initial_shared_version |
There was a problem hiding this comment.
That's the bug here, that sometimes it is SequenceNumber::new()
Added! See |
tzakian
force-pushed
the
tzakian/sod-fix-iso
branch
from
8afdd7c
to
6746227
Compare
## Description Update the semantics of resharing so it preserves the initial shared version of the shared object. ## Test plan Updated display for tests, and made sure that the initial shared version is updated correctly under the new protocol version. Bottom commit are the actual changes, and the updates to the meaningful tests. The top (fixup) commit are the changes to the rest of the tests to account for the update to the object owner display. --- ## 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:
…7834) (#17840) ## Description Update the semantics of resharing so it preserves the initial shared version of the shared object. ## Test plan Updated display for tests, and made sure that the initial shared version is updated correctly under the new protocol version. Bottom commit are the actual changes, and the updates to the meaningful tests. The top (fixup) commit are the changes to the rest of the tests to account for the update to the object owner display. --- ## 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: ## Description Describe the changes or additions included in this PR. ## 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:
Description
Update the semantics of resharing so it preserves the initial shared version of the shared object.
Test plan
Updated display for tests, and made sure that the initial shared version is updated correctly under the new protocol version.
Bottom commit are the actual changes, and the updates to the meaningful tests. The top (fixup) commit are the changes to the rest of the tests to account for the update to the object owner display.
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.