From 26452fd0eba18e18ab6de6ba87a4d0b23db71e1c Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Thu, 7 Aug 2025 12:31:15 +1200 Subject: [PATCH] ai review result --- docs/review-results.md | 81 +++++++++++++++++++ .../sum-balance-percent/src/main.rs | 4 + guest-examples/sum-balance/src/main.rs | 1 + guest-examples/swap-info/src/main.rs | 1 + guest-examples/total-supply/src/main.rs | 1 + poc/runtime/src/lib.rs | 5 ++ poc/runtime/src/pvq.rs | 1 + pvq-executor/src/executor.rs | 6 ++ pvq-executor/src/lib.rs | 4 + pvq-extension-core/src/lib.rs | 7 ++ pvq-extension-fungibles/src/lib.rs | 4 + pvq-extension-swap/src/lib.rs | 4 + pvq-extension/src/lib.rs | 3 + pvq-primitives/src/lib.rs | 5 ++ .../src/bin/pvq-program-metadata-gen.rs | 6 ++ pvq-program-metadata-gen/src/lib.rs | 2 + pvq-program/procedural/src/lib.rs | 2 + .../procedural/src/program/expand/mod.rs | 4 + pvq-program/src/lib.rs | 4 + pvq-runtime-api/src/lib.rs | 8 ++ pvq-test-runner/src/bin/pvq-test-runner.rs | 2 + pvq-test-runner/src/lib.rs | 5 ++ 22 files changed, 160 insertions(+) create mode 100644 docs/review-results.md diff --git a/docs/review-results.md b/docs/review-results.md new file mode 100644 index 0000000..e14e8f5 --- /dev/null +++ b/docs/review-results.md @@ -0,0 +1,81 @@ +# Code Review Results + +This document contains the results of the code review. + +## General Findings + +* [Add general findings here] + +### `pvq-executor` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. + +### `pvq-extension-swap` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. + +### `pvq-extension` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. + +### `pvq-primitives` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. +- **Undocumented Enum**: The `PvqError` enum variants should be documented. + +### `pvq-program-metadata-gen` + +- **No Tests**: The crate is missing tests. +- **Use of `expect()`**: The binary uses `expect()` which can cause panics. It should be replaced with proper error handling. +- **Long `main` function**: The `main` function in `src/bin/pvq-program-metadata-gen.rs` should be broken down into smaller, more manageable functions. + +### `pvq-program` + +- **No README.md**: The crate and its procedural macro sub-crate should have `README.md` files. +- **No Tests**: The crate and its procedural macro sub-crate are missing tests. UI tests for the procedural macro are particularly important. + +### `pvq-runtime-api` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. +- **Inefficient Data Types**: The API uses `Vec` for parameters and return types where slices (`&[u8]`) could be more efficient. + +### `pvq-test-runner` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The test runner itself is not tested. +- **Use of `expect()`/`unwrap()`**: The crate uses `expect()` and `unwrap()` extensively, which can lead to panics. +- **Long `main` function**: The `main` function in the binary is too long and should be refactored. +- **Hardcoded test data**: The test data and expected results are hardcoded within the runner. It would be more flexible to load them from external files (e.g., JSON or YAML). + +### `poc/runtime` + +- **No README.md**: The runtime crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The runtime and its extension implementations are not tested. +- **Hardcoded Gas Limit**: The default gas limit in the `execute_query` runtime API is hardcoded. It should be a constant. + +### `guest-examples` + +- **No README.md**: The `guest-examples` directory should have a `README.md` explaining each example. +- **Magic Numbers**: The examples use hardcoded `extension_id`s. These should be replaced with named constants. +- **Lack of Comments**: The examples could benefit from more comments explaining the code. +- **Misleading Calculation**: The `sum_balance_percent` example has a potentially misleading percentage calculation that should be clarified. Unit tests should be added for the executor logic and error handling. +- **`unwrap()` in `PvqExecutor::new`**: The `unwrap()` call should be replaced with proper error handling. +- **Long `execute` function**: The `execute` function in `executor.rs` should be broken down into smaller, more manageable functions. +- **Tuple as return type**: The return type of the `execute` function should be a struct for better readability. +- **Lack of comments**: More comments could be added to `executor.rs` to explain the execution flow. + +### `pvq-extension-core` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. +- **Commented-out functions**: The `lib.rs` file contains commented-out functions that should be implemented or removed. + +### `pvq-extension-fungibles` + +- **No README.md**: The crate should have a `README.md` explaining its purpose and how to use it. +- **No Tests**: The crate is missing tests. diff --git a/guest-examples/sum-balance-percent/src/main.rs b/guest-examples/sum-balance-percent/src/main.rs index 3468a24..478bff3 100644 --- a/guest-examples/sum-balance-percent/src/main.rs +++ b/guest-examples/sum-balance-percent/src/main.rs @@ -1,6 +1,7 @@ #![no_std] #![no_main] +// REVIEW: The `extension_id` is a magic number. It would be better to define it as a constant with a descriptive name. #[pvq_program::program] mod sum_balance_percent { type AssetId = u32; @@ -13,6 +14,9 @@ mod sum_balance_percent { #[program::entrypoint] fn sum_balance(asset: AssetId, accounts: alloc::vec::Vec) -> Balance { + // REVIEW: The entrypoint should be renamed to `sum_balance_percent` to reflect that it returns a percentage. + // Also, the percentage calculation `sum_balance * 100 / total_supply(asset)` can be misleading. + // It should be clarified that this is an integer division and might not be precise. let mut sum_balance = 0; for account in accounts { sum_balance += balance(asset, account); diff --git a/guest-examples/sum-balance/src/main.rs b/guest-examples/sum-balance/src/main.rs index 764cf32..dde0676 100644 --- a/guest-examples/sum-balance/src/main.rs +++ b/guest-examples/sum-balance/src/main.rs @@ -1,6 +1,7 @@ #![no_std] #![no_main] +// REVIEW: The `extension_id` is a magic number. It would be better to define it as a constant with a descriptive name. #[pvq_program::program] mod sum_balance { diff --git a/guest-examples/swap-info/src/main.rs b/guest-examples/swap-info/src/main.rs index 10538d9..bae74c9 100644 --- a/guest-examples/swap-info/src/main.rs +++ b/guest-examples/swap-info/src/main.rs @@ -1,6 +1,7 @@ #![no_std] #![no_main] +// REVIEW: The `extension_id` used in the `extension_fn` attributes is a magic number. It would be better to define it as a constant with a descriptive name. #[pvq_program::program] mod swap_info { diff --git a/guest-examples/total-supply/src/main.rs b/guest-examples/total-supply/src/main.rs index cdda439..2cc1d41 100644 --- a/guest-examples/total-supply/src/main.rs +++ b/guest-examples/total-supply/src/main.rs @@ -1,6 +1,7 @@ #![no_std] #![no_main] +// REVIEW: The `extension_id` is a magic number. It would be better to define it as a constant with a descriptive name. #[pvq_program::program] mod query_total_supply { #[program::extension_fn(extension_id = 4071833530116166512u64, fn_index = 0)] diff --git a/poc/runtime/src/lib.rs b/poc/runtime/src/lib.rs index b3a16bf..c34390e 100644 --- a/poc/runtime/src/lib.rs +++ b/poc/runtime/src/lib.rs @@ -1,3 +1,5 @@ +// REVIEW: The runtime lacks tests. While it's a proof-of-concept, adding some basic tests for the custom logic (like the PVQ execution) would be beneficial. +// REVIEW: A `README.md` file in the `poc/runtime` directory would be helpful to explain its purpose, how to build it, and how to run it with a node. #![cfg_attr(not(feature = "std"), no_std)] // Frame macros reference features which this crate does not have #![allow(unexpected_cfgs)] @@ -230,6 +232,9 @@ impl_runtime_apis! { } impl pvq_runtime_api::PvqApi for Runtime { + // REVIEW: The default gas limit is hardcoded here. It would be better to define this as a + // constant, e.g., `const DEFAULT_GAS_LIMIT: i64 = ONE_SECOND_IN_GAS * 2;` and then use + // that constant here. This will improve readability and maintainability. fn execute_query(program: Vec, args: Vec, gas_limit: Option) -> pvq_primitives::PvqResult { // Set a default gas limit of 2 seconds pvq::execute_query(&program, &args, gas_limit.unwrap_or(ONE_SECOND_IN_GAS * 2)) diff --git a/poc/runtime/src/pvq.rs b/poc/runtime/src/pvq.rs index ccde806..3047891 100644 --- a/poc/runtime/src/pvq.rs +++ b/poc/runtime/src/pvq.rs @@ -1,6 +1,7 @@ #[allow(unused_imports)] use frame::deps::scale_info::prelude::{format, string::String}; +// REVIEW: The extension implementations in this file are not tested. Unit tests should be added to ensure they behave as expected. use pvq_extension::metadata::Metadata; use pvq_extension::{extensions_impl, ExtensionsExecutor, InvokeSource}; diff --git a/pvq-executor/src/executor.rs b/pvq-executor/src/executor.rs index d2f1862..47fc27a 100644 --- a/pvq-executor/src/executor.rs +++ b/pvq-executor/src/executor.rs @@ -14,6 +14,8 @@ pub struct PvqExecutor { } impl PvqExecutor { + // REVIEW: The use of `.unwrap()` when creating a new `Engine` can cause a panic. + // This should be replaced with proper error handling, for instance, by returning a `Result`. pub fn new(config: Config, mut context: Ctx) -> Self { let engine = Engine::new(&config).unwrap(); let mut linker = Linker::::new(); @@ -26,6 +28,10 @@ impl PvqExecutor { } } + // REVIEW: The `execute` function is overly long and complex. It should be broken down into smaller, + // more focused functions to improve readability and maintainability. + // REVIEW: The `execute` function returns a tuple `(PvqExecutorResult, GasLimit)`. + // This should be encapsulated in a dedicated struct for better clarity. pub fn execute( &mut self, program: &[u8], diff --git a/pvq-executor/src/lib.rs b/pvq-executor/src/lib.rs index 8092ad9..d9acbed 100644 --- a/pvq-executor/src/lib.rs +++ b/pvq-executor/src/lib.rs @@ -1,3 +1,7 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. #![cfg_attr(not(feature = "std"), no_std)] extern crate alloc; diff --git a/pvq-extension-core/src/lib.rs b/pvq-extension-core/src/lib.rs index 4a5c039..1e89bd0 100644 --- a/pvq-extension-core/src/lib.rs +++ b/pvq-extension-core/src/lib.rs @@ -1,3 +1,7 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. #![cfg_attr(not(feature = "std"), no_std)] use pvq_extension::extension_decl; @@ -8,6 +12,9 @@ pub mod extension { pub trait ExtensionCore { type ExtensionId; fn has_extension(id: Self::ExtensionId) -> bool; + + // REVIEW: The following crypto and storage functions are commented out. + // They should be implemented or removed. // crypto functions // fn blake2_64(data: Vec) -> [u8; 8]; // fn blake2_128(data: Vec) -> [u8; 16]; diff --git a/pvq-extension-fungibles/src/lib.rs b/pvq-extension-fungibles/src/lib.rs index 2936bd9..4184392 100644 --- a/pvq-extension-fungibles/src/lib.rs +++ b/pvq-extension-fungibles/src/lib.rs @@ -1,3 +1,7 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. #![cfg_attr(not(feature = "std"), no_std)] use pvq_extension::extension_decl; diff --git a/pvq-extension-swap/src/lib.rs b/pvq-extension-swap/src/lib.rs index 1b46977..944a7d1 100644 --- a/pvq-extension-swap/src/lib.rs +++ b/pvq-extension-swap/src/lib.rs @@ -1,3 +1,7 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. #![cfg_attr(not(feature = "std"), no_std)] extern crate alloc; diff --git a/pvq-extension/src/lib.rs b/pvq-extension/src/lib.rs index 0f12aa3..0a0fb07 100644 --- a/pvq-extension/src/lib.rs +++ b/pvq-extension/src/lib.rs @@ -1,3 +1,6 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: While there are documentation comments, a `README.md` file would be beneficial for new contributors to understand the crate's purpose and usage. #![cfg_attr(not(feature = "std"), no_std)] //! # PVQ Extension System //! diff --git a/pvq-primitives/src/lib.rs b/pvq-primitives/src/lib.rs index 7f4f93d..4251add 100644 --- a/pvq-primitives/src/lib.rs +++ b/pvq-primitives/src/lib.rs @@ -1,3 +1,8 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. +// REVIEW: The variants of the `PvqError` enum should be documented to explain their meaning. #![cfg_attr(not(feature = "std"), no_std)] extern crate alloc; diff --git a/pvq-program-metadata-gen/src/bin/pvq-program-metadata-gen.rs b/pvq-program-metadata-gen/src/bin/pvq-program-metadata-gen.rs index e8f12a6..cc0d12c 100644 --- a/pvq-program-metadata-gen/src/bin/pvq-program-metadata-gen.rs +++ b/pvq-program-metadata-gen/src/bin/pvq-program-metadata-gen.rs @@ -15,6 +15,8 @@ struct Args { output_dir: PathBuf, } +// REVIEW: The `main` function is overly long and complex. It should be broken down into smaller, +// more focused functions to improve readability and maintainability. fn main() { // Initialize tracing tracing_subscriber::fmt::init(); @@ -79,3 +81,7 @@ fn main() { } info!("Metadata generation successful!"); } + +// REVIEW: The `main` function uses `expect()` in multiple places, which can cause the +// program to panic. Consider returning a `Result` from `main` and using the `?` operator to +// propagate errors. This will make the error handling more robust. diff --git a/pvq-program-metadata-gen/src/lib.rs b/pvq-program-metadata-gen/src/lib.rs index 82961fe..75eee39 100644 --- a/pvq-program-metadata-gen/src/lib.rs +++ b/pvq-program-metadata-gen/src/lib.rs @@ -1,3 +1,5 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. pub type ExtensionId = u64; pub type FnIndex = u8; mod features; diff --git a/pvq-program/procedural/src/lib.rs b/pvq-program/procedural/src/lib.rs index 37babc0..0bee93f 100644 --- a/pvq-program/procedural/src/lib.rs +++ b/pvq-program/procedural/src/lib.rs @@ -21,6 +21,8 @@ /// } /// ``` /// +// REVIEW: The procedural macro lacks tests. UI tests should be added to ensure the macro generates correct code and provides good error messages for invalid input. +// REVIEW: A `README.md` file in the `procedural` crate's directory would be beneficial to explain its usage and how it works. mod program; use proc_macro::TokenStream; diff --git a/pvq-program/procedural/src/program/expand/mod.rs b/pvq-program/procedural/src/program/expand/mod.rs index e685fab..6713e49 100644 --- a/pvq-program/procedural/src/program/expand/mod.rs +++ b/pvq-program/procedural/src/program/expand/mod.rs @@ -28,6 +28,10 @@ pub fn expand(mut def: Def) -> TokenStream2 { def.item.into_token_stream() } +// REVIEW: The `expand_extension_fn` and `expand_main` functions use `expect()` and +// `unreachable!()` in several places. While some of these might be safe due to checks in the +// parsing stage, it's generally better to return a `compile_error!` with a descriptive +// message. This will provide better error messages to the user of the macro. fn expand_extension_fn(extension_fn: &mut ExtensionFn, parity_scale_codec: &syn::Path) -> TokenStream2 { let extension_id = extension_fn.extension_id; let fn_index = extension_fn.fn_index; diff --git a/pvq-program/src/lib.rs b/pvq-program/src/lib.rs index dd2d335..742d974 100644 --- a/pvq-program/src/lib.rs +++ b/pvq-program/src/lib.rs @@ -1,2 +1,6 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. #![cfg_attr(not(feature = "std"), no_std)] pub use pvq_program_procedural::program; diff --git a/pvq-runtime-api/src/lib.rs b/pvq-runtime-api/src/lib.rs index af4110d..25973cb 100644 --- a/pvq-runtime-api/src/lib.rs +++ b/pvq-runtime-api/src/lib.rs @@ -1,3 +1,7 @@ +// REVIEW: The crate lacks any form of testing, which is critical for ensuring its correctness and stability. +// Unit tests should be added. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. +// A `README.md` file should be created. #![cfg_attr(not(feature = "std"), no_std)] extern crate alloc; @@ -12,7 +16,11 @@ use sp_api::decl_runtime_apis; // - `gas_limit`: Optional gas limit for query execution. When set to `None`, execution is constrained by the default time boundary. decl_runtime_apis! { pub trait PvqApi { + // REVIEW: The `program` and `args` parameters are passed as `Vec`, which implies ownership. + // Consider using `&[u8]` to avoid unnecessary allocations when passing data to the runtime API. fn execute_query(program: Vec, args: Vec, gas_limit: Option) -> PvqResult; + // REVIEW: The `metadata` function returns a `Vec`. If the metadata is static, + // consider returning a `&'static [u8]` to avoid allocations. fn metadata() -> Vec; } } diff --git a/pvq-test-runner/src/bin/pvq-test-runner.rs b/pvq-test-runner/src/bin/pvq-test-runner.rs index 189b6ec..2ca1995 100644 --- a/pvq-test-runner/src/bin/pvq-test-runner.rs +++ b/pvq-test-runner/src/bin/pvq-test-runner.rs @@ -39,6 +39,8 @@ impl std::fmt::Display for Chain { } } +// REVIEW: The `main` function is doing too much. It should be broken down into smaller functions for better readability and maintainability (e.g., a function for initializing tracing, a function for parsing args, a function for running the test). +// REVIEW: The `main` function uses `expect()` and `unwrap()` which can cause the program to panic. It should be updated to return a `Result` and propagate errors gracefully. fn main() { let registry = tracing_subscriber::registry(); diff --git a/pvq-test-runner/src/lib.rs b/pvq-test-runner/src/lib.rs index d9500ff..459afec 100644 --- a/pvq-test-runner/src/lib.rs +++ b/pvq-test-runner/src/lib.rs @@ -1,3 +1,5 @@ +// REVIEW: The crate lacks any form of testing for its own logic. While it is a test runner, adding unit tests for functions like `prepare_input_data` and `expected_result` would improve its reliability. +// REVIEW: There is no `README.md` file in the crate, making it difficult for new contributors to understand its purpose and usage. A `README.md` file should be created. use parity_scale_codec::Encode; use pvq_extension::{extensions_impl, ExtensionsExecutor, InvokeSource}; use sp_core::crypto::{AccountId32, Ss58Codec}; @@ -98,6 +100,9 @@ impl TestRunner { } } + // REVIEW: The `prepare_input_data` and `get_liquidity_pool` functions use `expect()` which + // can cause the program to panic. Consider returning a `Result` and using the `?` + // operator to propagate errors. This will make the error handling more robust. pub fn prepare_input_data(program_path: &str, chain: &str) -> Vec { let mut input_data = Vec::new();