support test file creation as a part of test txn generation #15688

yuunlimm · 2025-01-08T20:43:48Z

Description

Currently, external Indexer SDK users who want to test transactions must either maintain their own branch of aptos-core to avoid conflicts or copy our script into their test or processor repositories. This workflow can be confusing, so I’ve updated the transaction generation process to include the conversion step (from proto json to a rust file containing const variables) via new CLI arguments.

Existing Process:

Run the transaction generation CLI to produce JSON files (transaction proto JSON).
A build.rs script in the test-transactions crate converts these JSON files into a Rust source file with const variables.
The processor repository updates its reference to the test-transactions crate.

New Process:

Run the transaction generation CLI, using the new CLI options that let external users specify where to generate and store test transactions in their own repositories.
Use these generated transactions in their test repository.

How Has This Been Tested?

locally tested

cargo run -- --testing-folder ./imported_transactions --output-folder /U
sers/yuunlim/aptos-indexer-processors/rust/integration-tests/json_transactions --rust-file /Users/yuunlim/aptos-indexer-processors/rust/integration-tests/src/txns.rs --create-rust-file

Key Areas to Review

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 · 2025-01-08T20:43:51Z

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

Job	Cumulative Duration	Recent Runs
rust-move-tests	14m	🟩
rust-move-tests	14m	🟩
rust-move-tests	14m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-cargo-deny	9m	🟩 🟩 🟩 🟩 🟩
check-dynamic-deps	6m	🟩 🟩 🟩 🟩 🟩
general-lints	2m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	55s	🟩 🟩 🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩
check-branch-prefix	1s	🟩

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

yuunlimm · 2025-01-08T20:44:04Z

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.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

graphite-app · 2025-01-08T20:44:43Z

ecosystem/indexer-grpc/indexer-transaction-generator/src/transaction_code_builder.rs

+    /**
+    * Adds a directory of JSON files to the transaction code builder.
+    *
+    * @param src_dir: The source directory containing the JSON files
+    * @param dir_name: The name of the directory to be created in the `json_transactions` directory
+    * @param module_name: The name of the module to be used in the constant names
+    * @param generate_name_function: Whether to generate a transaction name lookup function
+    */


The function documentation parameters don't match the actual function signature. The current parameters src_dir and dir_name should be replaced with json_dir. The parameter descriptions should also be simplified to match the actual functionality - this function processes an existing directory of JSON files rather than creating new directories.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-08T21:05:48Z

ecosystem/indexer-grpc/indexer-transaction-generator/src/config.rs

+            match fs::write(dest_path, code) {
+                Ok(_) => println!("Successfully generated the transactions code."),
+                Err(e) => println!("Failed to generate the transactions code for dest_path:{:?}, {:?}", dest_path, e),
+            }
+            Ok(())


The error from fs::write is being silently converted to success. To properly handle failures, replace the match with:

fs::write(dest_path, code).context("Failed to write transactions code")?;

This ensures errors propagate up the call stack while preserving the error context.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

rtso · 2025-01-09T00:50:37Z

For the aptos-indexer-processor test transactions, should we keep it within the aptos-core repo or move it to aptos-indexer-processor repo? I think it makes sense to keep the test transaction json files and the generated Rust file in the same place, and iirc, the json files were kept in aptos-core for the diff checks.

larry-aptos · 2025-01-09T00:54:51Z

The overall approach makes sense: we decouple the json and the code. Only one question regarding to the incompatibility: If jsons are on an older version(say we have 5 jsons) and the code is on a newer version(say we import 6 txns), is it possible to notify/show the users to upgrade to resolve?

it looks like current behavior is compilation error..

rtso · 2025-01-09T00:55:23Z

ecosystem/indexer-grpc/indexer-test-transactions/build.rs

Should this file actually belong inside of indexer-transaction-generator since it's part of transaction generation?

the reason why I left it here is that the I thought we wanted to keep the json files in the aptos-core for the internal usage. so the process of adding new txn for processor testing remains the same, where we would have to update the cargo.toml in the processor repo with the latest commit hash for the internal use. And, we don't have to pass in the new CLI args create_rust_file and rust_file then it won't try to create a generate_transaction.rs in the processor repo

rtso · 2025-01-09T00:58:19Z

ecosystem/indexer-grpc/indexer-transaction-generator/src/config.rs

+    pub rust_file: Option<PathBuf>,
+
+    #[clap(short, long)]
+    pub create_rust_file: bool,


When is this flag going to be false? I think we'd always want to update the Rust file if there are new or changed transactions.

rtso · 2025-01-09T00:58:58Z

ecosystem/indexer-grpc/indexer-transaction-generator/src/config.rs

+
+    /// Path to the rust file to generate the transactions code.
+    #[clap(long)]
+    pub rust_file: Option<PathBuf>,


Could we reuse output_folder as the path to store the Rust file?

yuunlimm · 2025-01-09T01:07:24Z

The overall approach makes sense: we decouple the json and the code. Only one question regarding to the incompatibility: If jsons are on an older version(say we have 5 jsons) and the code is on a newer version(say we import 6 txns), is it possible to notify/show the users to upgrade to resolve?

it looks like current behavior is compilation error..

I don’t think compatibility is an issue in this scenario, because we’re assuming each user relies on their own dedicated set of test transactions.

but maybe we can discuss whether we should provide a fixed set of test transactions for them and store the jsons in the processor repo with the tests @rtso I agree that it makes sense to have the json files and generated rust file in the same place for debuggability, etc.

graphite-app · 2025-01-09T01:09:52Z

ecosystem/indexer-grpc/indexer-transaction-generator/src/config.rs

+    #[clap(long)]
+    pub rust_file: Option<PathBuf>,
+
+    #[clap(short, long)]
+    pub create_rust_file: bool,


Add default values to the new fields to maintain backward compatibility with existing tests: #[clap(long, default_value = "false")] for create_rust_file and keep rust_file as Option

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-09T01:09:52Z

ecosystem/indexer-grpc/indexer-test-transactions/Cargo.toml

+[build-dependencies]
+aptos-indexer-transaction-generator = { workspace = true }
 aptos-protos ={ workspace = true }


Dependencies aptos-protos and serde_json need to be moved back to [dependencies] section while keeping them in [build-dependencies] since they are needed both at build time and runtime. The current configuration causes compilation errors because these dependencies are not available at runtime.

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-10T04:33:28Z

.../indexer-grpc/indexer-transaction-generator/imported_transactions/imported_transactions.yaml

@@ -2,7 +2,7 @@ testnet:
  # Transaction Stream endpoint addresss.
  transaction_stream_endpoint: https://grpc.testnet.aptoslabs.com:443
  # (Optional) The key to use with developers.aptoslabs.com
-  api_key: TESTNET_API_KEY
+  api_key: aptoslabs_Yq2BZM4pCY9_GU1vCq1XwB2NdU2Z8AENXnvKdwSiqF3YQ


Please remove the API key from source control to prevent security risks. Consider replacing it with a placeholder like TESTNET_API_KEY and loading the actual value from environment variables at runtime. This follows security best practices for handling sensitive credentials.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

yuunlimm mentioned this pull request Jan 8, 2025

allow specifying account address at config level instead of random assignment #15465

Open

20 tasks

graphite-app bot reviewed Jan 8, 2025

View reviewed changes

yuunlimm force-pushed the 01-08-support_test_file_creation_as_a_part_of_test_txn_generation branch from d434b0a to e60d566 Compare January 8, 2025 21:04

yuunlimm marked this pull request as ready for review January 8, 2025 21:04

yuunlimm requested a review from a team January 8, 2025 21:05

graphite-app bot reviewed Jan 8, 2025

View reviewed changes

rtso reviewed Jan 9, 2025

View reviewed changes

graphite-app bot reviewed Jan 9, 2025

View reviewed changes

yuunlimm force-pushed the 12-03-allow_specifying_account_address_at_config_level_instead_of_random_assignment branch from 9f51332 to 5cad83d Compare January 9, 2025 19:29

yuunlimm force-pushed the 01-08-support_test_file_creation_as_a_part_of_test_txn_generation branch from e60d566 to cf9f9c7 Compare January 9, 2025 19:29

yuunlimm force-pushed the 12-03-allow_specifying_account_address_at_config_level_instead_of_random_assignment branch from 5cad83d to 4ff0a0c Compare January 9, 2025 21:52

yuunlimm force-pushed the 01-08-support_test_file_creation_as_a_part_of_test_txn_generation branch from cf9f9c7 to 3822921 Compare January 9, 2025 21:52

support test file creation as a part of test txn generation

3b82607

yuunlimm force-pushed the 01-08-support_test_file_creation_as_a_part_of_test_txn_generation branch from 3822921 to 3b82607 Compare January 10, 2025 04:32

graphite-app bot reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support test file creation as a part of test txn generation #15688

support test file creation as a part of test txn generation #15688

yuunlimm commented Jan 8, 2025 •

edited

Loading

trunk-io bot commented Jan 8, 2025 •

edited

Loading

yuunlimm commented Jan 8, 2025 •

edited

Loading

graphite-app bot Jan 8, 2025

graphite-app bot Jan 8, 2025

rtso commented Jan 9, 2025

larry-aptos commented Jan 9, 2025

rtso Jan 9, 2025

yuunlimm Jan 9, 2025

rtso Jan 9, 2025

rtso Jan 9, 2025

yuunlimm commented Jan 9, 2025

graphite-app bot Jan 9, 2025

graphite-app bot Jan 9, 2025

graphite-app bot Jan 10, 2025

support test file creation as a part of test txn generation #15688

Are you sure you want to change the base?

support test file creation as a part of test txn generation #15688

Conversation

yuunlimm commented Jan 8, 2025 • 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 Jan 8, 2025 • edited Loading

yuunlimm commented Jan 8, 2025 • edited Loading

graphite-app bot Jan 8, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 8, 2025

Choose a reason for hiding this comment

rtso commented Jan 9, 2025

larry-aptos commented Jan 9, 2025

rtso Jan 9, 2025

Choose a reason for hiding this comment

yuunlimm Jan 9, 2025

Choose a reason for hiding this comment

rtso Jan 9, 2025

Choose a reason for hiding this comment

rtso Jan 9, 2025

Choose a reason for hiding this comment

yuunlimm commented Jan 9, 2025

graphite-app bot Jan 9, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 9, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 10, 2025

Choose a reason for hiding this comment

yuunlimm commented Jan 8, 2025 •

edited

Loading

trunk-io bot commented Jan 8, 2025 •

edited

Loading

yuunlimm commented Jan 8, 2025 •

edited

Loading