[account-address] Make display and hex outputs consistent #54

gregnazario · 2022-04-19T02:43:53Z

Motivation

Now, all default outputs should be 0xCAPS. Additionally, there is now a from_hex_fuzzy() which handles both 0x and non-0x inputs for the time being.

This is built as follows:

from_hex -> Only hex characters, exact length
from_hex_literal -> Only 0x with hex characters, with length extension
from_hex_fuzzy -> Match either condition
short_str_lossless -> With 0x, but drop leading 0s
Drop the to_hex and to_hex_literal since we can now use Display or Debug using the proper formatter. Additionally to remove any confusion around it. All of are now consistent as upper case.

#53

gregnazario · 2022-04-19T02:44:04Z

@tnowacki lmk how this looks

tnowacki · 2022-04-19T22:51:23Z

Overall it looks fine to me! but I imagine there might be some changes if you have tests to fix (they are all red, not sure if related, looks like it might not be). Just ping me again when things are passing :)

gregnazario · 2022-04-20T17:08:01Z

@tnowacki it looks like there is a compilation issue in addition to the tests failing, though the compilation issue doesn't look like it's due to me

language/move-ir-compiler/move-ir-to-bytecode/syntax/src/syntax.rs

tnowacki · 2022-04-20T17:29:41Z

language/testing-infra/transactional-test-runner/src/tasks.rs

 impl RawAddress {
    pub fn parse(s: &str) -> Result<Self> {
-        if let Ok(addr) = parse_address_literal(s) {
+        if let Ok(addr) = AccountAddress::from_hex_fuzzy(s) {


This isn't the same behavior. WIthout the 0x parses from a decimal value, and that should be the behavior for this, and all CLI tools, to match the source language

parse_address_literal was already doing fuzzy matching. So, if that's the case I'm not making this any more wrong, but it was wrong in the first place

This the wrong sort of fuzzy parsing though. It should be following the source languages convention: 0x is hex, otherwise decimal

But then again.... this is mostly for identifiers, so maybe we should just force the 0x

Should probably reserve the dec/hex parsing for args

tnowacki · 2022-04-20T17:47:41Z

language/tools/move-cli/src/sandbox/commands/run.rs

@@ -66,7 +66,7 @@ move run` must be applied to a module inside `storage/`",

    let signer_addresses = signers
        .iter()
-        .map(|s| AccountAddress::from_hex_literal(s))
+        .map(|s| AccountAddress::from_hex_fuzzy(s))


maybe we should use the source language parsing here?

And I was surprised to not a see a change for other arg parsing. Have you looked at that?

I'm trying to scope down my stuff to AccountAddresses for now, we can come back and do other args later.

tnowacki · 2022-04-20T17:49:25Z

language/tools/move-package/src/source_package/manifest_parser.rs

@@ -213,7 +213,10 @@ pub fn parse_addresses(tval: TV) -> Result<PM::AddressDeclarations> {
                        } else if addresses
                            .insert(
                                ident,
-                                Some(parse_address_literal(entry_str).context("Invalid address")?),
+                                Some(
+                                    AccountAddress::from_hex_fuzzy(entry_str)


I think these should not be fuzzy. We should force 0x for package addresses

Didn't we add a test for that on the last PR?

The original one parse_address_literal was fuzzy, so I'm unsure there. I can switch it back.

Let's make sure we add back the dropped in tests from #59, those should test this change here one way or another

mkurnikov · 2022-04-22T07:59:57Z

Does this fix diem/move#141 ?

tnowacki · 2022-04-22T16:11:55Z

Does this fix diem/move#141 ?

It should at least make it easier to fix :P, unsure right now if it fixes it or not

tnowacki

Let's switch the calls in manifest_parser to from_hex_literal. Requesting changes to make sure we get those tests back from the revert.

Thanks for all of this! It should have been done long ago

gregnazario · 2022-04-28T19:15:17Z

Let's switch the calls in manifest_parser to from_hex_literal. Requesting changes to make sure we get those tests back from the revert.

Thanks for all of this! It should have been done long ago

Sorry, been very distracted on other things, I'll probably come back on a pass through this in a week or two.

tnowacki · 2022-05-09T16:48:54Z

@gregnazario, ping me when you want another review! (looks like you are still working through some changes)

This reverts commit 443c45a.

* Revert "[bytecode verifier] Meter type instantiations (#64)" This reverts commit 803d4d1. * Revert "[bytecode verifer] Adjust metering to decrease runtime of some tests. (#62)" This reverts commit 459029f. * Revert "[bytecode-verifier] Add metering logic and apply to absint based analysis (#58)" This reverts commit 14d624d. * Revert "copyloc-pop test (#54)" This reverts commit 443c45a. * Revert "[verifier] fix incorrect error code for per-module back edge limit check" This reverts commit 1926f1a. * Revert "[verifier] limit the number of back edges" This reverts commit ef2f4d4. * Revert "[language] add catch_unwind panic handling (move-language#750)" This reverts commit b110827.

* [verifier] limit the number of back edges * [verifier] fix incorrect error code for per-module back edge limit check * copyloc-pop test (#54) * [gas] allow natives to read the gas balance * [bytecode-verifier] Add metering logic and apply to absint based analysis (#58) This adds a simple meter to the bytecode verifier which counts the number of abstract operations performed and can enforce a limit. The meter is for now only connected to locals and reference analysis, but plumped to all phases of the verifier so they can easily make use of it. A set of test cases have been added which exercise the new meter for a number of known pathological cases. PR history: - Add metering in type safety, to capture cost of very large types. This reduces timing of large_type_test to 1/4 - Adjusting max metering units upwards and adding a new sample which needs it - Addressing reviewer comments - Add links to security advisories, and verify that all are covered. - Switching metering granularity from function to module. - Adding non-linear growing penalty to using input refs x output refs relations (bicycles), for dealing better with `test_bicliques`. Adding printing size in benchmarks. * [bytecode verifer] Adjust metering to decrease runtime of some tests. (#62) Specifically the test below now runs in 1/2 of the time. This adjustment appeard useful because the overall time allocation had to be increased to 8000 million units in production. Adjusted this as the default here too. ``` --> test_merge_state: verification time: 59.414ms, result: CONSTRAINT_NOT_SATISFIED, size: 63kb ``` Also adjusts the default to what aptos uses now in production. * [bytecode verifier] Meter type instantiations (#64) Instead of just metering size of types on the operand stack, also meter size of type instantiations in calls and other places. This e.g. capture the size of types in calls like `f<T>()`, where the type does not appear on the operand stack. --------- Co-authored-by: Victor Gao <[email protected]> Co-authored-by: Teng Zhang <[email protected]>

gregnazario mentioned this pull request Apr 19, 2022

[Feature Request] Make AccountAddress formatting consistent aptos-labs/aptos-core#534

Closed

3 tasks

gregnazario force-pushed the make-account-address-hex-consistent branch 2 times, most recently from 244bf78 to 4684dca Compare April 19, 2022 19:01

gregnazario force-pushed the make-account-address-hex-consistent branch from 4684dca to 01e13bb Compare April 20, 2022 17:22

tnowacki reviewed Apr 20, 2022

View reviewed changes

language/move-ir-compiler/move-ir-to-bytecode/syntax/src/syntax.rs Outdated Show resolved Hide resolved

gregnazario force-pushed the make-account-address-hex-consistent branch from 01e13bb to b76efe2 Compare April 20, 2022 17:39

tnowacki reviewed Apr 20, 2022

View reviewed changes

gregnazario force-pushed the make-account-address-hex-consistent branch from b76efe2 to 74fb5f8 Compare April 20, 2022 17:52

gregnazario mentioned this pull request Apr 20, 2022

Revert "[account-address] Input and output with a 0x for hex output (… #59

Merged

gregnazario force-pushed the make-account-address-hex-consistent branch from 74fb5f8 to 8f012d4 Compare April 20, 2022 18:13

tnowacki suggested changes Apr 22, 2022

View reviewed changes

gregnazario force-pushed the make-account-address-hex-consistent branch 4 times, most recently from 0af42bc to 369402e Compare May 8, 2022 06:53

[account-address] Make display and hex outputs consistent

d9ab787

gregnazario force-pushed the make-account-address-hex-consistent branch from 369402e to d9ab787 Compare May 8, 2022 06:53

gregnazario closed this Aug 2, 2022

wrwg pushed a commit to wrwg/move-lang that referenced this pull request Feb 10, 2023

copyloc-pop test (move-language#54)

c763ca4

vgao1996 pushed a commit to vgao1996/move that referenced this pull request Feb 27, 2023

copyloc-pop test (move-language#54)

9edd3fe

vgao1996 pushed a commit that referenced this pull request Feb 27, 2023

copyloc-pop test (#54)

a596557

vgao1996 pushed a commit to vgao1996/move that referenced this pull request Feb 27, 2023

copyloc-pop test (move-language#54)

9947233

vgao1996 pushed a commit that referenced this pull request Feb 28, 2023

copyloc-pop test (#54)

ddfc83b

nkysg referenced this pull request in starcoinorg/move Feb 28, 2023

copyloc-pop test (#54)

fe6a6c2

nkysg referenced this pull request in starcoinorg/move Feb 28, 2023

copyloc-pop test (#54)

443c45a

nkysg referenced this pull request in starcoinorg/move Feb 28, 2023

Revert "copyloc-pop test (#54)"

f45ad12

This reverts commit 443c45a.

wrwg pushed a commit to wrwg/move-lang that referenced this pull request Mar 1, 2023

copyloc-pop test (move-language#54)

2a48403

banool mentioned this pull request Mar 17, 2023

Add load_function_without_types to VM session and loader #990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[account-address] Make display and hex outputs consistent #54

[account-address] Make display and hex outputs consistent #54

gregnazario commented Apr 19, 2022

gregnazario commented Apr 19, 2022

tnowacki commented Apr 19, 2022

gregnazario commented Apr 20, 2022

tnowacki Apr 20, 2022

gregnazario Apr 20, 2022

tnowacki Apr 20, 2022

tnowacki Apr 20, 2022

tnowacki Apr 20, 2022

tnowacki Apr 20, 2022

gregnazario Apr 20, 2022

tnowacki Apr 20, 2022

tnowacki Apr 20, 2022

gregnazario Apr 20, 2022

tnowacki Apr 20, 2022

mkurnikov commented Apr 22, 2022

tnowacki commented Apr 22, 2022

tnowacki left a comment

gregnazario commented Apr 28, 2022

tnowacki commented May 9, 2022

[account-address] Make display and hex outputs consistent #54

[account-address] Make display and hex outputs consistent #54

Conversation

gregnazario commented Apr 19, 2022

Motivation

gregnazario commented Apr 19, 2022

tnowacki commented Apr 19, 2022

gregnazario commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurnikov commented Apr 22, 2022

tnowacki commented Apr 22, 2022

tnowacki left a comment

Choose a reason for hiding this comment

gregnazario commented Apr 28, 2022

tnowacki commented May 9, 2022