review-changes

name	review-changes
description	Review and fix local git changes against Gem Wallet Core coding standards and patterns
argument-hint	[--subagent]
disable-model-invocation	true
allowed-tools	Read, Edit, Write, Glob, Grep, Bash, Task

Review and Fix Local Changes

Review uncommitted changes against the coding standards and patterns defined in this repository, then fix any issues found.

Arguments

• --subagent: Run in a subagent (isolates context, runs in background)

Mode Selection

Check if --subagent is in the arguments:

• If --subagent is present: Use the Task tool with subagent_type: "general-purpose" to run this review in a subagent, passing all other arguments
• If --subagent is NOT present: Run directly in current context (default)

Context

Current git diff to review: !git diff --no-color

Changed files: !git diff --name-only

Review Checklist

Analyze the diff above and check for the following issues:

1. Import Patterns

• No inline imports: All imports must be at the top of the file, never inside functions
• No full paths inline: Never use storage::DatabaseClient::new() inline; import types first
• Import order: Standard library first, then external crates, then local crates, then pub use re-exports

2. Naming Conventions

• Files/modules: snake_case (e.g., asset_id.rs)
• Functions/variables: snake_case
• Structs/enums: PascalCase
• Constants: SCREAMING_SNAKE_CASE
• No generic names: Avoid util, utils, normalize, or similar vague names
• Concise helper names: Within a module, use scope-reliant names (prefer is_spot_swap over is_hypercore_spot_swap)
• No type suffixes: Avoid _str, _int, _vec suffixes; Rust's type system makes them redundant

3. Error Handling

• Prefer plain Error: Use plain Error types, not thiserror macros
• Implement From traits: For error conversion between types
• Propagate with ?: Prefer ? operator over manual map_err where possible
• Consistent Result<T, Error>: Use consistent return types
• Constructor methods on errors: Use ErrorType::constructor(msg) instead of verbose ErrorType::Variant("redundant context".into())

3b. JSON Parameter Extraction

• Use primitives::ValueAccess: For serde_json::Value access, use composable trait methods (get_value(key), at(index), string()) instead of manual .get().ok_or() chains. Chain for compound access: params.get_value("key")?.at(0)?.string()?
• Accessor methods on parent types: Add accessor methods (e.g., TransactionLoadInput::get_data_extra()) to avoid pattern-matching boilerplate at call sites

4. Code Style

• Line length: Maximum 180 characters
• Avoid matches!: Don't use matches! for pattern matching; it's easy to miss cases later
• No over-engineering: Only make changes directly requested or clearly necessary
• No docstrings/comments/annotations: Don't add docstrings, comments, or /// docs unless explicitly asked; remove any that were added (including in mod.rs files)
• No #[allow(dead_code)]: Remove dead code instead of suppressing warnings; if code is needed, use it
• No unused fields: Remove unused fields from structs/models; don't keep fields "for future use"
• Constants for magic numbers: Extract magic numbers into named constants with clear meaning
• Minimum interface: Don't expose unnecessary functions; if client only needs one function, don't add multiple variants
• Use uniffi::remote: For UniFFI wrapper types around external models, use #[uniffi::remote] instead of creating duplicate structs with From implementations:

  // Record example
  use primitives::AuthNonce;
  pub type GemAuthNonce = AuthNonce;
  #[uniffi::remote(Record)]
  pub struct GemAuthNonce { pub nonce: String, pub timestamp: u32 }
  
  // Enum example
  use primitives::SwapperMode;
  pub type GemSwapperMode = SwapperMode;
  #[uniffi::remote(Enum)]
  pub enum GemSwapperMode { ExactIn, ExactOut }
  

• Simple solutions: Three similar lines is better than a premature abstraction
• Avoid mut: Prefer immutable bindings; use mut only when truly necessary
• Prefer one-liners: Inline single-use variables; avoid creating variables used only once
• Avoid #[serde(default)]: Only use when the field is genuinely optional in the API response; if the field is always present, omit it
• Use accessor methods for enum variants: Instead of destructuring enum variants with match, use typed accessor methods (e.g., metadata.get_sequence()? instead of match &metadata { Cosmos { sequence, .. } => ... })

5. Code Organization

• Modular structure: Break down files into smaller, focused modules; separate models from clients/logic (e.g., models.rs + client.rs, not everything in one file)
• Folder modules for complexity: When a module has multiple concerns (models, client, mappers), use a folder with mod.rs instead of a single file
• Avoid duplication: Search for existing implementations before writing new code; reuse existing code or crates
• Shared crates: Reusable logic belongs in shared crates (e.g., gem_solana, gem_evm), not in utility binaries; move shared code to appropriate crates
• Bird's eye view: Step back and identify opportunities to simplify and consolidate

6. Async Patterns

• Tokio runtime: Use tokio for async operations
• Shared state: Use Arc<tokio::sync::Mutex<T>> for shared async state
• Async client structs: Should return Result<T, Error>

7. Database Patterns

• Separate models: Database models should be separate from domain primitives
• Use as_primitive(): For conversion from database models
• Repository pattern: Access via DatabaseClient methods

8. Blockchain/RPC Patterns

• Use gem_jsonrpc::JsonRpcClient: For blockchain RPC interactions
• Use primitives::hex: For hex encoding/decoding (not alloy_primitives::hex)
• U256 conversions: Use u256_to_biguint and biguint_to_u256 from gem_evm/src/u256.rs
• Provider pattern: Fetch raw data via RPC, then use mapper functions for conversion
• Mapper files: Place mapper functions in separate *_mapper.rs files

9. Testing

• #[tokio::test]: Use for async tests
• Test naming: Prefix with test_ descriptively
• Error handling: Use Result<(), Box<dyn std::error::Error + Send + Sync>>
• Test data: For long JSON (>20 lines), store in testdata/ and use include_str!()
• .unwrap() not .expect(): Never use .expect() in tests; use .unwrap() for brevity
• No assert! with contains: Use assert_eq! with concrete values; assert!(x.contains(...)) gives useless failure messages
• No fallback, fail fast: Don't silently return defaults on errors (e.g., unwrap_or(0)). Propagate errors with ? or return Result. Fail rather than mask issues with fallbacks.
• Methods over free functions: Helper functions should be methods on the relevant struct, not top-level free functions
• Mock methods in testkit: Use Type::mock() constructors in testkit/ modules instead of inline struct construction in tests
• PartialEq + assert_eq!: Derive PartialEq on test-relevant enums and use direct assert_eq! with constructed expected values instead of destructuring with let ... else { panic! } or match ... { _ => panic! }
• Test helpers: Create concise constructor functions (e.g., fn object(json: &str) -> EnumType, fn sign_message(chain, sign_type, data) -> Action) for frequently constructed enum variants in test modules

10. Security

• No hardcoded secrets: Check for API keys, passwords, credentials
• Input validation: Validate at system boundaries (user input, external APIs)
• OWASP top 10: Watch for command injection, XSS, SQL injection vulnerabilities

Workflow

Iterate at least 2-3 times to ensure all issues are caught and fixed:

Each Iteration:

1. Analyze: Review the diff against the checklist
2. Read: Read the full content of each changed file to understand context
3. Fix: Apply fixes directly using the Edit tool for each issue found
4. Format: Run rustfmt --edition 2024 <files> on modified files
5. Verify: Run cargo clippy -p <crate> -- -D warnings on affected crates
6. Check: Review the changes again - new issues may have been introduced or revealed

Stop when:

• No more issues are found after a full review pass
• Clippy passes with no warnings
• Code is properly formatted

Output Format

After fixing issues, provide a summary:

1. Issues Fixed: List each fix made with:
   • File and line reference
   • Category (from checklist above)
   • What was changed
2. Manual Review Needed: Issues that require human decision (if any)
3. Verification: Clippy/format results

Severity levels for reporting:

• CRITICAL: Security issues or bugs - fix immediately
• WARNING: Coding standard violations - fix automatically
• SUGGESTION: Minor improvements - fix if straightforward, otherwise note for user