Use ObjectID over AccountAddress #468
Conversation
oxade
requested review from
lxfind,
sblackshear,
huitseeker,
arun-koshy,
awelc,
gdanezis,
lanvidr,
patrickkuo,
stella3d and
velvia
|
For the functions that are exactly the same between ObjectID and AccountAddress, could we simply call into AccountAddress for that? That reduces duplication and allows us to reuse as much as possible but keep the differences. |
That's what's going on, but I still need the wrappers for the Rust type system since they're two different type. |
|
|
||
| /// Converts from hex string to ObjectID where the string is prefixed with 0x | ||
| /// Its okay if the strings are less than expected | ||
| pub fn from_hex_literal(literal: &str) -> Result<Self, ObjectIDParseError> { |
There was a problem hiding this comment.
I believe we could do much more to dedup.
For example, for from_hex_literal function, we could very well call into AccountAddress::from_hex_literal, and map the error for return.
There was a problem hiding this comment.
Right this was somewhat intentional. AccountAddress impls, will return AccountAddressParseError => "unable to parse AccountAddress" on any failure which is not descriptive enough and also confusing for users because it's ObjectIDs we're after (saw this scenario once). I'm trying to be descriptive on why things failed. Doing this unfortunately requires doing some checks of our own. It also makes sense to control our parsing logic/errors since this is user-facing. If I just map_err AccountAddressParseError, to ObjectIDParseError, it still doesn't convey all info because AccountAddressParseError disregards details.
But if you have an approach that returns clear errors and also avoids duplication, please make a commit and I'll take a look.
There was a problem hiding this comment.
I see. Let me file a separate issue to add better error message in Move repo for AccountAddressParseError.
There was a problem hiding this comment.
No problemo. Thank you
+1 to this! |
|
@lxfind @sblackshear any other comments on this?
Existing issue in many other files. |
|
@lxfind @sblackshear is there's nothing else, please approve when possible so I can land it. |
sui_types/src/base_types.rs
Outdated
| /// Hex address: 0x0 | ||
| pub const ZERO: Self = Self::new([0u8; Self::LENGTH]); | ||
| /// Hex address: 0x1 | ||
| pub const ONE: Self = Self::get_hex_address_one(); |
sui_types/src/base_types.rs
Outdated
| } | ||
|
|
||
| impl std::ops::Deref for ObjectID { | ||
| type Target = [u8; Self::LENGTH]; |
There was a problem hiding this comment.
I think what @sblackshear meant here is probably to deref it to AccountAddress, instead of slice. But I will let Sam to confirm.
You might want to consider using stacked PR if this is blocking you. |
ObjectID now wraps
move_core_types::account_address::AccountAddressinstead of being a raw alias.This reduces confusion in code and error reporting.
Fixes #463
Few things:
sui_programmabilitywas largely left untouched since this is the boundary between Move (using AccountAddress) and Rust (using ObjectID).base-typesfile is growing huge so I can potentially separate out the ObjectID impls if that's a problem