fix: audit #174
fix: audit #174
Conversation
WalkthroughThis pull request encompasses multiple modifications across various modules in the Initia and Minitia standard libraries. The changes primarily focus on improving code readability, error handling, and introducing new utility functions. Key modifications include updating method calls in Rust, refactoring code formatting, enhancing error reporting in cryptographic modules, and adding new conversion methods for data structures like tables. Additionally, new fields for cache capacities have been introduced in cache-related structs. Changes
Sequence DiagramsequenceDiagram
participant Table
participant SimpleMap
Note over Table: Create Table
Table->>SimpleMap: Convert using to_simple_map()
SimpleMap-->>Table: Returns populated SimpleMap
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
precompile/modules/initia_stdlib/sources/table.move (1)
237-248: LGTM! Consider reserving capacity for the simple map.The implementation is correct and follows a clean pattern for converting a Table to a SimpleMap. However, since we know the size of the table upfront, we could optimize the conversion by reserving capacity in the simple map.
Consider adding a capacity hint to the simple map creation:
- let map = std::simple_map::new(); + let map = std::simple_map::create_with_capacity(table::length(table));precompile/modules/minitia_stdlib/sources/multisig.move (1)
490-490: LGTM! Fixed proposal expiration logic.The conditions have been correctly inverted to properly check if the current height/timestamp has exceeded the expiration height/timestamp.
Consider adding a test case that specifically verifies the boundary conditions of the expiration logic:
#[test] fun test_proposal_expiration_boundary() { let max_period = Period { height: option::some(10), timestamp: option::some(10) }; // Test height boundary assert!(!is_proposal_expired(&max_period, 100, 100, 109, 100), 1); // Not expired assert!(is_proposal_expired(&max_period, 100, 100, 110, 100), 2); // Expired // Test timestamp boundary assert!(!is_proposal_expired(&max_period, 100, 100, 100, 109), 3); // Not expired assert!(is_proposal_expired(&max_period, 100, 100, 100, 110), 4); // Expired }Also applies to: 496-496
precompile/modules/minitia_stdlib/sources/table.move (1)
237-248: Implementation looks correct but consider performance implications.The implementation correctly converts a Table to a SimpleMap by iterating through all entries. However, be aware that this operation:
- Copies all values due to the
copyconstraint on type V- Requires memory proportional to the table size
Consider documenting the performance characteristics in the function comment, especially for large tables. You might want to add:
+ /// Converts a Table to a SimpleMap by copying all entries. + /// Note: This operation requires memory proportional to the table size + /// and copies all values. Use with caution for large tables. public fun to_simple_map<K: store + copy + drop, V: store + copy>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
crates/natives/src/block.rs(1 hunks)crates/types/src/module.rs(1 hunks)precompile/modules/initia_stdlib/sources/bigdecimal.move(2 hunks)precompile/modules/initia_stdlib/sources/crypto/ed25519.move(1 hunks)precompile/modules/initia_stdlib/sources/crypto/secp256k1.move(1 hunks)precompile/modules/initia_stdlib/sources/minitswap.move(15 hunks)precompile/modules/initia_stdlib/sources/multisig.move(1 hunks)precompile/modules/initia_stdlib/sources/stableswap.move(3 hunks)precompile/modules/initia_stdlib/sources/table.move(1 hunks)precompile/modules/minitia_stdlib/sources/bigdecimal.move(2 hunks)precompile/modules/minitia_stdlib/sources/crypto/ed25519.move(1 hunks)precompile/modules/minitia_stdlib/sources/crypto/secp256k1.move(1 hunks)precompile/modules/minitia_stdlib/sources/multisig.move(1 hunks)precompile/modules/minitia_stdlib/sources/table.move(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- precompile/modules/minitia_stdlib/sources/crypto/secp256k1.move
- precompile/modules/initia_stdlib/sources/crypto/secp256k1.move
- precompile/modules/initia_stdlib/sources/stableswap.move
- precompile/modules/initia_stdlib/sources/minitswap.move
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Rust libmovevm
- GitHub Check: rust lint
🔇 Additional comments (10)
crates/natives/src/block.rs (1)
63-63: LGTM! More idiomatic method call.The change from static to instance method call style improves readability while maintaining the same functionality.
crates/types/src/module.rs (1)
61-61: LGTM! Removed unnecessary clone.Good optimization by removing the redundant
clone()call sincecodeis already owned and can be moved directly intoModule::new().precompile/modules/initia_stdlib/sources/crypto/ed25519.move (1)
47-47: LGTM! Improved error handling clarity.Good improvement by using the semantic error code
E_WRONG_PUBKEY_SIZEinstead ofPUBLIC_KEY_SIZE. This makes the error more descriptive and aligns with best practices for error handling.precompile/modules/minitia_stdlib/sources/crypto/ed25519.move (1)
47-47: LGTM! Maintains consistency with initia_stdlib.Good improvement by using the semantic error code
E_WRONG_PUBKEY_SIZE, maintaining consistency with the parallel change in initia_stdlib.precompile/modules/initia_stdlib/sources/bigdecimal.move (2)
120-123: LGTM! Added division by zero check.The assertion correctly prevents division by zero when calculating the reciprocal of a BigDecimal.
626-631: LGTM! Added test for division by zero check.The test verifies that calling
revwith a zero BigDecimal correctly triggers the expected failure.precompile/modules/minitia_stdlib/sources/bigdecimal.move (2)
120-123: LGTM! Added division by zero check.The assertion correctly prevents division by zero when calculating the reciprocal of a BigDecimal.
626-631: LGTM! Added test for division by zero check.The test verifies that calling
revwith a zero BigDecimal correctly triggers the expected failure.precompile/modules/initia_stdlib/sources/multisig.move (1)
490-490: Verify the expiration logic change.The expiration logic has been inverted to check if the current height/timestamp is greater than or equal to the sum of max period and proposal height/timestamp. This change appears to be correct as it properly determines if a proposal has expired by checking if we've moved past the voting window.
Let's verify the expiration logic by checking test cases:
Also applies to: 496-496
✅ Verification successful
Expiration logic is correctly implemented
The implementation
height >= (max_voting_period_height + proposal_height)is correct as it properly determines if the current block height has moved past the voting window (proposal_height + max_voting_period_height). This ensures proposals expire exactly when intended, neither too early nor too late.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test cases that verify proposal expiration logic. # Test: Search for test cases that verify proposal expiration. rg -A 10 'vote_after_(height|timestamp)_expired' --type moveLength of output: 91
Script:
#!/bin/bash # Search for test cases and implementation of proposal expiration rg -l "is_proposal_expired" . # Get the full context of the is_proposal_expired function rg -B 10 -A 10 "is_proposal_expired" . # Search for test files related to proposal expiration fd ".*test.*\.move$"Length of output: 29384
precompile/modules/minitia_stdlib/sources/table.move (1)
237-248: Add unit tests for the new function.Please add test cases to verify:
- Converting an empty table
- Converting a table with a single entry
- Converting a table with multiple entries
- Preserving the order of entries
Here's a suggested test implementation:
#[test(account = @0x1)] fun test_to_simple_map(account: &signer) { let t = new<u64, u8>(); // Test empty table let map = to_simple_map(&t); assert!(std::simple_map::length(&map) == 0, 1); // Test single entry add(&mut t, 1, 10); let map = to_simple_map(&t); assert!(std::simple_map::length(&map) == 1, 2); assert!(*std::simple_map::borrow(&map, 1) == 10, 3); // Test multiple entries add(&mut t, 2, 20); add(&mut t, 3, 30); let map = to_simple_map(&t); assert!(std::simple_map::length(&map) == 3, 4); assert!(*std::simple_map::borrow(&map, 1) == 10, 5); assert!(*std::simple_map::borrow(&map, 2) == 20, 6); assert!(*std::simple_map::borrow(&map, 3) == 30, 7); move_to(account, TableHolder { t }); }✅ Verification successful
Add unit tests for the new
to_simple_mapfunctionThe verification confirms that there are no existing tests for the
to_simple_mapfunction in the codebase. The suggested test cases in the review comment are appropriate and should be implemented to ensure the function's correctness.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find test files in the module directory fd -e move . precompile/modules/minitia_stdlib/sources/ # Search for existing tests of to_simple_map rg "to_simple_map" precompile/modules/minitia_stdlib/sources/ -B 2 -A 2Length of output: 3893
Script:
#!/bin/bash # Look for test module and test cases in table.move rg "#\[test\]" precompile/modules/minitia_stdlib/sources/table.move -B 2 -A 10 # Also check for any test module declarations rg "module.*tests" precompile/modules/minitia_stdlib/sources/table.move -A 5Length of output: 159
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/storage/src/script_cache.rs (2)
47-55: Consider enhancing the error message with size information.While the capacity check is correctly implemented, the error message could be more informative by including the script size and cache capacity.
- eprintln!("WARNING: failed to insert script into cache"); + eprintln!( + "WARNING: failed to insert script into cache (size: {} bytes, capacity: {} bytes)", + allocated_size, + self.capacity + );
88-96: Consider enhancing the error message with size information (similar to previous comment).For consistency with the previous suggestion, enhance the error message here as well.
- eprintln!("WARNING: failed to insert script into cache"); + eprintln!( + "WARNING: failed to insert script into cache (size: {} bytes, capacity: {} bytes)", + allocated_size, + self.capacity + );crates/storage/src/module_cache.rs (3)
82-85: Consider adding size information to the error message.The error message includes module ID, which is helpful. Consider also adding size information for better debugging.
eprintln!( - "WARNING: failed to insert module {:?} into cache", + "WARNING: failed to insert module {:?} into cache (size: {} bytes, capacity: {} bytes)", module_id.short_str_lossless().to_string(), + allocated_size, + self.capacity );Also applies to: 102-106
130-141: Consider adding size information to the error message (similar to previous comment).For consistency, enhance the error message with size information here as well.
eprintln!( - "WARNING: failed to insert module {:?} into cache", + "WARNING: failed to insert module {:?} into cache (size: {} bytes, capacity: {} bytes)", module_id.short_str_lossless().to_string(), + allocated_size, + self.capacity );
177-189: Consider adding size information to the error message (similar to previous comments).For consistency across all methods, enhance the error message with size information here as well.
eprintln!( - "WARNING: failed to insert module {:?} into cache", + "WARNING: failed to insert module {:?} into cache (size: {} bytes, capacity: {} bytes)", id.short_str_lossless().to_string(), + allocated_size, + self.capacity );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/storage/src/module_cache.rs(6 hunks)crates/storage/src/script_cache.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust libmovevm
🔇 Additional comments (2)
crates/storage/src/script_cache.rs (1)
15-15: LGTM! Capacity field addition and initialization look good.The capacity field is correctly initialized in megabytes and converted to bytes. The same value is consistently used for CLruCache configuration.
Also applies to: 22-22
crates/storage/src/module_cache.rs (1)
54-54: LGTM! Capacity field addition and initialization are consistent.The implementation matches the approach in script_cache.rs, maintaining consistency across cache types.
Also applies to: 63-63
Description
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Release Notes
New Features
to_simple_mapfunction in bothinitia_std::tableandminitia_std::tablemodules to convert tables to simple maps.capacityin bothInitiaModuleCacheandInitiaScriptCacheto manage cache limits.Bug Fixes
Code Improvements