Conversation
WalkthroughThis update introduces a new Rust crate implementing Monero's seed algorithm with multilingual support, comprehensive tests, and language word lists. It expands the workspace, refines dependency management, adds a patch to increase Monero wallet RPC timeouts, and enhances the Monero Rust bindings build script with embedded patching and iOS cross-compilation support. A minor public re-export is also applied. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SeedCrate
participant WordList
participant RNG
User->>SeedCrate: Seed::new(rng, language)
SeedCrate->>RNG: Generate 32 bytes entropy
SeedCrate->>WordList: Map entropy to word indices
WordList-->>SeedCrate: Word list for language
SeedCrate->>SeedCrate: Compute checksum, assemble phrase
SeedCrate-->>User: Seed struct with phrase
User->>SeedCrate: Seed::from_string(language, phrase)
SeedCrate->>WordList: Lookup word indices (with prefix trimming)
SeedCrate->>SeedCrate: Validate checksum and entropy
SeedCrate-->>User: Seed struct or error
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🔭 Outside diff range comments (1)
seed/src/words/ang.rs (1)
1-1629: Missing constant declaration for the word list
The file currently contains only a raw array literal without apub constorstaticbinding. It won’t compile as-is and the word list won’t be exposed to the crate.Please define and expose the array, for example:
pub const WORDLIST_ANG: &[&str] = &[ "like", // ... "weary", ];
♻️ Duplicate comments (2)
seed/src/words/eo.rs (1)
1-1628: Missing constant declaration for the Esperanto word list
Same issue as inang.rs: this file only contains a literal&[...]. Wrap it in apub constorstaticso it compiles and is accessible.seed/src/words/en.rs (1)
1-1628: Missing constant declaration for the English word list
Same issue as noted inang.rs: define this literal as apub constorstaticto compile and expose the list.
🧹 Nitpick comments (16)
Cargo.toml (2)
3-3: Add new workspace memberseed
Theseedcrate is correctly added to the workspace members. Consider alphabetizing the list for consistency and maintainability.Ensure the full workspace still builds:
cargo build --workspace.
5-6: Introduce workspace lint configuration
Settingrust.unused_crate_dependencies = "warn"helps catch unused dependencies. Great addition—consider expanding lints (e.g.,rust.denied-warnings) as needed.monero-rpc-pool/src/lib.rs (1)
8-9: Re-exportNetworkpublicly
The public re-export simplifies downstream usage across crates. Consider updating the crate-level documentation to mentionNetworkavailability.seed/src/words/nl.rs (1)
1-1628: To comprehensively check for duplicates, sort order, and test coverage, let’s extract the actual word entries and run the following:#!/usr/bin/env bash words_file=seed/src/words/nl.rs # 1. Extract each quoted entry (the Dutch words) into a list file sed -n 's/^\s*"\([^"]*\)".*$/\1/' "$words_file" > /tmp/nl_words.txt # 2. Show any duplicates echo "Duplicate words (if any):" sort /tmp/nl_words.txt | uniq -d || echo " → none" # 3. Check if the list is strictly sorted echo echo "Sorting check:" if diff <(cat /tmp/nl_words.txt) <(sort /tmp/nl_words.txt); then echo " → list is sorted" else echo " → list is NOT sorted" fi # 4. Confirm there’s a test referencing NL in seed/src/tests.rs echo echo "NL coverage in tests:" grep -R "nl" -n seed/src/tests.rs || echo " → no NL-specific tests found"seed/src/words/de.rs (1)
1-1629: Let’s check duplicates and enforce case-insensitive alphabetical ordering (which matches German dictionary order) rather than raw Unicode code points:#!/usr/bin/env bash words_file=seed/src/words/de.rs # Extract words into a file grep -oP '^\s*"\K[^"]+(?=")' "$words_file" > /tmp/de_words.txt # 1) Duplicate entries echo "=== Duplicate entries ===" sort /tmp/de_words.txt | uniq -d # 2) Case-insensitive alphabetical order check echo "=== Case-insensitive alphabetical order ===" awk ' NR==1 { prev=tolower($0); next } { curr=tolower($0) if (curr < prev) { print "Order error at line " NR ": \"" $0 "\" should come before previous entry" exit 1 } prev=curr } END { print "All entries are in case-insensitive alphabetical order" } ' /tmp/de_words.txtseed/README.md (1)
8-8: Adjust heading level for consistency
Per Markdown conventions, heading levels should increase by one. Change### Cargo Featuresto## Cargo Features.-### Cargo Features +## Cargo Featuresseed/src/words/pt.rs (1)
1-1628: Consider externalizing the word list for maintainability.
Loading a newline-delimited file viainclude_str!and parsing it at compile time can reduce source bloat and simplify updates.seed/src/words/fr.rs (1)
1-1628: Consider externalizing the word list viainclude_str!.
Decoupling the data into an external file simplifies maintenance and keeps code size manageable.seed/src/words/it.rs (1)
1-1628: Consider centralizing word list maintenance.
Usinginclude_str!to embed a plain-text list at compile time can make updates easier and reduce merge conflicts in code.seed/src/words/ja.rs (1)
1-1628: Consider parsing an external word file at compile time.
Leveraginginclude_str!on a newline-delimited file can simplify updates, reduce code size, and avoid merge conflicts on large arrays.seed/Cargo.toml (1)
19-19: Consider pinning Git dependencies to specific commits or tags.Git dependencies without version tags or commit hashes can lead to build instability if the upstream repository changes.
-std-shims = { git = "https://github.com/serai-dex/serai", version = "^0.1.1", default-features = false } +std-shims = { git = "https://github.com/serai-dex/serai", rev = "COMMIT_HASH", version = "^0.1.1", default-features = false } -monero-primitives = { git = "https://github.com/serai-dex/serai", default-features = false, features = ["std"] } +monero-primitives = { git = "https://github.com/serai-dex/serai", rev = "COMMIT_HASH", default-features = false, features = ["std"] }Also applies to: 30-30
monero-sys/build.rs (1)
94-152: Improve OpenSSL build configuration for iOS.The OpenSSL build function has several areas for improvement:
- The prefix is set to
/tmpbut the comment says "We don't actually install"- Missing error handling for some edge cases
- Could benefit from caching the OpenSSL build more robustly
"--prefix=/tmp", // We don't actually install, just build + &format!("--prefix={}", openssl_dir.join("install").display()),seed/src/tests.rs (2)
202-202: Consider removing or conditionalizing debug print statementsThe
println!statements on lines 202 and 227 will produce output during test runs, which can be noisy in CI/CD environments. Consider either removing them or using conditional compilation with a debug feature flag.- println!("{}. language: {:?}, seed: {}", line!(), vector.language, vector.seed.clone()); + #[cfg(feature = "debug-tests")] + println!("{}. language: {:?}, seed: {}", line!(), vector.language, vector.seed.clone());- println!("{}. seed: {}", line!(), *seed.to_string()); + #[cfg(feature = "debug-tests")] + println!("{}. seed: {}", line!(), *seed.to_string());Also applies to: 227-227
184-190: Reuse existing trimming logic from the parent moduleThe
trim_by_langfunction duplicates the unique prefix length logic that's already defined in theLANGUAGESmap in the parent module. This could lead to maintenance issues if the prefix lengths change.Consider importing and using the
trimfunction from the parent module:- fn trim_by_lang(word: &str, lang: Language) -> String { - if lang != Language::DeprecatedEnglish { - word.chars().take(LANGUAGES[&lang].unique_prefix_length).collect() - } else { - word.to_string() - } - } + fn trim_by_lang(word: &str, lang: Language) -> String { + if lang != Language::DeprecatedEnglish { + trim(word, LANGUAGES[&lang].unique_prefix_length).to_string() + } else { + word.to_string() + } + }seed/src/lib.rs (2)
175-197: Add documentation for the base conversion algorithmThe conversion from bytes to word indices uses a complex base conversion algorithm that would benefit from detailed documentation explaining the mathematical relationship.
Add a comment block explaining the algorithm:
// convert to words // 4 bytes -> 3 words. 8 digits base 16 -> 3 digits base 1626 + // Algorithm: Each 4-byte segment is treated as a little-endian u32, then converted + // to 3 word indices using base conversion. The indices are calculated such that: + // - indices[1] = segment % list_len + // - indices[2] = (indices[1] + segment / list_len) % list_len + // - indices[3] = (indices[2] + segment / list_len^2) % list_len + // This ensures reversibility when converting back from words to bytes. let mut segment = [0; 4];
278-291: Document the inverse base conversion algorithmThe
innerclosure performs the inverse of the base conversion inkey_to_seed, but its logic is not immediately clear.Add documentation explaining the inverse algorithm:
+ // Calculate the base value for reconstruction + // This reverses the base conversion from key_to_seed by computing: + // base = (list_len - indices[i] + indices[i+1]) % list_len + // Then shifts by multiplying by list_len^i let inner = |i| { let mut base = (lang_word_list.word_list.len() - indices[i] + indices[i + 1]) % lang_word_list.word_list.len();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.toml(1 hunks)monero-rpc-pool/src/lib.rs(1 hunks)monero-sys/Cargo.toml(2 hunks)monero-sys/build.rs(2 hunks)monero-sys/patches/wallet2_3min_remote_rpc_node_timeout.patch(1 hunks)seed/Cargo.toml(1 hunks)seed/LICENSE(1 hunks)seed/README.md(1 hunks)seed/src/lib.rs(1 hunks)seed/src/tests.rs(1 hunks)seed/src/words/ang.rs(1 hunks)seed/src/words/de.rs(1 hunks)seed/src/words/en.rs(1 hunks)seed/src/words/eo.rs(1 hunks)seed/src/words/es.rs(1 hunks)seed/src/words/fr.rs(1 hunks)seed/src/words/it.rs(1 hunks)seed/src/words/ja.rs(1 hunks)seed/src/words/jbo.rs(1 hunks)seed/src/words/nl.rs(1 hunks)seed/src/words/pt.rs(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
seed/README.md
8-8: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (18)
monero-sys/Cargo.toml (2)
8-8: Enabletokiofeature onbackoff
The feature addition aligns with asynchronous build script requirements. Confirm that the build script leveragesbackoff::futurecorrectly.
17-17: Adddiffyas a build dependency
diffyis required for embedded patch diffing inbuild.rs. Ensure no version conflicts arise.seed/LICENSE (1)
1-22: License file looks correct
The MIT license text is complete, formatted properly, and covers the newseedcrate.seed/src/words/pt.rs (2)
1-1628: Static Portuguese word list is correctly formatted and alphabetically ordered.
The slice literal uses proper quoting, commas, and UTF-8 encoding, matching the structure of the other language files.
1-1628: Verify Portuguese word list for duplicates and official alignment.
Ensure there are no duplicate entries and that the ordering matches the Monero Portuguese mnemonic specification.#!/bin/bash # Count entries and check for duplicates words=$(awk -F\" '/"/{print $2}' seed/src/words/pt.rs) echo "Total entries: $(echo "$words" | wc -l)" echo "Duplicates:" echo "$words" | sort | uniq -dseed/src/words/fr.rs (2)
1-1628: Static French word list is correctly formatted and sorted.
All entries adhere to the required slice structure and use consistent UTF-8 quoting.
1-1628: Verify French word list against the official Monero list.
Check for duplicates and confirm the word order matches the French mnemonic standard.#!/bin/bash words=$(awk -F\" '/"/{print $2}' seed/src/words/fr.rs) echo "Total entries: $(echo "$words" | wc -l)" echo "Duplicates:" echo "$words" | sort | uniq -dseed/src/words/it.rs (2)
1-1628: Static Italian word list is correctly formatted and alphabetically ordered.
The entries follow the slice literal conventions consistent across languages.
1-1628: Verify Italian word list entry count and duplicates.
Make sure the file contains exactly 1628 unique words matching the Monero Italian mnemonic list.#!/bin/bash words=$(awk -F\" '/"/{print $2}' seed/src/words/it.rs) echo "Total entries: $(echo "$words" | wc -l)" echo "Duplicates:" echo "$words" | sort | uniq -dseed/src/words/ja.rs (2)
1-1628: Static Japanese word list is correctly formatted and encoded.
Entries use proper hiragana/katakana strings in a slice literal, consistent with the other language files.
1-1628: Verify Japanese word list entry count and duplicates.
Ensure the list contains exactly 1628 unique entries as required by the Monero Japanese mnemonic standard.#!/bin/bash entries=$(awk -F\" '/"/{print $2}' seed/src/words/ja.rs) echo "Total entries: $(echo "$entries" | wc -l)" echo "Duplicates:" echo "$entries" | sort | uniq -dseed/src/words/jbo.rs (1)
1-1628: Verify no duplicate words exist in the Lojban word list.The word list structure looks correct. However, please ensure there are no duplicate entries which could compromise seed phrase security.
#!/bin/bash # Description: Check for duplicate words in the Lojban word list # Expected: No duplicates should be found # Extract words and check for duplicates rg --no-line-number '"([^"]+)"' seed/src/words/jbo.rs -o -r '$1' | sort | uniq -d # Count total unique words echo "Total unique words: $(rg --no-line-number '"([^"]+)"' seed/src/words/jbo.rs -o -r '$1' | sort -u | wc -l)"seed/src/words/es.rs (1)
1-1628: Verify Spanish word list integrity.The Spanish word list appears properly formatted with correct accents. Please verify no duplicates exist and that all language files have consistent word counts.
#!/bin/bash # Description: Verify Spanish word list and check consistency across languages # Check for duplicates in Spanish word list echo "Checking for duplicates in Spanish word list:" rg --no-line-number '"([^"]+)"' seed/src/words/es.rs -o -r '$1' | sort | uniq -d # Count words in each language file echo -e "\nWord count per language file:" for file in seed/src/words/*.rs; do count=$(rg --no-line-number '"[^"]+"' "$file" | wc -l) echo "$(basename $file): $count words" doneseed/Cargo.toml (2)
6-6: Repository URL points to external repository.The repository URL points to
kayabaNerve/monero-wallet-utilinstead ofUnstoppableSwap/core. Is this intentional? If this is meant to be an external dependency, consider documenting this clearly.
9-9: Rust version 1.80 appears to be incorrect.The specified Rust version
1.80doesn't exist yet. The latest stable Rust version is around 1.75. This should likely be1.70or similar.-rust-version = "1.80" +rust-version = "1.70"Likely an incorrect or invalid review comment.
monero-sys/patches/wallet2_3min_remote_rpc_node_timeout.patch (1)
1-34: Consider if 3-minute timeout is appropriate for user experience.Increasing the timeout from 15 seconds to 3 minutes seems excessive and could lead to poor user experience with hanging requests. Since the comment mentions that "timeout and retrying logic is delegated to monero-rpc-pool", consider if a lower timeout (e.g., 30-60 seconds) would be more appropriate while still allowing the RPC pool to handle retries.
monero-sys/build.rs (1)
33-92: Consider the implications of modifying source files during build.The patch application modifies Monero source files in-place during the build process. This approach could cause issues:
- Dirty working directories if builds fail
- Conflicts with version control if the monero submodule is being worked on
- Potential race conditions in parallel builds
Consider documenting this behavior clearly or exploring alternative approaches like copying sources to a build directory first.
seed/src/lib.rs (1)
1-353: Well-structured implementation with strong security practicesThe implementation demonstrates excellent security practices:
- Consistent use of
Zeroizingfor sensitive data throughout- Comprehensive language support with proper word list management
- Thorough checksum validation with CRC32
- Clean API design with proper error handling (except for the noted panic)
- Const-time CRC32 table generation avoiding timing attacks
The modular structure with separate word lists and clear separation of concerns makes this a maintainable implementation of the Monero seed algorithm.
|
|
||
| // Help CMake find OpenSSL for iOS | ||
| // We need to build or provide OpenSSL for iOS | ||
| let ios_sdk_path = "/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.1.sdk"; |
There was a problem hiding this comment.
Hardcoded iOS SDK version will break when Xcode updates.
The iOS SDK path is hardcoded to version 18.1, which will cause build failures when users have different Xcode versions installed.
Consider detecting the SDK dynamically:
- let ios_sdk_path = "/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.1.sdk";
+ let sdk_output = Command::new("xcrun")
+ .args(&["--sdk", "iphoneos", "--show-sdk-path"])
+ .output()
+ .expect("Failed to get iOS SDK path");
+ let ios_sdk_path = String::from_utf8_lossy(&sdk_output.stdout).trim().to_string();
// Also update in build_openssl_for_ios():
- .env("CROSS_SDK", "iPhoneOS18.1.sdk")
+ .env("CROSS_SDK", "iPhoneOS.sdk")Also applies to: 133-133
🤖 Prompt for AI Agents
In monero-sys/build.rs at lines 133 and 187, the iOS SDK path is hardcoded to
version 18.1, which will break builds when Xcode updates. Replace the hardcoded
path with code that dynamically detects the installed iOS SDK path, for example
by invoking `xcrun --sdk iphoneos --show-sdk-path` and capturing its output to
set the SDK path variable. This ensures compatibility with different Xcode
versions.
| if (words.len() != SEED_LENGTH) && (words.len() != SEED_LENGTH_WITH_CHECKSUM) { | ||
| panic!("invalid seed passed to seed_to_bytes"); | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling
The function uses panic! for invalid input validation, which can crash the application. This should return a proper error instead.
if (words.len() != SEED_LENGTH) && (words.len() != SEED_LENGTH_WITH_CHECKSUM) {
- panic!("invalid seed passed to seed_to_bytes");
+ return Err(SeedError::InvalidSeed);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (words.len() != SEED_LENGTH) && (words.len() != SEED_LENGTH_WITH_CHECKSUM) { | |
| panic!("invalid seed passed to seed_to_bytes"); | |
| } | |
| if (words.len() != SEED_LENGTH) && (words.len() != SEED_LENGTH_WITH_CHECKSUM) { | |
| return Err(SeedError::InvalidSeed); | |
| } |
🤖 Prompt for AI Agents
In seed/src/lib.rs around lines 221 to 223, replace the panic! call used for
invalid seed length validation with proper error handling by returning a Result
type with an appropriate error variant or message. Modify the function signature
to return a Result if it does not already, and ensure that invalid input cases
return an Err instead of panicking, allowing the caller to handle the error
gracefully.
|
Superseded by #439, just keeping this open for reference |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
Tests
Chores