[CLI][Move] support public structs/enums as txn args in CLI#18591
[CLI][Move] support public structs/enums as txn args in CLI#18591rahxephon89 wants to merge 2 commits intoteng/public-struct-txn-argsfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
88acdd2 to
dfd47a0
Compare
fc1c26d to
d31e4d0
Compare
155b20c to
398a435
Compare
f37ad1b to
b30d55a
Compare
170c1ef to
e22d876
Compare
8f27993 to
dd7a3d2
Compare
bfc10dc to
ff0ca1f
Compare
dd7a3d2 to
966306b
Compare
ff0ca1f to
12c09c2
Compare
12c09c2 to
f69d861
Compare
966306b to
a6c536f
Compare
aptos-move/move-examples/cli-e2e-tests/common/sources/cli_e2e_tests.move
Outdated
Show resolved
Hide resolved
f69d861 to
faf499b
Compare
| fn check_depth(depth: u8, type_name: &str) -> CliTypedResult<()> { | ||
| if depth > MAX_NESTING_DEPTH { | ||
| return Err(CliError::CommandArgumentError(format!( | ||
| "{} nesting depth {} exceeds maximum allowed depth of {}", |
There was a problem hiding this comment.
| "{} nesting depth {} exceeds maximum allowed depth of {}", | |
| "`{}` nesting depth {} exceeds maximum allowed depth of {}", |
| } | ||
|
|
||
| /// Construct a struct argument by parsing fields and encoding to BCS. | ||
| pub async fn construct_struct_argument( |
There was a problem hiding this comment.
I am curious about the necessity of getting struct def from rest API for verifying the args. It seems to bring in extra complexities.
There was a problem hiding this comment.
if we cannot get the abi, we have to deserialize the module to get enough information
| ) -> CliTypedResult<Vec<u8>> { | ||
| // Check nesting depth limit | ||
| Self::check_depth(depth, "Struct")?; | ||
| let module = self.verify_struct_exists(struct_tag).await?; |
There was a problem hiding this comment.
There is repeated work by verify_struct_exists and the code below (e.g., potential deserialization of the bytecode module). Maybe creating a separate data struct for onchain modules, where we can cache and reuse the analysis results along the way.
| } | ||
|
|
||
| /// Convert MoveType to TypeTag. | ||
| fn convert_move_type_to_type_tag(move_type: &MoveType) -> CliTypedResult<TypeTag> { |
There was a problem hiding this comment.
It's worth checking if we already have APIs to do this and other convertions defined in this file.
| } | ||
|
|
||
| /// Parse a value based on its Move type and encode to BCS. | ||
| fn parse_value_by_type<'a>( |
There was a problem hiding this comment.
Can we simply make it a async fn so that we can simplify the return type?
There was a problem hiding this comment.
Also, some abstraction would help this function to be more readable and maintainable.
| ))); | ||
| } | ||
|
|
||
| match move_type { |
There was a problem hiding this comment.
Do we have existing APIs doing this thing?
| })?; | ||
| bcs::to_bytes(&v).map_err(|e| CliError::BCS("bool", e)) | ||
| }, | ||
| MoveType::U8 => { |
There was a problem hiding this comment.
The parsing here and below has an inconsistent error handling with MoveType::Bool.
There was a problem hiding this comment.
do you mean the error type is different?
| // 1. Array format (legacy): [] or [value] | ||
| // 2. Enum format (new): {"None": {}} or {"Some": {"0": value}} | ||
|
|
||
| if value.is_array() { |
There was a problem hiding this comment.
I remember to have seen similar code in several other places? Maybe making a function for it?
| } | ||
| } | ||
|
|
||
| match qualified_name.as_str() { |
There was a problem hiding this comment.
Can we have a more systematic and generic way to handle these special cases? I suppose they will grow as time goes by.
There was a problem hiding this comment.
this corresponds to allowed structs so I will keep it as it is and add TODO once we have a more general way to handle them
| let s = if value.is_string() { | ||
| value.as_str().unwrap() | ||
| } else if value.is_number() { | ||
| &value.to_string() |
There was a problem hiding this comment.
We use value.as_str().unwrap() above and &value.to_string() here. Is it intended?
|
@rahxephon89 I did a first look at the PR. The code makes sense but can benefit from some clean-up and refactoring. I will take another look afterward. |
5e897f2 to
dac7a55
Compare
a6c536f to
810bb48
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
dac7a55 to
b89306f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
b89306f to
de9d553
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
de9d553 to
a91387e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
a91387e to
ccaeb25
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
ccaeb25 to
3a1dc50
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
3a1dc50 to
498bca7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
498bca7 to
0904383
Compare

Description
This PR:
How Has This Been Tested?
Newly added cli e2e tests.
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist
Note
Medium Risk
Touches CLI transaction argument parsing and encoding, adding async REST-dependent logic that could break
move run/view/simulatefor JSON inputs if type detection or BCS encoding is incorrect. Local-testnet orchestration changes also affect e2e execution flow and process lifecycle management.Overview
Adds public struct/enum transaction argument support to the Aptos CLI when using
--json-file, including enum variant/object syntax andOption<T>in both legacy vector ([]/[v]) and new variant ({"None":{}}/{"Some":{"0":v}}) formats. This introduces an async parsing path that fetches on-chain module bytecode/ABI to validate types and BCS-encode nested structs/enums, implemented via a newStructArgParserwith module caching and depth limits.Updates
aptos move run,move simulate, andmove viewto use the async parser only when JSON input is present, and hard-errors if struct/enum types are encountered in the previous synchronous conversion path. Adds a new Move test package and Python e2e cases to exercise struct/enum/Option argument handling, and extends the e2e harness with--use-local-testnet(auto-start or manual) to run tests without Docker. Also introduces sharedMODULE_SEPARATOR/std-type constants inmove-core-typesand addsasync-recursionto the CLI crate.Written by Cursor Bugbot for commit 0904383. This will update automatically on new commits. Configure here.