Skip to content

Commit 72707e7

Browse files
CopilotSteake
andcommitted
Address code review comments: fix bip39 crate, chain IDs, documentation, and safety improvements
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
1 parent cdce26c commit 72707e7

9 files changed

Lines changed: 220 additions & 45 deletions

File tree

crates/bitcell-wallet/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ rand_core.workspace = true
2121
hex.workspace = true
2222

2323
# BIP39/BIP32 seed phrases
24-
tiny-bip39 = "1.0"
24+
bip39 = "2.0"
2525
hmac = "0.12"
2626
pbkdf2 = { version = "0.12", default-features = false }
2727

crates/bitcell-wallet/src/address.rs

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ impl Address {
4545
}
4646

4747
/// Generate a BitCell address from a public key
48+
///
49+
/// Uses double SHA256 and takes the first 20 bytes as the address.
50+
/// This is a simplified scheme for BitCell's native addresses.
4851
pub fn from_public_key_bitcell(public_key: &PublicKey, index: u32) -> Self {
49-
// BitCell address = RIPEMD160(SHA256(compressed_pubkey))
50-
// For simplicity, we use SHA256 twice (similar to Bitcoin but simplified)
5152
let pubkey_bytes = public_key.as_bytes();
5253
let hash1 = Sha256::digest(pubkey_bytes);
5354
let hash2 = Sha256::digest(hash1);
@@ -58,10 +59,15 @@ impl Address {
5859
}
5960

6061
/// Generate a Bitcoin P2PKH address from a public key
62+
///
63+
/// Note: This is a simplified implementation using double SHA256.
64+
/// For full Bitcoin compatibility, use RIPEMD160(SHA256(pubkey)).
65+
/// Addresses generated here are for internal use and may not be
66+
/// compatible with external Bitcoin wallets.
6167
pub fn from_public_key_bitcoin(public_key: &PublicKey, testnet: bool, index: u32) -> Self {
6268
let pubkey_bytes = public_key.as_bytes();
63-
// Bitcoin address = RIPEMD160(SHA256(compressed_pubkey))
6469
// Simplified: using double SHA256 and taking 20 bytes
70+
// For full compatibility, implement RIPEMD160(SHA256(pubkey))
6571
let hash1 = Sha256::digest(pubkey_bytes);
6672
let hash2 = Sha256::digest(hash1);
6773
let address_bytes = hash2[..20].to_vec();
@@ -71,10 +77,16 @@ impl Address {
7177
}
7278

7379
/// Generate an Ethereum address from a public key
80+
///
81+
/// Note: This is a simplified implementation using SHA256.
82+
/// For full Ethereum compatibility, use Keccak256 on the uncompressed
83+
/// public key (excluding the 0x04 prefix) and take the last 20 bytes.
84+
/// Addresses generated here are for internal use and may not be
85+
/// compatible with external Ethereum wallets.
7486
pub fn from_public_key_ethereum(public_key: &PublicKey, testnet: bool, index: u32) -> Self {
75-
// Ethereum address = last 20 bytes of Keccak256(uncompressed_pubkey[1:])
76-
// Simplified: using SHA256 for demonstration
7787
let pubkey_bytes = public_key.as_bytes();
88+
// Simplified: using SHA256 instead of Keccak256
89+
// For full compatibility, implement Keccak256(uncompressed_pubkey[1:])
7890
let hash = Sha256::digest(pubkey_bytes);
7991
let address_bytes = hash[12..].to_vec(); // Last 20 bytes
8092

crates/bitcell-wallet/src/balance.rs

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,15 +54,18 @@ impl Balance {
5454
}
5555

5656
/// Format with fixed decimal places
57+
///
58+
/// Note: Uses u128 for intermediate calculations to prevent overflow
59+
/// with large fractions (e.g., Ethereum's 18 decimals).
5760
pub fn format_fixed(&self, decimal_places: u8) -> String {
5861
let decimals = self.chain.decimals() as u32;
5962
let divisor = 10u64.pow(decimals);
6063
let whole = self.amount / divisor;
6164
let fraction = self.amount % divisor;
6265

63-
// Scale fraction to desired decimal places
66+
// Scale fraction to desired decimal places using u128 to prevent overflow
6467
let scale = 10u64.pow(decimal_places as u32);
65-
let scaled_fraction = (fraction * scale) / divisor;
68+
let scaled_fraction = ((fraction as u128 * scale as u128) / divisor as u128) as u64;
6669

6770
format!(
6871
"{}.{:0>width$} {}",
@@ -99,6 +102,10 @@ impl Balance {
99102
}
100103

101104
/// Convert amount from one unit to smallest unit
105+
///
106+
/// Note: This conversion can lose precision for very large values or
107+
/// values with many decimal places due to floating-point limitations.
108+
/// For precise conversions, consider using a decimal arithmetic library.
102109
pub fn from_units(amount: f64, chain: Chain) -> Self {
103110
let decimals = chain.decimals() as u32;
104111
let multiplier = 10u64.pow(decimals);
@@ -127,6 +134,9 @@ impl std::fmt::Display for Balance {
127134
}
128135

129136
/// Balance tracker for multiple addresses
137+
///
138+
/// Note: When deserializing, call `rebuild_totals()` to ensure
139+
/// the cached totals are correctly populated.
130140
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
131141
pub struct BalanceTracker {
132142
/// Balances by address string
@@ -145,6 +155,14 @@ impl BalanceTracker {
145155
}
146156
}
147157

158+
/// Rebuild cached totals after deserialization
159+
///
160+
/// Call this method after deserializing a BalanceTracker to ensure
161+
/// the totals cache is correctly populated.
162+
pub fn rebuild_totals(&mut self) {
163+
self.update_totals();
164+
}
165+
148166
/// Set balance for an address
149167
pub fn set_balance(&mut self, address: &Address, balance: Balance) {
150168
let key = address.to_string_formatted();

crates/bitcell-wallet/src/chain.rs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,19 @@ pub enum Chain {
2424

2525
impl Chain {
2626
/// Get the chain ID
27+
///
28+
/// Each chain has a unique identifier to prevent replay attacks across chains.
29+
/// - BitCell: 8888 (custom chain ID)
30+
/// - Bitcoin: 0 (Bitcoin doesn't use chain IDs traditionally)
31+
/// - Bitcoin Testnet: 1
32+
/// - Ethereum Mainnet: 1 (EIP-155)
33+
/// - Ethereum Sepolia: 11155111
2734
pub fn chain_id(&self) -> u32 {
2835
match self {
29-
Chain::BitCell => 1,
30-
Chain::Bitcoin => 0,
31-
Chain::BitcoinTestnet => 1,
32-
Chain::Ethereum => 1,
36+
Chain::BitCell => 8888, // Unique BitCell chain ID
37+
Chain::Bitcoin => 0, // Bitcoin mainnet
38+
Chain::BitcoinTestnet => 1, // Bitcoin testnet
39+
Chain::Ethereum => 1, // Ethereum mainnet (EIP-155)
3340
Chain::EthereumSepolia => 11155111,
3441
Chain::Custom(id) => *id,
3542
}

crates/bitcell-wallet/src/history.rs

Lines changed: 83 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,15 @@ impl TransactionRecord {
106106
}
107107

108108
/// Get a short version of the tx hash
109+
///
110+
/// Returns a truncated hash in the format "prefix...suffix".
111+
/// Assumes the hash is an ASCII hex string.
109112
pub fn short_hash(&self) -> String {
110-
if self.tx_hash.len() > 16 {
111-
format!("{}...{}", &self.tx_hash[..8], &self.tx_hash[self.tx_hash.len()-8..])
113+
let chars: Vec<char> = self.tx_hash.chars().collect();
114+
if chars.len() > 16 {
115+
let prefix: String = chars[..8].iter().collect();
116+
let suffix: String = chars[chars.len()-8..].iter().collect();
117+
format!("{}...{}", prefix, suffix)
112118
} else {
113119
self.tx_hash.clone()
114120
}
@@ -137,15 +143,76 @@ impl TransactionRecord {
137143
}
138144

139145
/// Format date for display
146+
///
147+
/// Returns a human-readable date string in the format "YYYY-MM-DD HH:MM:SS UTC".
140148
pub fn format_date(&self) -> String {
141-
// Simple date formatting (YYYY-MM-DD HH:MM)
142-
use std::time::{Duration, UNIX_EPOCH};
143-
let datetime = UNIX_EPOCH + Duration::from_secs(self.timestamp);
144-
format!("{:?}", datetime) // Simplified; in production use chrono
149+
// Convert Unix timestamp to date components
150+
const SECONDS_PER_MINUTE: u64 = 60;
151+
const SECONDS_PER_HOUR: u64 = 3600;
152+
const SECONDS_PER_DAY: u64 = 86400;
153+
154+
let mut remaining = self.timestamp;
155+
156+
// Calculate days since epoch
157+
let days = remaining / SECONDS_PER_DAY;
158+
remaining %= SECONDS_PER_DAY;
159+
160+
// Calculate time components
161+
let hours = remaining / SECONDS_PER_HOUR;
162+
remaining %= SECONDS_PER_HOUR;
163+
let minutes = remaining / SECONDS_PER_MINUTE;
164+
let seconds = remaining % SECONDS_PER_MINUTE;
165+
166+
// Calculate year, month, day from days since epoch (1970-01-01)
167+
let (year, month, day) = days_to_ymd(days);
168+
169+
format!("{:04}-{:02}-{:02} {:02}:{:02}:{:02} UTC", year, month, day, hours, minutes, seconds)
170+
}
171+
}
172+
173+
/// Convert days since epoch to year, month, day
174+
fn days_to_ymd(days: u64) -> (u64, u64, u64) {
175+
// Simplified calculation - handles dates from 1970 onwards
176+
let mut remaining_days = days;
177+
let mut year = 1970u64;
178+
179+
loop {
180+
let days_in_year = if is_leap_year(year) { 366 } else { 365 };
181+
if remaining_days < days_in_year {
182+
break;
183+
}
184+
remaining_days -= days_in_year;
185+
year += 1;
186+
}
187+
188+
let days_in_months: [u64; 12] = if is_leap_year(year) {
189+
[31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
190+
} else {
191+
[31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
192+
};
193+
194+
let mut month = 1u64;
195+
for days_in_month in days_in_months.iter() {
196+
if remaining_days < *days_in_month {
197+
break;
198+
}
199+
remaining_days -= *days_in_month;
200+
month += 1;
145201
}
202+
203+
let day = remaining_days + 1;
204+
(year, month, day)
205+
}
206+
207+
/// Check if a year is a leap year
208+
fn is_leap_year(year: u64) -> bool {
209+
(year % 4 == 0 && year % 100 != 0) || (year % 400 == 0)
146210
}
147211

148212
/// Transaction history manager
213+
///
214+
/// Note: When deserializing, call `rebuild_index()` to ensure
215+
/// the hash index is correctly populated for lookups.
149216
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
150217
pub struct TransactionHistory {
151218
/// All transactions
@@ -164,6 +231,16 @@ impl TransactionHistory {
164231
}
165232
}
166233

234+
/// Rebuild the hash index after deserialization
235+
///
236+
/// Call this method after deserializing a TransactionHistory to ensure
237+
/// lookups via get() and get_mut() work correctly.
238+
pub fn ensure_indexed(&mut self) {
239+
if self.hash_index.is_empty() && !self.transactions.is_empty() {
240+
self.rebuild_index();
241+
}
242+
}
243+
167244
/// Add a transaction to history
168245
pub fn add(&mut self, record: TransactionRecord) {
169246
let hash = record.tx_hash.clone();

crates/bitcell-wallet/src/mnemonic.rs

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
//! for wallet creation and recovery.
55
66
use crate::{Error, Result};
7-
use bip39::{Language, Mnemonic as Bip39Mnemonic, MnemonicType, Seed};
7+
use bip39::{Language, Mnemonic as Bip39Mnemonic};
88
use zeroize::Zeroize;
99

1010
/// Mnemonic word count options
@@ -20,11 +20,12 @@ pub enum WordCount {
2020
}
2121

2222
impl WordCount {
23-
fn to_mnemonic_type(self) -> MnemonicType {
23+
/// Get the number of entropy bytes for this word count
24+
fn entropy_bytes(self) -> usize {
2425
match self {
25-
WordCount::Words12 => MnemonicType::Words12,
26-
WordCount::Words18 => MnemonicType::Words18,
27-
WordCount::Words24 => MnemonicType::Words24,
26+
WordCount::Words12 => 16, // 128 bits
27+
WordCount::Words18 => 24, // 192 bits
28+
WordCount::Words24 => 32, // 256 bits
2829
}
2930
}
3031
}
@@ -38,7 +39,19 @@ pub struct Mnemonic {
3839
impl Mnemonic {
3940
/// Generate a new random mnemonic with the specified word count
4041
pub fn generate(word_count: WordCount) -> Self {
41-
let mnemonic = Bip39Mnemonic::new(word_count.to_mnemonic_type(), Language::English);
42+
let entropy_size = word_count.entropy_bytes();
43+
let mut entropy = vec![0u8; entropy_size];
44+
45+
// Use rand to generate entropy
46+
use rand::RngCore;
47+
rand::thread_rng().fill_bytes(&mut entropy);
48+
49+
let mnemonic = Bip39Mnemonic::from_entropy(&entropy)
50+
.expect("Valid entropy size should always produce valid mnemonic");
51+
52+
// Securely clear entropy
53+
entropy.zeroize();
54+
4255
Self { inner: mnemonic }
4356
}
4457

@@ -49,35 +62,37 @@ impl Mnemonic {
4962

5063
/// Parse a mnemonic from a phrase string
5164
pub fn from_phrase(phrase: &str) -> Result<Self> {
52-
let mnemonic = Bip39Mnemonic::from_phrase(phrase, Language::English)
65+
let mnemonic = Bip39Mnemonic::parse_in_normalized(Language::English, phrase)
5366
.map_err(|e| Error::InvalidMnemonic(e.to_string()))?;
5467
Ok(Self { inner: mnemonic })
5568
}
5669

5770
/// Get the mnemonic phrase as a string
58-
pub fn phrase(&self) -> &str {
59-
self.inner.phrase()
71+
pub fn phrase(&self) -> String {
72+
self.inner.to_string()
6073
}
6174

6275
/// Get words as a vector
6376
pub fn words(&self) -> Vec<&str> {
64-
self.inner.phrase().split_whitespace().collect()
77+
self.inner.words().collect()
6578
}
6679

6780
/// Get the number of words in the mnemonic
6881
pub fn word_count(&self) -> usize {
69-
self.words().len()
82+
self.inner.word_count()
7083
}
7184

7285
/// Derive a seed from the mnemonic with optional passphrase
7386
pub fn to_seed(&self, passphrase: &str) -> SeedBytes {
74-
let seed = Seed::new(&self.inner, passphrase);
75-
SeedBytes::new(seed.as_bytes().try_into().expect("Seed should be 64 bytes"))
87+
let seed = self.inner.to_seed(passphrase);
88+
let seed_array: [u8; 64] = seed.try_into()
89+
.expect("BIP39 seed should always be 64 bytes");
90+
SeedBytes::new(seed_array)
7691
}
7792

7893
/// Validate the mnemonic phrase
7994
pub fn validate(phrase: &str) -> bool {
80-
Bip39Mnemonic::from_phrase(phrase, Language::English).is_ok()
95+
Bip39Mnemonic::parse_in_normalized(Language::English, phrase).is_ok()
8196
}
8297
}
8398

@@ -185,7 +200,7 @@ mod tests {
185200
#[test]
186201
fn test_mnemonic_validation() {
187202
let mnemonic = Mnemonic::new();
188-
assert!(Mnemonic::validate(mnemonic.phrase()));
203+
assert!(Mnemonic::validate(&mnemonic.phrase()));
189204
assert!(!Mnemonic::validate("invalid phrase here"));
190205
}
191206

crates/bitcell-wallet/src/transaction.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,10 @@ impl TransactionBuilder {
219219
}
220220

221221
/// Build the transaction
222+
///
223+
/// Note: A zero fee is allowed but may cause transactions to be rejected
224+
/// or remain pending indefinitely on some chains. Consider using
225+
/// FeeEstimator to calculate appropriate fees.
222226
pub fn build(self) -> Result<Transaction> {
223227
let from = self.from.ok_or(Error::TransactionError("Missing sender address".into()))?;
224228
let to = self.to.ok_or(Error::TransactionError("Missing recipient address".into()))?;
@@ -286,11 +290,17 @@ impl FeeEstimator {
286290
}
287291

288292
/// Estimate fee for simple transfer
293+
///
294+
/// Note: These are placeholder values for demonstration purposes.
295+
/// Real implementations should use dynamic fee estimation based on
296+
/// network conditions (e.g., gas price oracles for Ethereum).
297+
/// The Ethereum base fee of 20 Gwei may be outdated depending on
298+
/// network congestion.
289299
pub fn estimate_simple_transfer(&self, chain: Chain) -> u64 {
290300
let base_fee: u64 = match chain {
291301
Chain::BitCell => 100,
292302
Chain::Bitcoin | Chain::BitcoinTestnet => 2000, // ~200 bytes * 10 sat/byte
293-
Chain::Ethereum | Chain::EthereumSepolia => 21_000_u64 * 20_000_000_000_u64, // 21k gas * 20 Gwei
303+
Chain::Ethereum | Chain::EthereumSepolia => 21_000_u64 * 20_000_000_000_u64, // 21k gas * 20 Gwei (placeholder)
294304
Chain::Custom(_) => 100,
295305
};
296306

0 commit comments

Comments
 (0)