ai review result

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<u8>` 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.

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<AccountId>) -> 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);

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 {
 

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 {
 

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)]

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<Block> 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<u8>, args: Vec<u8>, gas_limit: Option<i64>) -> 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))

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};
 

pvq-executor/src/executor.rs

@@ -14,6 +14,8 @@ pub struct PvqExecutor<Ctx: PvqExecutorContext> {
 }
 
 impl<Ctx: PvqExecutorContext> PvqExecutor<Ctx> {
+    // 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::<Ctx::UserData, Ctx::UserError>::new();
@@ -26,6 +28,10 @@ impl<Ctx: PvqExecutorContext> PvqExecutor<Ctx> {
         }
     }
 
+    // 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<Ctx::UserError>, GasLimit)`.
+    // This should be encapsulated in a dedicated struct for better clarity.
     pub fn execute(
         &mut self,
         program: &[u8],

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;

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>) -> [u8; 8];
         // fn blake2_128(data: Vec<u8>) -> [u8; 16];

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;
 

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;
 

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
 //!

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;

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.

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;

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;
 

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;

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;

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<u8>`, which implies ownership.
+        // Consider using `&[u8]` to avoid unnecessary allocations when passing data to the runtime API.
         fn execute_query(program: Vec<u8>, args: Vec<u8>, gas_limit: Option<i64>) -> PvqResult;
+        // REVIEW: The `metadata` function returns a `Vec<u8>`. If the metadata is static,
+        // consider returning a `&'static [u8]` to avoid allocations.
         fn metadata() -> Vec<u8>;
     }
 }

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();
 

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<u8> {
         let mut input_data = Vec::new();