Record start version for new ConsensusV2 object streams #21614
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
aschran
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| .. | ||
| }) = input_objects.get(id).map(|obj| &obj.owner) | ||
| { | ||
| *start_version = *previous_start_version; |
There was a problem hiding this comment.
Is there any reason that start_version may not be equal to previous_start_version here?
There was a problem hiding this comment.
Not that I can think of, but I copied that logic from the shared object case above to be safe
Is there a reason that it can differ for the shared objects or could all of this be simplified?
There was a problem hiding this comment.
I believe the answer is "reshares" but maybe @tzakian would remember
EDIT: Yeah that looks to be the case, it might have been zero-d out in the reshare case, but we need to preserve the original initial_shared_version for that case
There was a problem hiding this comment.
Right that makes sense. In this case "reshare" may not even be dumb/extraneous, because you could be transferring it to a new owner/authenticator but still keeping the same ownership type
There was a problem hiding this comment.
Yeah so I think the only cases were we need to preserve this are when the "owner" doesn't change, so that any transactions previously scheduled for execution don't fail?
I am presuming here that if the start_version doesn't match the start_version of the argument that we will fail in a similar manner as if a shared object was deleted?
There was a problem hiding this comment.
@tnowacki Could you remind me what "reshare" means here?
In anyway it would be good to document the context as comment there. I am sure I will forget about this in a few months.
There was a problem hiding this comment.
Consider this move function
public fun foo(o: MySharedObj) {
sui::transfer::share_object(o)
}
A "reshare" is when foo is called with an input object that is a shared object (and it couldn't be any other kind of input object because we do not support upgrades currently)
This case became possible once we allowed shared objects to be taken by-value to enable shared object deletion.
There was a problem hiding this comment.
@lxfind added a comment per your suggestion
aschran
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
|
LGTM, but let's make sure the comments seem fine to @lxfind--and if he wants something similar on the share object case (even though it isn't new in this PR) |
crates/sui-types/src/execution.rs
Outdated
| }) = input_objects.get(id).map(|obj| &obj.owner) | ||
| { | ||
| // Assign existing start_version in case a ConsensusV2 object was | ||
| // re-transferred to the same Owner type. |
There was a problem hiding this comment.
As you mentioned since you could be transferring it to another address but still in the form of ConsensusV2 type, "re-transferred" seems inaccurate here. (which applies better to the old shared object case)
There was a problem hiding this comment.
can you say more about the inaccuracy? in case you transfer to another SingleOwner address with ConsensusV2 type, we still want this condition to trigger and preserve the original start_version, so as far as I can tell this is accurate as written?
There was a problem hiding this comment.
would it make more sense if I just wrote "transferred" instead of "re-transferred"?
There was a problem hiding this comment.
changing to "transferred" as agreed on chat
aschran
force-pushed
the
aschran/cov2-start-version
branch
from
79e8744 to
4e97ffe
Compare
aschran
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
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.