[account-address] Make account address consistent always full with 0x in front

[gregnazario]

Motivation

This is my third attempt at it, but we should have 0x....1 rather than 0x1. This makes it very confusing for lots of people.

Additionally, no 000001, or uppercase, or mixed outputs, always the full 0x000000...001

An example is below, where you can see the module ID isn't the same as the stake.

Additionally, all the compiler outputs aren't the same as well (as you can see in the baseline tests)

thread 'main' panicked at 'Error calling genesis.create_initialize_validators: VMError with status ABORTED with sub status 65540 at location Module ModuleId { address: 0000000000000000000000000000000000000000000000000000000000000001, name: Identifier("stake") } and message 0x0000000000000000000000000000000000000000000000000000000000000001::stake::join_validator_set_internal at offset 45 at code offset 45 in function definition 31', aptos-move/vm-genesis/src/lib.rs:240:13
stack backtrace:

Have you read the Contributing Guidelines on pull requests?

Yes, of course I have

Test Plan

All the tests should be updated!

[gregnazario]

@bmwill do you know who's handling the CI, looks like it's trying to install dependencies but failing https://github.com/move-language/move/runs/8217066801?check_suite_focus=true

[bmwill]

@gregnazario I'll ping some folks and see who owns it.

[gregnazario]

The prover succeeds on my machine and fails on the server.

I suspect this is a CVC / BOOGIE versioning problem...

[gregnazario]

We accept both 0xADDRESS or ADDRESS in from_str, but only 0xADDRESS in from_hex_literal.

And now replace from_hex_literal usage cases with from_str in this PR, maybe cause some ambiguity issue.

Should we let from_str == from_hex_literal?

impl FromStr for AccountAddress {
    type Err = AccountAddressParseError;
    fn from_str(s: &str) -> Result<Self, AccountAddressParseError> {
        // Accept 0xADDRESS or ADDRESS
        if let Ok(address) = AccountAddress::from_hex_literal(s) {
            Ok(address)
        } else {
            Self::from_hex(s)
        }
    }
}

For backwards compatibility purposes, this is doing fuzzy matching

[nanne007]

Glad to see this! we at starcoin have been using 0x0000000...0000001 long time ago due to the same reason (0x1 can be confusing and only these special addresses are shortend)

[tnowacki]

I'll also add at least for historical data, that @sblackshear and myself worked a while back to do exactly the opposite of this PR, which was to trim the leading zeros from the addresses (and as you are finding here, it took a while to do that). As such, the motivation for this PR might need some broader discussion.

[wrwg]

I'll also add at least for historical data, that @sblackshear and myself worked a while back to do exactly the opposite of this PR, which was to trim the leading zeros from the addresses (and as you are finding here, it took a while to do that). As such, the motivation for this PR might need some broader discussion.

It may have been lost in the discussion but I also asked for killing the redundant leading zeros as a requirement for moving forward with this, and @gregnazario said that would be easily possible with the groundwork being done here.

[gregnazario]

I'll also add at least for historical data, that @sblackshear and myself worked a while back to do exactly the opposite of this PR, which was to trim the leading zeros from the addresses (and as you are finding here, it took a while to do that). As such, the motivation for this PR might need some broader discussion.

It may have been lost in the discussion but I also asked for killing the redundant leading zeros as a requirement for moving forward with this, and @gregnazario said that would be easily possible with the groundwork being done here.

Yeah, realistically it's now a couple line change, plus regeneration of baseline files of course

[gregnazario]

I'll also add at least for historical data, that @sblackshear and myself worked a while back to do exactly the opposite of this PR, which was to trim the leading zeros from the addresses (and as you are finding here, it took a while to do that). As such, the motivation for this PR might need some broader discussion.

I'm not opposed to removing leading 0s, just keeping in mind it'll be the norm anyways for only special addresses. Most addresses will be filled with characters anyways. Whichever works, I just want it to be the same everywhere!

[jolestar]

I do not think using the short representation of addresses as default is a good idea.

The full string representation is the address memory binary's hex string format, it is default. We do not need to do extra jobs for aligning the implementation in different SDKs or off-chain usage.

Such as some Apps use the address or module id as a key of the hashmap, or directly save the address or module id to the database for the query. Those Apps need to consider which addresses need to trim the leading zeros, but only a few addresses benefit from shortening.

I think the chain is the App's infrastructure and should be prioritized to ensure program-friendly rather than end-user-friendly.

If we want end-users to see shorter addresses, we can do shortening on the wallet or block explorer, not need to do this at the API layer.

[wrwg]

I do not think using the short representation of addresses as default is a good idea.

The full string representation is the address memory binary's hex string format, it is default. We do not need to do extra jobs for aligning the implementation in different SDKs or off-chain usage.

Such as some Apps use the address or module id as a key of the hashmap, or directly save the address or module id to the database for the query. Those Apps need to consider which addresses need to trim the leading zeros, but only a few addresses benefit from shortening.

I think the chain is the App's infrastructure and should be prioritized to ensure program-friendly rather than end-user-friendly.

If we want end-users to see shorter addresses, we can do shortening on the wallet or block explorer, not need to do this at the API layer.

I'm puzzled by this. This is about the to_string representation, no one should use that as a key for maps.

No to_string in the world on numbers prepends a bunch of leading zeros. No one does that. Why are we doing this here?

The few cases where it matters are important for having readable examples, especially if if you have 32 bytes address length like Aptos.

If this is a compatible issue, we may start to make this configurable somehow via a feature flag.

[jolestar]

No to_string in the world on numbers prepends a bunch of leading zeros. No one does that. Why are we doing this here?

As I say, we need to align the implementations in different SDKs or off-chain usage to ensure this.

If this is a compatible issue, we may start to make this configurable somehow via a feature flag.

Making this configurable as a feature flag is a solution.

[jiangying000]

Intuitively，I'd support default output 0x full length address for simplicity, but of course, a configurable flag for this is enough.

Also, curious why the commit https://github.com/move-language/move/pull/441/commits/6e711e5c0459f1d5d6879a162f1efbbf3e232983 is so large ?

[gregnazario]

Intuitively，I'd support default output 0x full length address for simplicity, but of course, a configurable flag for this is enough.

Also, curious why the commit 6e711e5 is so large ?

I'm not sure how this commit became so large, it will have to be cleaned up

[jolestar]

Can we make this PR move on? this PR changes too many files and easily conflicts with other PRs.

[lightmark]

Bear with me, just wanna share my two cents since I also got complaints from move dapps developers.

I'm puzzled by this. This is about the to_string representation, no one should use that as a key for maps.

Dapp developers never read all the VM code but they usually just assume to_string from an address is canonical. If it is canonical, there is no reason to not use the output as a key in hashmap of other languages.

No to_string in the world on numbers prepends a bunch of leading zeros. No one does that. Why are we doing this here?

Agree with numbers. But AFAIU we are talking about address. Is address a number? yes and no. I think we all agree address is represented as an n-byte array, though it can also be treated as a 8*n bit number. But IMO in most context, it is used as an array but not a number. For example, in merkle tree addressing, we iterate 256 bits one by one to locate a leaf, which reflects that the leading zeros are meaningful data. On the other hand, for addresses with different lengths, full-length address with leading zeros gives more semantic information. With 0x1, you cannot tell it is a 16-byte address or 32-byte address, while with 0x00000000000000000000000000000001, you are sure the address space is 16 bytes/128 bits. And for debugging purpose, it is always preferable to remove trailing zeros but this can be a feature only for debugging.

Anyways, I think the bottomline is to make it consistent. I didn't read the code but heard it is not consistent now across the codebase from @gregnazario .

[jolestar]

@wrwg @sblackshear Can we merge this PR?

[sblackshear]

@wrwg @sblackshear Can we merge this PR?

Put this on the agenda for the community meeting tomorrow

[wrwg]

As discussed in various forums, a change here has become infeasible because of compatibility. Rather we have introduced AccountAddress::to_canonical_string which can be used to incrementally update representation. Closing this PR.