Implement real transaction system for wallet GUI and RPC#87
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>
There was a problem hiding this comment.
Pull request overview
This PR implements a complete real transaction system for the BitCell wallet GUI and RPC layer, replacing mock transaction messages with cryptographic signing and blockchain broadcasting. The implementation follows the separation of concerns pattern by introducing signing_hash() for signature creation (excluding the signature field) while maintaining hash() for transaction identification (including the signature field), preventing circular dependencies in transaction signing.
Key Changes:
- Added
Transaction::signing_hash()method that excludes signature field for proper cryptographic signing - Updated RPC validation to use
signing_hash()instead ofhash()for signature verification - Implemented complete transaction flow in wallet GUI: nonce fetching, transaction creation, signing, serialization, and broadcasting
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
crates/bitcell-consensus/src/block.rs |
Added signing_hash() method to compute transaction hash excluding signature field, plus comprehensive unit tests validating the signing flow and signature exclusion |
crates/bitcell-node/src/rpc.rs |
Updated eth_sendRawTransaction to verify signatures using signing_hash() instead of hash(), ensuring correct signature validation |
crates/bitcell-wallet/src/wallet.rs |
Added get_secret_key() method to expose signing keys when wallet is unlocked, enabling external transaction signing |
crates/bitcell-wallet-gui/src/main.rs |
Replaced mock transaction display with complete real transaction flow: nonce fetching, address parsing, transaction creation, cryptographic signing, and RPC broadcasting |
crates/bitcell-wallet-gui/Cargo.toml |
Added dependencies for bitcell-consensus (transaction structs) and bincode (serialization) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn parse_public_key(address: &str) -> Result<PublicKey, String> { | ||
| let hex_str = address.strip_prefix("0x").unwrap_or(address); | ||
| let bytes = hex::decode(hex_str) | ||
| .map_err(|e| format!("Invalid hex: {}", e))?; | ||
|
|
||
| if bytes.len() != 33 { | ||
| return Err(format!("Invalid public key length: expected 33 bytes, got {}", bytes.len())); | ||
| } | ||
|
|
||
| let mut pk_bytes = [0u8; 33]; | ||
| pk_bytes.copy_from_slice(&bytes); | ||
|
|
||
| PublicKey::from_bytes(pk_bytes) | ||
| .map_err(|e| format!("Invalid public key: {}", e)) | ||
| } |
There was a problem hiding this comment.
[nitpick] The parse_public_key function could benefit from additional validation. While it checks for the correct byte length (33 bytes), it doesn't validate that the first byte is a valid compressed public key prefix (0x02 or 0x03 for secp256k1). Although PublicKey::from_bytes() will catch invalid keys, adding an early check would provide a clearer error message.
Consider adding:
if bytes[0] != 0x02 && bytes[0] != 0x03 {
return Err(format!("Invalid public key prefix: expected 0x02 or 0x03, got 0x{:02x}", bytes[0]));
}| // Create transaction with placeholder signature (will be replaced after signing) | ||
| let placeholder_sig = bitcell_crypto::Signature::from_bytes(PLACEHOLDER_SIGNATURE); | ||
|
|
||
| let mut tx = bitcell_consensus::Transaction { | ||
| nonce, | ||
| from: from_pk, | ||
| to: to_pk, | ||
| amount: amount_units, | ||
| gas_limit, | ||
| gas_price, | ||
| data: Vec::new(), | ||
| signature: placeholder_sig, | ||
| }; | ||
|
|
||
| // Compute signing hash and sign | ||
| let signing_hash = tx.signing_hash(); | ||
| let signature = secret_key.sign(signing_hash.as_bytes()); | ||
| tx.signature = signature; |
There was a problem hiding this comment.
Consider adding integration tests for the complete transaction flow: creating, signing, and broadcasting transactions via the GUI. While unit tests exist for the consensus layer's signing_hash() method, there's no test coverage for the wallet GUI's transaction creation and signing logic (lines 605-622). This would help catch regressions in the transaction creation flow.
Example test areas to cover:
- Transaction creation with valid inputs
- Handling of invalid addresses
- Wallet locked/unlocked states
- Signature verification after signing
| let state_clone = state.clone(); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
[nitpick] Consider adding client-side balance validation before sending the transaction. Currently, balance validation only happens in the RPC layer (server-side), which means users won't know if they have insufficient funds until after the transaction is signed and sent. Adding a balance check here would improve UX by providing immediate feedback.
Example:
// After getting the wallet and before the async spawn
let balance = wallet.get_balance(&wallet_addr)?;
let total_cost = amount_units + (gas_limit * gas_price);
if balance < total_cost {
wallet_state.set_status_message(format!(
"Insufficient balance: {} CELL available, {} CELL required",
balance as f64 / 100_000_000.0,
total_cost as f64 / 100_000_000.0
).into());
return;
}| # Serialization | ||
| serde.workspace = true | ||
| serde_json = "1.0" | ||
| bincode = "1.3" |
There was a problem hiding this comment.
[nitpick] Consider using bincode.workspace = true instead of specifying the version directly. The workspace already defines bincode = "1.3" in Cargo.toml, and using the workspace dependency ensures version consistency across the codebase. This follows the pattern used by other crates like bitcell-consensus and bitcell-state.
| bincode = "1.3" | |
| bincode.workspace = true |
| /// Compute transaction hash (includes signature for uniqueness) | ||
| pub fn hash(&self) -> Hash256 { | ||
| // Note: bincode serialization to Vec cannot fail for this structure | ||
| let serialized = bincode::serialize(self).expect("transaction serialization should never fail"); | ||
| Hash256::hash(&serialized) | ||
| } |
There was a problem hiding this comment.
[nitpick] The comment says "includes signature for uniqueness" which is accurate. However, consider expanding the documentation to clarify the distinction between hash() and signing_hash():
/// Compute transaction hash (includes signature for uniqueness)
///
/// This hash uniquely identifies a transaction and is used for:
/// - Transaction pool deduplication
/// - Block transaction merkle trees
/// - Transaction receipts and lookups
///
/// **Note**: Use `signing_hash()` for signature creation/verification,
/// not this method, as it includes the signature field itself.
This helps prevent future developers from accidentally using the wrong hash method.
| Fee: {} CELL", | ||
| tx_hash, | ||
| amount_units as f64 / 100_000_000.0, | ||
| (gas_limit * gas_price) as f64 / 100_000_000.0 |
There was a problem hiding this comment.
Potential overflow in fee calculation. The expression (gas_limit * gas_price) could overflow if both values are large. Consider using checked_mul() or saturating_mul() to handle potential overflow safely before converting to f64.
Example fix:
gas_limit.saturating_mul(gas_price) as f64 / 100_000_000.0| (gas_limit * gas_price) as f64 / 100_000_000.0 | |
| (gas_limit.saturating_mul(gas_price)) as f64 / 100_000_000.0 |
| } | ||
|
|
||
| /// Get the secret key for an address | ||
| /// |
There was a problem hiding this comment.
The documentation states this method "allows signing transactions outside the wallet's transaction system." While this is necessary for the GUI implementation, consider adding a warning in the documentation about security implications of exposing secret keys directly. For example:
/// Get the secret key for an address
///
/// **Security Warning**: This exposes raw secret key material. Use with caution
/// and ensure the key is properly zeroized after use. Prefer using the wallet's
/// built-in `sign_transaction()` method when possible.
///
/// This allows signing transactions outside the wallet's transaction system.
/// The wallet must be unlocked to access secret keys.
| /// | |
| /// | |
| /// **Security Warning**: This exposes raw secret key material. Use with caution | |
| /// and ensure the key is properly zeroized after use. Prefer using the wallet's | |
| /// built-in `sign_transaction()` method when possible. | |
| /// |
|
[autopilot] @copilot this PR has merge conflicts. Please resolve them by merging the base branch and fixing conflicts, then push the updates. |
- Resolved conflicts in transaction signing implementation - Adopted better documentation from master for signing_hash methods - Used master's parse_address_to_pubkey with BC1 prefix support - Used master's get_secret_key_for_address with immutable self - Master branch already had complete transaction implementation in GUI
|
[autopilot] 🤖 LLM Analysis of Copilot Review Decision: approve |
✅ Merge Conflicts Resolved
Successfully merged origin/master and resolved all conflicts by adopting the better implementations from master branch.
Conflict Resolutions
1. Transaction Signing Documentation (
bitcell-consensus/src/block.rs)signing_hash()methodhash()andsigning_hash()2. RPC Signature Validation (
bitcell-node/src/rpc.rs)3. Wallet Secret Key Method (
bitcell-wallet/src/wallet.rs)get_secret_key_for_address(&self)&selfinstead of&mut self(better design)4. Wallet GUI (
bitcell-wallet-gui/src/main.rs)parse_address_to_pubkey()with BC1 prefix supportget_secret_key_for_address()methodKey Improvements from Master
&selfwhere possibleChanges Summary
Status
The PR can now proceed with integration testing and review.
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.