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.

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

[cleanup] Rename type ID in Move Framework to VersionedID #493

Merged
merged 2 commits into from
Feb 21, 2022

Conversation

666lcz
Copy link
Contributor

@666lcz 666lcz commented Feb 20, 2022

Per discussion in #291, VersionedID is an more appropriate name because the struct contains both the id and version.

@666lcz 666lcz changed the title Rename type ID in Move Framework to Versioned ID [cleanup] Rename type ID in Move Framework to VersionedID Feb 20, 2022
@666lcz 666lcz requested review from lxfind and sblackshear February 20, 2022 05:27
Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for taking this on! This unblocks some other improvements I'd like to see in ID:

  • Changing get_id to return an &ID instead of a VersionedID--getting a stable ID is likely what folks want in the common case, not an unstable VersionedID
  • Moving IDBytes to a different module--I think this will allow us to fix a lot of the horribly confusing naming (that I am responsible for, apologies) of functions that return ID vs IDBytes
  • We could consider rearranging the API's to prevent Move programmers from reading the version altogether... it's not used by any application code right now, and writing code that reads the version is bound to be hard for tools like the Move prover to reason about because the logic for updating it lives in the adapter. In addition, the new testing code doesn't mock versions (and it would be tricky to change it to do so), so this change would also prevent folks from writing tests that won't work.

One final note If you don't mind, I would appreciate if you don't land this until I can get #490 in, since it is blocking some future testing + tutorials.

@sblackshear
Copy link
Collaborator

#490 is in--sorry for the conflicts!

@666lcz 666lcz merged commit c697bcf into main Feb 21, 2022
@666lcz 666lcz deleted the rename-id-type branch February 21, 2022 20:05
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cleanup] The ID type in our Move framework should no longer be called ID
2 participants