Implement core functionality of BitCell wallet#11
Conversation
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Add Copilot Coding Agent config with larger runner
|
@copilot try again |
Pulled latest changes. Runner is configured as |
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
…kages, simplify tar command Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements core wallet functionality for the BitCell blockchain with multi-chain support. It introduces a new bitcell-wallet crate providing modular components for wallet creation, BIP39 mnemonic phrases, hierarchical deterministic address generation, transaction signing, balance tracking, and transaction history management.
Key changes:
- New modular wallet architecture with 6 core modules (wallet, mnemonic, address, transaction, balance, history, chain)
- BIP39 seed phrase support with secure zeroization
- Multi-chain address generation for BitCell, Bitcoin, and Ethereum
- Transaction builder pattern with fee estimation
- Comprehensive test coverage (80+ tests added)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/bitcell-wallet/src/lib.rs | Module definitions and public API exports with error types |
| crates/bitcell-wallet/src/wallet.rs | Core wallet implementation with key derivation, locking, and transaction signing |
| crates/bitcell-wallet/src/mnemonic.rs | BIP39 mnemonic generation and seed derivation with secure memory handling |
| crates/bitcell-wallet/src/address.rs | Multi-chain address generation and validation with format conversion |
| crates/bitcell-wallet/src/transaction.rs | Transaction creation, signing, and fee estimation |
| crates/bitcell-wallet/src/balance.rs | Balance tracking with chain-specific decimal formatting |
| crates/bitcell-wallet/src/history.rs | Transaction history management with filtering and summary statistics |
| crates/bitcell-wallet/src/chain.rs | Chain configuration and properties for multiple blockchains |
| crates/bitcell-wallet/Cargo.toml | Dependencies for cryptography, BIP39, and serialization |
| Cargo.toml | Workspace updates to include new wallet crate |
| .github/copilot-setup-steps.yml | CI configuration for GitHub Copilot |
Critical Issues Found:
- Bitcoin and Ethereum address generation use incorrect cryptographic primitives, producing incompatible addresses
- Non-standard key derivation breaks wallet interoperability with BIP32-compliant wallets
- Multiple chains share the same chain_id, creating replay attack vulnerabilities
- Serialization issues with cached/indexed fields that aren't rebuilt on deserialization
- Several arithmetic overflow risks in balance calculations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hex.workspace = true | ||
|
|
||
| # BIP39/BIP32 seed phrases | ||
| tiny-bip39 = "1.0" |
There was a problem hiding this comment.
The tiny-bip39 crate has been deprecated. Consider using the bip39 crate instead, which is the maintained fork. The tiny-bip39 crate is no longer receiving updates and may have security vulnerabilities.
| tiny-bip39 = "1.0" | |
| bip39 = "2.0" |
| /// Generate an Ethereum address from a public key | ||
| pub fn from_public_key_ethereum(public_key: &PublicKey, testnet: bool, index: u32) -> Self { | ||
| // Ethereum address = last 20 bytes of Keccak256(uncompressed_pubkey[1:]) | ||
| // Simplified: using SHA256 for demonstration | ||
| let pubkey_bytes = public_key.as_bytes(); | ||
| let hash = Sha256::digest(pubkey_bytes); | ||
| let address_bytes = hash[12..].to_vec(); // Last 20 bytes | ||
|
|
||
| let chain = if testnet { Chain::EthereumSepolia } else { Chain::Ethereum }; | ||
| Self::new(address_bytes, AddressType::Ethereum, chain, index) | ||
| } |
There was a problem hiding this comment.
Ethereum address generation uses SHA256 instead of Keccak256, producing addresses incompatible with the Ethereum network. The comment on line 75 acknowledges this but uses SHA256 anyway.
Ethereum addresses must use Keccak256 hash of the uncompressed public key (excluding the 0x04 prefix byte), taking the last 20 bytes. Consider adding a proper Keccak256 dependency (e.g., sha3 crate) for Ethereum compatibility.
| pub fn hash(&self) -> Hash256 { | ||
| let mut data = Vec::new(); | ||
| data.extend_from_slice(&(self.chain.chain_id() as u64).to_le_bytes()); | ||
| data.extend_from_slice(self.from.as_bytes()); | ||
| data.extend_from_slice(self.to.as_bytes()); | ||
| data.extend_from_slice(&self.amount.to_le_bytes()); | ||
| data.extend_from_slice(&self.fee.to_le_bytes()); | ||
| data.extend_from_slice(&self.nonce.to_le_bytes()); | ||
| data.extend_from_slice(&self.data); | ||
| data.extend_from_slice(&self.timestamp.to_le_bytes()); | ||
| Hash256::hash(&data) | ||
| } |
There was a problem hiding this comment.
The transaction hash includes all transaction fields but uses chain_id() which returns inconsistent values. For example, Bitcoin, Ethereum, and BitCell all have chain_id 1, making transactions potentially vulnerable to replay attacks across these chains.
Consider using unique chain identifiers in transaction hashes, or implement chain-specific transaction formats that prevent cross-chain replay.
| /// Get a short version of the tx hash | ||
| pub fn short_hash(&self) -> String { | ||
| if self.tx_hash.len() > 16 { | ||
| format!("{}...{}", &self.tx_hash[..8], &self.tx_hash[self.tx_hash.len()-8..]) | ||
| } else { | ||
| self.tx_hash.clone() | ||
| } | ||
| } |
There was a problem hiding this comment.
The short_hash() method uses direct string slicing which can panic if the hash contains multi-byte UTF-8 characters or if the hash is shorter than expected. While transaction hashes are typically hex strings (ASCII), it's safer to use character boundary-aware slicing or handle potential panics.
Consider using .chars() and .char_indices() for safer string slicing, or document that this method assumes ASCII hex strings.
| pub config: WalletConfig, | ||
| pub addresses: AddressManager, | ||
| pub balances: BalanceTracker, | ||
| pub history: TransactionHistory, | ||
| pub nonces: HashMap<String, u64>, | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The WalletExport struct includes public fields that expose internal wallet data structures (AddressManager, BalanceTracker, etc.). While this provides flexibility, it bypasses encapsulation and could lead to data inconsistencies if these fields are modified directly after import.
Consider making these fields private and providing controlled access methods, or documenting that direct modification of these fields after import may lead to undefined behavior.
| pub config: WalletConfig, | |
| pub addresses: AddressManager, | |
| pub balances: BalanceTracker, | |
| pub history: TransactionHistory, | |
| pub nonces: HashMap<String, u64>, | |
| } | |
| config: WalletConfig, | |
| addresses: AddressManager, | |
| balances: BalanceTracker, | |
| history: TransactionHistory, | |
| nonces: HashMap<String, u64>, | |
| } | |
| impl WalletExport { | |
| pub fn config(&self) -> &WalletConfig { | |
| &self.config | |
| } | |
| pub fn addresses(&self) -> &AddressManager { | |
| &self.addresses | |
| } | |
| pub fn balances(&self) -> &BalanceTracker { | |
| &self.balances | |
| } | |
| pub fn history(&self) -> &TransactionHistory { | |
| &self.history | |
| } | |
| pub fn nonces(&self) -> &HashMap<String, u64> { | |
| &self.nonces | |
| } | |
| } |
| /// Transaction history manager | ||
| #[derive(Debug, Clone, Default, Serialize, Deserialize)] | ||
| pub struct TransactionHistory { | ||
| /// All transactions | ||
| transactions: Vec<TransactionRecord>, | ||
| /// Index by tx hash for quick lookup | ||
| #[serde(skip)] | ||
| hash_index: HashMap<String, usize>, | ||
| } |
There was a problem hiding this comment.
The hash_index field is skipped during serialization but not rebuilt during deserialization. This means after deserializing a TransactionHistory, lookups via get() and get_mut() will fail because the index is empty, even though the transactions are present.
Add a custom deserialization implementation or call rebuild_index() in a post-deserialization step to ensure the hash index is populated.
| /// Derive a key at a specific path | ||
| /// | ||
| /// Note: This uses a simplified key derivation scheme for the initial implementation. | ||
| /// For full BIP32 compatibility with external wallets, implement proper HMAC-SHA512 | ||
| /// based hierarchical deterministic key derivation. The current implementation | ||
| /// provides deterministic key generation that is secure but may not be compatible | ||
| /// with other BIP32-compliant wallets. | ||
| fn derive_key(&mut self, path: &DerivationPath) -> Result<&DerivedKey> { | ||
| let path_str = path.to_string(); | ||
|
|
||
| if self.derived_keys.contains_key(&path_str) { | ||
| return Ok(&self.derived_keys[&path_str]); | ||
| } | ||
|
|
||
| let seed = self.master_seed.as_ref().ok_or(Error::WalletLocked)?; | ||
|
|
||
| // Simplified key derivation using HMAC-like construction | ||
| // For full BIP32 compatibility, use a proper BIP32 library | ||
| let mut derivation_data = Vec::new(); | ||
| derivation_data.extend_from_slice(seed.as_bytes()); | ||
| derivation_data.extend_from_slice(path_str.as_bytes()); | ||
|
|
||
| let derived_hash = Hash256::hash(&derivation_data); | ||
| let secret_key = SecretKey::from_bytes(derived_hash.as_bytes())?; | ||
|
|
||
| let derived_key = DerivedKey::new(secret_key, path.clone()); | ||
| self.derived_keys.insert(path_str.clone(), derived_key); | ||
|
|
||
| Ok(&self.derived_keys[&path_str]) | ||
| } |
There was a problem hiding this comment.
The key derivation implementation is simplified and not BIP32-compliant. While the documentation notes this, the current implementation may produce keys that differ from standard HD wallets. This means:
- Users cannot import this wallet's mnemonic into other BIP32-compliant wallets
- Users cannot use mnemonics from other wallets in this implementation
- The derivation path format (m/44'/.../) is misleading since BIP32 isn't actually used
Consider implementing proper BIP32 HMAC-SHA512 hierarchical deterministic key derivation for wallet interoperability, or rename the derivation path format to indicate this is a custom scheme.
| /// Get the chain ID | ||
| pub fn chain_id(&self) -> u32 { | ||
| match self { | ||
| Chain::BitCell => 1, | ||
| Chain::Bitcoin => 0, | ||
| Chain::BitcoinTestnet => 1, | ||
| Chain::Ethereum => 1, | ||
| Chain::EthereumSepolia => 11155111, | ||
| Chain::Custom(id) => *id, | ||
| } | ||
| } |
There was a problem hiding this comment.
Multiple chains have the same chain_id value of 1 (BitCell, Bitcoin, and Ethereum mainnet). This creates ambiguity and potential security issues. Each chain should have a unique chain_id to prevent confusion and replay attacks.
For reference, Ethereum mainnet uses chain_id 1, but BitCell and Bitcoin should use distinct values. Bitcoin doesn't traditionally use chain IDs in the same way, but for this multi-chain wallet, unique identifiers are essential.
| /// Format date for display | ||
| pub fn format_date(&self) -> String { | ||
| // Simple date formatting (YYYY-MM-DD HH:MM) | ||
| use std::time::{Duration, UNIX_EPOCH}; | ||
| let datetime = UNIX_EPOCH + Duration::from_secs(self.timestamp); | ||
| format!("{:?}", datetime) // Simplified; in production use chrono | ||
| } |
There was a problem hiding this comment.
The format_date function uses a simplified Debug formatting which will produce a non-user-friendly output like "SystemTime { tv_sec: ..., tv_nsec: ... }". This is not suitable for display purposes.
Consider using a proper datetime library like chrono or formatting the timestamp manually into a human-readable format (YYYY-MM-DD HH:MM:SS).
|
|
||
| // Scale fraction to desired decimal places | ||
| let scale = 10u64.pow(decimal_places as u32); | ||
| let scaled_fraction = (fraction * scale) / divisor; |
There was a problem hiding this comment.
The calculation (fraction * scale) / divisor can overflow when fraction is large and scale is also large. For example, with Ethereum's 18 decimals, fraction could be up to 10^18 - 1, and multiplying by scale could exceed u64::MAX.
Consider using checked arithmetic or u128 for intermediate calculations to prevent overflow.
| let scaled_fraction = (fraction * scale) / divisor; | |
| let scaled_fraction = ((fraction as u128 * scale as u128) / divisor as u128) as u64; |
…test cases Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
…ac-linux Add cross-platform release workflow for Win/Mac/Linux builds
Add BDD-style integration tests for BitCell Wallet
|
@copilot apply changes based on the comments in this thread |
…on, and safety improvements Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Applied all review comments in commit
|
…r unused variable Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Explore repository structure and understand codebase
Build project and run existing tests (148 tests pass)
Create
bitcell-walletcrate with modular Rust components:Initial blockchain support:
Design modular, maintainable architecture with minimal memory footprint
Add comprehensive tests for wallet functionality (80 tests)
Run full test suite to verify no regressions (246 total tests pass)
Fix code review issues:
Fix clippy warnings:
#[default]attribute for enum defaultsclamp()instead ofmax().min()Displaytrait instead of inherentto_string()Run code review and security checks
Address Copilot code review comments:
tiny-bip39withbip39v2.0format_fixed()using u128rebuild_totals()method for BalanceTracker deserializationshort_hash()to use char-safe slicingensure_indexed()for TransactionHistory deserializationformat_date()with proper date formattingFixes Implement Core Functionality of BitCell Wallet #7
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.