|
| 1 | +--- |
| 2 | +name: review-changes |
| 3 | +description: Review and fix local git changes against Gem Wallet Core coding standards and patterns |
| 4 | +argument-hint: "[--subagent]" |
| 5 | +disable-model-invocation: true |
| 6 | +allowed-tools: Read, Edit, Write, Glob, Grep, Bash, Task |
| 7 | +--- |
| 8 | + |
| 9 | +# Review and Fix Local Changes |
| 10 | + |
| 11 | +Review uncommitted changes against the coding standards and patterns defined in this repository, then fix any issues found. |
| 12 | + |
| 13 | +## Arguments |
| 14 | +- `--subagent`: Run in a subagent (isolates context, runs in background) |
| 15 | + |
| 16 | +## Mode Selection |
| 17 | + |
| 18 | +Check if `--subagent` is in the arguments: |
| 19 | +- If `--subagent` is present: Use the Task tool with `subagent_type: "general-purpose"` to run this review in a subagent, passing all other arguments |
| 20 | +- If `--subagent` is NOT present: Run directly in current context (default) |
| 21 | + |
| 22 | +## Context |
| 23 | + |
| 24 | +Current git diff to review: |
| 25 | +!`git diff --no-color` |
| 26 | + |
| 27 | +Changed files: |
| 28 | +!`git diff --name-only` |
| 29 | + |
| 30 | +## Review Checklist |
| 31 | + |
| 32 | +Analyze the diff above and check for the following issues: |
| 33 | + |
| 34 | +### 1. Import Patterns |
| 35 | +- [ ] **No inline imports**: All imports must be at the top of the file, never inside functions |
| 36 | +- [ ] **No full paths inline**: Never use `storage::DatabaseClient::new()` inline; import types first |
| 37 | +- [ ] **Import order**: Standard library first, then external crates, then local crates, then `pub use` re-exports |
| 38 | + |
| 39 | +### 2. Naming Conventions |
| 40 | +- [ ] **Files/modules**: `snake_case` (e.g., `asset_id.rs`) |
| 41 | +- [ ] **Functions/variables**: `snake_case` |
| 42 | +- [ ] **Structs/enums**: `PascalCase` |
| 43 | +- [ ] **Constants**: `SCREAMING_SNAKE_CASE` |
| 44 | +- [ ] **No generic names**: Avoid `util`, `utils`, `normalize`, or similar vague names |
| 45 | +- [ ] **Concise helper names**: Within a module, use scope-reliant names (prefer `is_spot_swap` over `is_hypercore_spot_swap`) |
| 46 | +- [ ] **No type suffixes**: Avoid `_str`, `_int`, `_vec` suffixes; Rust's type system makes them redundant |
| 47 | + |
| 48 | +### 3. Error Handling |
| 49 | +- [ ] **Prefer plain `Error`**: Use plain Error types, not `thiserror` macros |
| 50 | +- [ ] **Implement `From` traits**: For error conversion between types |
| 51 | +- [ ] **Propagate with `?`**: Prefer `?` operator over manual `map_err` where possible |
| 52 | +- [ ] **Consistent `Result<T, Error>`**: Use consistent return types |
| 53 | +- [ ] **Constructor methods on errors**: Use `ErrorType::constructor(msg)` instead of verbose `ErrorType::Variant("redundant context".into())` |
| 54 | + |
| 55 | +### 3b. JSON Parameter Extraction |
| 56 | +- [ ] **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()?` |
| 57 | +- [ ] **Accessor methods on parent types**: Add accessor methods (e.g., `TransactionLoadInput::get_data_extra()`) to avoid pattern-matching boilerplate at call sites |
| 58 | + |
| 59 | +### 4. Code Style |
| 60 | +- [ ] **Line length**: Maximum 180 characters |
| 61 | +- [ ] **Avoid `matches!`**: Don't use `matches!` for pattern matching; it's easy to miss cases later |
| 62 | +- [ ] **No over-engineering**: Only make changes directly requested or clearly necessary |
| 63 | +- [ ] **No docstrings/comments/annotations**: Don't add docstrings, comments, or `///` docs unless explicitly asked; remove any that were added (including in mod.rs files) |
| 64 | +- [ ] **No `#[allow(dead_code)]`**: Remove dead code instead of suppressing warnings; if code is needed, use it |
| 65 | +- [ ] **No unused fields**: Remove unused fields from structs/models; don't keep fields "for future use" |
| 66 | +- [ ] **Constants for magic numbers**: Extract magic numbers into named constants with clear meaning |
| 67 | +- [ ] **Minimum interface**: Don't expose unnecessary functions; if client only needs one function, don't add multiple variants |
| 68 | +- [ ] **Use uniffi::remote**: For UniFFI wrapper types around external models, use `#[uniffi::remote]` instead of creating duplicate structs with `From` implementations: |
| 69 | + ```rust |
| 70 | + // Record example |
| 71 | + use primitives::AuthNonce; |
| 72 | + pub type GemAuthNonce = AuthNonce; |
| 73 | + #[uniffi::remote(Record)] |
| 74 | + pub struct GemAuthNonce { pub nonce: String, pub timestamp: u32 } |
| 75 | + |
| 76 | + // Enum example |
| 77 | + use primitives::SwapperMode; |
| 78 | + pub type GemSwapperMode = SwapperMode; |
| 79 | + #[uniffi::remote(Enum)] |
| 80 | + pub enum GemSwapperMode { ExactIn, ExactOut } |
| 81 | + ``` |
| 82 | +- [ ] **Simple solutions**: Three similar lines is better than a premature abstraction |
| 83 | +- [ ] **Avoid `mut`**: Prefer immutable bindings; use `mut` only when truly necessary |
| 84 | +- [ ] **Prefer one-liners**: Inline single-use variables; avoid creating variables used only once |
| 85 | +- [ ] **Avoid `#[serde(default)]`**: Only use when the field is genuinely optional in the API response; if the field is always present, omit it |
| 86 | +- [ ] **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, .. } => ... }`) |
| 87 | + |
| 88 | +### 5. Code Organization |
| 89 | +- [ ] **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) |
| 90 | +- [ ] **Folder modules for complexity**: When a module has multiple concerns (models, client, mappers), use a folder with `mod.rs` instead of a single file |
| 91 | +- [ ] **Avoid duplication**: Search for existing implementations before writing new code; reuse existing code or crates |
| 92 | +- [ ] **Shared crates**: Reusable logic belongs in shared crates (e.g., `gem_solana`, `gem_evm`), not in utility binaries; move shared code to appropriate crates |
| 93 | +- [ ] **Bird's eye view**: Step back and identify opportunities to simplify and consolidate |
| 94 | + |
| 95 | +### 6. Async Patterns |
| 96 | +- [ ] **Tokio runtime**: Use `tokio` for async operations |
| 97 | +- [ ] **Shared state**: Use `Arc<tokio::sync::Mutex<T>>` for shared async state |
| 98 | +- [ ] **Async client structs**: Should return `Result<T, Error>` |
| 99 | + |
| 100 | +### 7. Database Patterns |
| 101 | +- [ ] **Separate models**: Database models should be separate from domain primitives |
| 102 | +- [ ] **Use `as_primitive()`**: For conversion from database models |
| 103 | +- [ ] **Repository pattern**: Access via `DatabaseClient` methods |
| 104 | + |
| 105 | +### 8. Blockchain/RPC Patterns |
| 106 | +- [ ] **Use `gem_jsonrpc::JsonRpcClient`**: For blockchain RPC interactions |
| 107 | +- [ ] **Use `primitives::hex`**: For hex encoding/decoding (not `alloy_primitives::hex`) |
| 108 | +- [ ] **U256 conversions**: Use `u256_to_biguint` and `biguint_to_u256` from `gem_evm/src/u256.rs` |
| 109 | +- [ ] **Provider pattern**: Fetch raw data via RPC, then use mapper functions for conversion |
| 110 | +- [ ] **Mapper files**: Place mapper functions in separate `*_mapper.rs` files |
| 111 | + |
| 112 | +### 9. Testing |
| 113 | +- [ ] **`#[tokio::test]`**: Use for async tests |
| 114 | +- [ ] **Test naming**: Prefix with `test_` descriptively |
| 115 | +- [ ] **Error handling**: Use `Result<(), Box<dyn std::error::Error + Send + Sync>>` |
| 116 | +- [ ] **Test data**: For long JSON (>20 lines), store in `testdata/` and use `include_str!()` |
| 117 | +- [ ] **`.unwrap()` not `.expect()`**: Never use `.expect()` in tests; use `.unwrap()` for brevity |
| 118 | +- [ ] **No `assert!` with `contains`**: Use `assert_eq!` with concrete values; `assert!(x.contains(...))` gives useless failure messages |
| 119 | +- [ ] **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. |
| 120 | +- [ ] **Methods over free functions**: Helper functions should be methods on the relevant struct, not top-level free functions |
| 121 | +- [ ] **Mock methods in testkit**: Use `Type::mock()` constructors in `testkit/` modules instead of inline struct construction in tests |
| 122 | +- [ ] **`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! }` |
| 123 | +- [ ] **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 |
| 124 | + |
| 125 | +### 10. Security |
| 126 | +- [ ] **No hardcoded secrets**: Check for API keys, passwords, credentials |
| 127 | +- [ ] **Input validation**: Validate at system boundaries (user input, external APIs) |
| 128 | +- [ ] **OWASP top 10**: Watch for command injection, XSS, SQL injection vulnerabilities |
| 129 | + |
| 130 | +## Workflow |
| 131 | + |
| 132 | +Iterate at least 2-3 times to ensure all issues are caught and fixed: |
| 133 | + |
| 134 | +### Each Iteration: |
| 135 | +1. **Analyze**: Review the diff against the checklist |
| 136 | +2. **Read**: Read the full content of each changed file to understand context |
| 137 | +3. **Fix**: Apply fixes directly using the Edit tool for each issue found |
| 138 | +4. **Format**: Run `rustfmt --edition 2024 <files>` on modified files |
| 139 | +5. **Verify**: Run `cargo clippy -p <crate> -- -D warnings` on affected crates |
| 140 | +6. **Check**: Review the changes again - new issues may have been introduced or revealed |
| 141 | + |
| 142 | +### Stop when: |
| 143 | +- No more issues are found after a full review pass |
| 144 | +- Clippy passes with no warnings |
| 145 | +- Code is properly formatted |
| 146 | + |
| 147 | +## Output Format |
| 148 | + |
| 149 | +After fixing issues, provide a summary: |
| 150 | + |
| 151 | +1. **Issues Fixed**: List each fix made with: |
| 152 | + - File and line reference |
| 153 | + - Category (from checklist above) |
| 154 | + - What was changed |
| 155 | +2. **Manual Review Needed**: Issues that require human decision (if any) |
| 156 | +3. **Verification**: Clippy/format results |
| 157 | + |
| 158 | +Severity levels for reporting: |
| 159 | +- **CRITICAL**: Security issues or bugs - fix immediately |
| 160 | +- **WARNING**: Coding standard violations - fix automatically |
| 161 | +- **SUGGESTION**: Minor improvements - fix if straightforward, otherwise note for user |
0 commit comments