-
Notifications
You must be signed in to change notification settings - Fork 40
Add Bitcoin WalletConnect support #849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
DRadmir
commented
Dec 18, 2025
- Add BIP122 namespace for Bitcoin chain identification
- Add sendTransfer and signMessage methods to WalletConnectionMethods
- Create gem_bitcoin signer module with BIP137 message signing
- Add BitcoinRequestHandler for WalletConnect request parsing
- Add BitcoinPersonal sign type and Bitcoin transaction type
Changed Files
|
Summary of ChangesHello @DRadmir, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands WalletConnect capabilities by adding full support for Bitcoin. It establishes the necessary infrastructure to allow users to securely sign messages and send transactions on the Bitcoin network through WalletConnect, enhancing interoperability and user experience for Bitcoin holders. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces comprehensive Bitcoin support for WalletConnect, enabling personal message signing and transfer functionality. Key changes include adding hex, serde_json, sha2, and signer dependencies, and integrating a new gem_bitcoin signer module with a signer feature. The gem_bitcoin crate now includes sign_personal for BIP137-compliant signatures and defines BitcoinSignMessageData and BitcoinSignDataResponse for message handling. WalletConnect primitives were updated to recognize Bitcoin via Bip122 CAIP2, and new BtcSendTransfer and BtcSignMessage methods were added. The gemstone crate was configured to support Bitcoin in WalletConnect, including a new BitcoinPersonal sign digest type and a dedicated BitcoinRequestHandler for parsing WalletConnect requests. Review comments highlighted the need to improve error handling in BitcoinSignMessageData::to_bytes by returning a Result instead of using unwrap_or_default(), making the address parameter mandatory in parse_sign_message, and updating corresponding call sites and test cases to reflect these error propagation changes.
| pub fn to_bytes(&self) -> Vec<u8> { | ||
| serde_json::to_vec(self).unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The to_bytes method uses unwrap_or_default(), which can hide serialization errors by returning an empty vector. This can lead to obscure bugs in downstream code that consumes this output. It's better to propagate the error by returning a Result, which also makes the method's signature symmetric with from_bytes.
| pub fn to_bytes(&self) -> Vec<u8> { | |
| serde_json::to_vec(self).unwrap_or_default() | |
| } | |
| pub fn to_bytes(&self) -> Result<Vec<u8>, SignerError> { | |
| serde_json::to_vec(self).map_err(SignerError::from) | |
| } |
| #[test] | ||
| fn test_sign_bitcoin_personal() { | ||
| let btc_data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()); | ||
| let data = btc_data.to_bytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[test] | ||
| fn test_sign_bitcoin_personal_rejects_invalid_key() { | ||
| let btc_data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()); | ||
| let data = btc_data.to_bytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gemstone/src/message/signer.rs
Outdated
| SignDigestType::BitcoinPersonal => { | ||
| let message = BitcoinSignMessageData::from_bytes(&self.message.data).unwrap_or_default(); | ||
| message.message | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unwrap_or_default() here can hide deserialization errors from from_bytes. If an error occurs, an empty string will be returned for the preview without any indication of what went wrong. It's better to handle the Result explicitly to avoid providing a misleading or empty preview.
| SignDigestType::BitcoinPersonal => { | |
| let message = BitcoinSignMessageData::from_bytes(&self.message.data).unwrap_or_default(); | |
| message.message | |
| } | |
| SignDigestType::BitcoinPersonal => { | |
| BitcoinSignMessageData::from_bytes(&self.message.data) | |
| .map(|m| m.message) | |
| .unwrap_or_default() | |
| } |
| .unwrap_or_default(); | ||
|
|
||
| let btc_data = BitcoinSignMessageData::new(message.to_string(), address.to_string()); | ||
| let data = String::from_utf8(btc_data.to_bytes()).map_err(|e| format!("Failed to encode BitcoinSignMessageData: {}", e))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To accommodate the recommended change of to_bytes returning a Result, this line needs to be updated to handle the potential serialization error before converting the bytes to a UTF-8 string.
| let data = String::from_utf8(btc_data.to_bytes()).map_err(|e| format!("Failed to encode BitcoinSignMessageData: {}", e))?; | |
| let data = String::from_utf8(btc_data.to_bytes().map_err(|e| e.to_string())?).map_err(|e| e.to_string())?; |
- Add BIP122 namespace for Bitcoin chain identification - Add sendTransfer and signMessage methods to WalletConnectionMethods - Create gem_bitcoin signer module with BIP137 message signing - Add BitcoinRequestHandler for WalletConnect request parsing - Add BitcoinPersonal sign type and Bitcoin transaction type
960fa92 to
cfb1a4a
Compare
gemcoder21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xh3rman can you review and run a small audit?
gemstone/src/message/signer.rs
Outdated
| Ok(self.get_result(&signed)) | ||
| } | ||
| SignDigestType::BitcoinPersonal => { | ||
| let response = bitcoin_sign_personal(&self.message.data, &private_key)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do one liner
gemstone/src/message/signer.rs
Outdated
| Ok(decoded) | ||
| } | ||
| SignDigestType::BitcoinPersonal => { | ||
| let message = BitcoinSignMessageData::from_bytes(&self.message.data)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do one liner
| } | ||
|
|
||
| fn parse_send_transaction(chain: Chain, params: Value) -> Result<WalletConnectAction, String> { | ||
| params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add model to decode recipientAddress and amount ? easier to read then
gemstone/Cargo.toml
Outdated
| bs58 = { workspace = true } | ||
| url = { workspace = true } | ||
| zeroize = "1.8" | ||
| sha2 = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep sha2 it out of gemstone?
gemstone/src/message/signer.rs
Outdated
| _ => "".to_string(), | ||
| }, | ||
| SignDigestType::BitcoinPersonal => { | ||
| let message = BitcoinSignMessageData::from_bytes(&self.message.data).unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap_or_default no unwrap here, throw an error if not able to decode
| fn test_response_to_json() { | ||
| let response = BitcoinSignDataResponse::new("bc1qtest".to_string(), "27abcdef".to_string()); | ||
|
|
||
| let json = response.to_json().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do 1 iner
| let data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()); | ||
| let hash = data.hash(); | ||
|
|
||
| assert_eq!(hash.len(), 32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check for hash.len() if you check for hex
| let private_key = hex::decode("1e9d38b5274152a78dff1a86fa464ceadc1f4238ca2c17060c3c507349424a34").expect("valid hex"); | ||
|
|
||
| let response = sign_personal(&data, &private_key).expect("signing succeeds"); | ||
| let json = response.to_json().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do 1 liner
| let btc_data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()); | ||
| let data = btc_data.to_bytes(); | ||
|
|
||
| let private_key = hex::decode("1e9d38b5274152a78dff1a86fa464ceadc1f4238ca2c17060c3c507349424a34").expect("valid hex"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let private_key = hex::decode("1e9d38b5274152a78dff1a86fa464ceadc1f4238ca2c17060c3c507349424a34").expect("valid hex"); | |
| let private_key = hex::decode("1e9d38b5274152a78dff1a86fa464ceadc1f4238ca2c17060c3c507349424a34").unwrap(); |
|
|
||
| #[test] | ||
| fn test_sign_bitcoin_personal() { | ||
| let btc_data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just do let let data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()).to_bytes()
| } | ||
| } | ||
|
|
||
| fn encode_varint(n: usize) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method can be moved to an encoding.rs can be reused later
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default(); | ||
|
|
||
| let btc_data = BitcoinSignMessageData::new(message.to_string(), address.to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we can use serde to decode and encode data? Value js serde_json::Value and String::from_utf8(btc_data.to_bytes()) can be simplified
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_encode_varint_small() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge into one test_encode_varint
| fn test_sign_bitcoin_personal() { | ||
| let data = BitcoinSignMessageData::new("Hello Bitcoin".to_string(), "bc1qtest".to_string()).to_bytes(); | ||
| let private_key = hex::decode("1e9d38b5274152a78dff1a86fa464ceadc1f4238ca2c17060c3c507349424a34").unwrap(); | ||
| let parsed: serde_json::Value = serde_json::from_str(&sign_personal(&data, &private_key).unwrap().to_json().unwrap()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let data = sign_personal(&data, &private_key).unwrap()