[CLI] Set Move 2 as default in Aptos CLI and add `move-1` flag #15431

rahxephon89 · 2024-11-30T04:08:59Z

Description

This PR

sets Compiler v2 as the default compiler and Move 2 as the default language version.
adds new --move-1 flag to use Compiler v1 and Move 1.

How Has This Been Tested?

Manually test aptos move compile and aptos move publish will compile and publish modules generated by compiler v2.

Manually test aptos move compile --move-1 and aptos move publish --move-1 will compile and publish modules generated by compiler v1.

Manually test aptos move compile --move-2 and aptos move publish --move-2 will compile and publish modules generated by compiler v2.

Manually test aptos move test with: (a) --move-2, (b) --move-1, (c) neither. (a) and (c) does not panic on MVC_BLOCK_V1=1 while (b) does.

Manually test aptos move run-script with: (a) --move-2, (b) --move-1, (c) neither. (a) and (c) does not panic on MVC_BLOCK_V1=1 while (b) does.

Manually test aptos governance propose with: (a) --move-2, (b) --move-1, (c) neither. (a) and (c) does not panic on MVC_BLOCK_V1=1 while (b) does.

Manually test aptos governance verify-proposal with: (a) --move-2, (b) --move-1, (c) neither. (a) and (c) does not panic on MVC_BLOCK_V1=1 while (b) does.

Key Areas to Review

Whether this change covers all the use cases of using the compiler.
Also we need to make sure that this change should be independent of changing V2 as the default compiler for aptos-core

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-11-30T04:09:02Z

⏱️ 1h 13m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-tests	13m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-cargo-deny	9m	⬜ 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	9m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	6m	⬜
rust-move-tests	4m	⬜
general-lints	3m	🟩 🟩 🟩 🟩 🟩 (+1 more)
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	46s	⬜
permission-check	24s	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	18s	🟩 🟩 🟩 🟩 🟩 (+2 more)

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

crates/aptos/src/common/types.rs

crates/aptos/src/move_tool/mod.rs

banool · 2025-01-08T10:56:49Z

crates/aptos/src/common/types.rs

            skip_attribute_checks: false,
            check_test_code: false,
-            move_2: false,
+            move_2: true,
+            move_1: false,


Should there be a move_version field that takes an enum instead? Because I assume it's invalid to set both to true or false?

Agree, but perhaps we should have either just keep this as move_2 with true the default, or remove one of the flags. Enum seems overkill.

Hi @banool, I was thinking of the same idea but we may still need to keep the move-2 flag in case some eco system projects depend on this flag to compile their smart contracts in CI? Enum seems be an overkill because move-1 will only be used in urgent time and will be removed when V1 retires. Also, external users cannot set both of them to the same value because they are put in the same ArgGroup.

wrwg

Approve modulo addressing comments

wrwg · 2025-01-08T16:45:47Z

crates/aptos/src/common/types.rs

            skip_attribute_checks: false,
            check_test_code: false,
-            move_2: false,
+            move_2: true,
+            move_1: false,


Agree, but perhaps we should have either just keep this as move_2 with true the default, or remove one of the flags. Enum seems overkill.

wrwg · 2025-01-08T16:47:28Z

crates/aptos/src/move_tool/mod.rs

@@ -564,8 +564,14 @@ impl CliCommand<&'static str> for TestPackage {
                    self.move_options.bytecode_version,
                    self.move_options.language_version,
                ),
-                compiler_version: self.move_options.compiler_version,
-                language_version: self.move_options.language_version,
+                compiler_version: self


Why are these changes needed? This should just be the same logic, based on different defaults in MoveOptions?

I made this change mainly to make sure that so long as move_options is not set to V1 explicitly, it will always pass V2 to the compiler config.

Co-authored-by: Vineeth Kashyap <[email protected]>

github-actions · 2025-01-13T17:09:54Z

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `318452ab965c7ae6db3d55d97b3bac33b525c867`

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> 318452ab965c7ae6db3d55d97b3bac33b525c867 (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 16996.89 txn/s, latency: 1995.20 ms, (p50: 1800 ms, p70: 1900, p90: 2500 ms, p99: 4500 ms), latency samples: 566700
2. Upgrading first Validator to new version: 318452ab965c7ae6db3d55d97b3bac33b525c867
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7410.40 txn/s, latency: 4002.10 ms, (p50: 4700 ms, p70: 4900, p90: 5000 ms, p99: 5100 ms), latency samples: 138460
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7226.25 txn/s, latency: 4589.87 ms, (p50: 5000 ms, p70: 5100, p90: 5100 ms, p99: 5200 ms), latency samples: 247560
3. Upgrading rest of first batch to new version: 318452ab965c7ae6db3d55d97b3bac33b525c867
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6867.01 txn/s, latency: 4364.36 ms, (p50: 5000 ms, p70: 5300, p90: 5700 ms, p99: 5800 ms), latency samples: 126980
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6879.54 txn/s, latency: 4908.60 ms, (p50: 5300 ms, p70: 5400, p90: 5600 ms, p99: 6100 ms), latency samples: 232240
4. upgrading second batch to new version: 318452ab965c7ae6db3d55d97b3bac33b525c867
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11205.92 txn/s, latency: 2606.88 ms, (p50: 2700 ms, p70: 3100, p90: 3600 ms, p99: 3900 ms), latency samples: 195240
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5221.34 txn/s, submitted: 5221.49 txn/s, expired: 0.15 txn/s, latency: 2712.38 ms, (p50: 2800 ms, p70: 3000, p90: 3100 ms, p99: 3500 ms), latency samples: 390869
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> 318452ab965c7ae6db3d55d97b3bac33b525c867 passed
Test Ok

github-actions · 2025-01-13T17:12:22Z

✅ Forge suite `realistic_env_max_load` success on `318452ab965c7ae6db3d55d97b3bac33b525c867`

two traffics test: inner traffic : committed: 14842.55 txn/s, submitted: 14842.76 txn/s, expired: 0.21 txn/s, latency: 2678.23 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3800 ms), latency samples: 5643460
two traffics test : committed: 99.97 txn/s, latency: 1285.89 ms, (p50: 1300 ms, p70: 1300, p90: 1500 ms, p99: 1600 ms), latency samples: 1940
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.651, avg: 1.554", "ConsensusProposalToOrdered: max: 0.292, avg: 0.288", "ConsensusOrderedToCommit: max: 0.316, avg: 0.302", "ConsensusProposalToCommit: max: 0.605, avg: 0.590"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.76s no progress at version 54399 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.56s no progress at version 1089450 (avg 0.54s) [limit 16].
Test Ok

graphite-app bot reviewed Nov 30, 2024

View reviewed changes

crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved

rahxephon89 force-pushed the teng/add-move-1-flags branch 3 times, most recently from f2cbfa0 to 66949be Compare November 30, 2024 05:52

graphite-app bot reviewed Nov 30, 2024

View reviewed changes

crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved

rahxephon89 force-pushed the teng/add-move-1-flags branch from 66949be to 072f8a1 Compare November 30, 2024 05:56

rahxephon89 changed the title ~~[CLI][WIP] Set Move 2 as default in Aptos CLI and add move-1 flag~~ [CLI] Set Move 2 as default in Aptos CLI and add move-1 flag Nov 30, 2024

rahxephon89 requested review from vineethk and brmataptos November 30, 2024 08:01

rahxephon89 marked this pull request as ready for review November 30, 2024 08:01

rahxephon89 requested review from gregnazario and banool as code owners November 30, 2024 08:01

brmataptos approved these changes Nov 30, 2024

View reviewed changes

crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved

crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved

vineethk reviewed Dec 2, 2024

View reviewed changes

crates/aptos/src/common/types.rs Show resolved Hide resolved

crates/aptos/src/common/types.rs Show resolved Hide resolved

rahxephon89 force-pushed the teng/add-move-1-flags branch from 072f8a1 to b05cbeb Compare December 2, 2024 18:35

rahxephon89 commented Dec 2, 2024

View reviewed changes

crates/aptos/src/move_tool/mod.rs Show resolved Hide resolved

rahxephon89 requested a review from vineethk December 2, 2024 18:37

rahxephon89 force-pushed the teng/add-move-1-flags branch from b05cbeb to c1a821b Compare December 15, 2024 01:39

rahxephon89 force-pushed the teng/add-move-1-flags branch from c1a821b to 20faae0 Compare January 6, 2025 17:46

rahxephon89 requested a review from wrwg January 6, 2025 18:34

rahxephon89 force-pushed the teng/add-move-1-flags branch 3 times, most recently from 9b6983d to 2f9e3bb Compare January 7, 2025 08:31

vineethk approved these changes Jan 8, 2025

View reviewed changes

crates/aptos/src/move_tool/mod.rs Outdated Show resolved Hide resolved

vineethk approved these changes Jan 8, 2025

View reviewed changes

rahxephon89 force-pushed the teng/add-move-1-flags branch from 987f4de to 3618442 Compare January 8, 2025 04:57

banool reviewed Jan 8, 2025

View reviewed changes

wrwg approved these changes Jan 8, 2025

View reviewed changes

rahxephon89 requested a review from banool January 8, 2025 20:08

rahxephon89 force-pushed the teng/add-move-1-flags branch 2 times, most recently from 6b4bee7 to 2e4f5d0 Compare January 10, 2025 22:32

rahxephon89 and others added 4 commits January 10, 2025 17:07

add move-1 flag

8a51f84

governance

03bd97e

set default as v2

1748792

Update crates/aptos/src/move_tool/mod.rs

38a1522

Co-authored-by: Vineeth Kashyap <[email protected]>

rahxephon89 force-pushed the teng/add-move-1-flags branch from 2e4f5d0 to 38a1522 Compare January 11, 2025 01:07

rahxephon89 added 2 commits January 12, 2025 00:02

Merge branch 'main' into teng/add-move-1-flags

7c7159a

Merge branch 'main' into teng/add-move-1-flags

318452a

rahxephon89 enabled auto-merge (squash) January 13, 2025 16:42

This comment has been minimized.

Sign in to view

rahxephon89 merged commit ff54cea into main Jan 13, 2025
79 of 88 checks passed

rahxephon89 deleted the teng/add-move-1-flags branch January 13, 2025 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLI] Set Move 2 as default in Aptos CLI and add `move-1` flag #15431

[CLI] Set Move 2 as default in Aptos CLI and add `move-1` flag #15431

rahxephon89 commented Nov 30, 2024 •

edited

Loading

trunk-io bot commented Nov 30, 2024 •

edited

Loading

banool Jan 8, 2025

wrwg Jan 8, 2025

rahxephon89 Jan 8, 2025

wrwg left a comment

wrwg Jan 8, 2025

wrwg Jan 8, 2025

rahxephon89 Jan 8, 2025

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

[CLI] Set Move 2 as default in Aptos CLI and add move-1 flag #15431

[CLI] Set Move 2 as default in Aptos CLI and add move-1 flag #15431

Conversation

rahxephon89 commented Nov 30, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Nov 30, 2024 • edited Loading

banool Jan 8, 2025

Choose a reason for hiding this comment

wrwg Jan 8, 2025

Choose a reason for hiding this comment

rahxephon89 Jan 8, 2025

Choose a reason for hiding this comment

wrwg left a comment

Choose a reason for hiding this comment

wrwg Jan 8, 2025

Choose a reason for hiding this comment

wrwg Jan 8, 2025

Choose a reason for hiding this comment

rahxephon89 Jan 8, 2025

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 13, 2025

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> 318452ab965c7ae6db3d55d97b3bac33b525c867

github-actions bot commented Jan 13, 2025

✅ Forge suite realistic_env_max_load success on 318452ab965c7ae6db3d55d97b3bac33b525c867

[CLI] Set Move 2 as default in Aptos CLI and add `move-1` flag #15431

[CLI] Set Move 2 as default in Aptos CLI and add `move-1` flag #15431

rahxephon89 commented Nov 30, 2024 •

edited

Loading

trunk-io bot commented Nov 30, 2024 •

edited

Loading

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `318452ab965c7ae6db3d55d97b3bac33b525c867`

✅ Forge suite `realistic_env_max_load` success on `318452ab965c7ae6db3d55d97b3bac33b525c867`