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

Unify the size of SuiAddress and ObjectID (also move AccountAddress) #540

Merged
merged 4 commits into from
Feb 24, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 23, 2022

ObjectID and Move AccountAddress have been 20 bytes, while SuiAddress has been 32 bytes.
This difference in size made it impossible to use the native address Move type to represent SuiAddress. We have been using a custom defined Address wrapper of raw bytes to represent SuiAddress.

Now that we are aligned that SuiAddress and Move AccountAddress can be the same length, this PR aligns them and removes the Address type from Sui framework. (See #519)
Specifically, this PR does the following:

  1. Deleted Address.move, which removes both Address type and Signer type from the framework. All uses are replaced with the native type address and signer.
  2. In all places where we are passing a vector to represent SuiAddress, now we can use AccountAddress type.
  3. Added a native function new_signer_from_address to create a signer from an address, for testing purpose only.

A few TODOs left:

  1. We probably can remove the type IDBytes from Sui Move framework, as it's now just a wrapper of address.
  2. We now have a few types that all share the same inner content: ObjectID, AccountAddress, SuiAddress. The conversion logic is getting a bit messy. There is probably some cleanup we could do there.
  3. Once we decide on the length, we could adjust the length properly. (if we decide on 32 bytes, we will need to add that support to Move repo, a trivial change though)

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Love it! The tiny changes, like the AccountAddress::from instances, are super nice and more maintainable.

@sblackshear
Copy link
Collaborator

Hugely in favor of removing IDBytes later!

@@ -582,7 +582,7 @@ async fn test_handle_move_transaction() {

// Check that gas is properly deducted.
// If the number changes, we want to verify that the change is intended.
let gas_cost = 62;
let gas_cost = 53;
Copy link
Contributor

Choose a reason for hiding this comment

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

where do we get this number from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually looked at the gas used.

@oxade
Copy link
Contributor

oxade commented Feb 23, 2022

Love that SuiAddress is no longer Vec!

…ytes

This changes from Addresses on 32 bytes.
There is no situation where the `TryFrom<&[u8]>` isn't more flexible and preferred.
@lxfind lxfind merged commit dba291b into main Feb 24, 2022
@lxfind lxfind deleted the unify-addresses branch February 24, 2022 02:05
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.

4 participants