Add TON WalletConnect support #826
Conversation
|
Review changes with 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 the platform's blockchain interoperability by integrating TON (The Open Network) with WalletConnect. The changes enable seamless communication between decentralized applications on TON and compatible wallets, allowing users to sign messages and initiate transactions directly through WalletConnect. This broadens the ecosystem's reach and enhances user experience for TON users. Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for TON blockchain to WalletConnect. The changes are comprehensive, touching upon primitives to define TON-specific types, and the gemstone crate to handle request/response logic and message decoding. The implementation is mostly solid with good test coverage for the new functionality. I have two main points of feedback. First, a medium-severity suggestion to improve a data structure for better consistency and usability. Second, a high-severity issue in the TON message decoding logic, where silent failures could lead to users signing unintended data. Addressing this is important for security and correctness.
gemstone/src/wallet_connect/mod.rs
Outdated
| fn decode_ton_sign_message(&self, data: String) -> SignMessage { | ||
| if let Ok(json) = serde_json::from_str::<serde_json::Value>(&data) { | ||
| let payload_type = json.get("type").and_then(|v| v.as_str()).unwrap_or("text"); | ||
|
|
||
| let message_data = match payload_type { | ||
| "text" => json.get("text").and_then(|v| v.as_str()).unwrap_or_default().as_bytes().to_vec(), | ||
| "binary" => { | ||
| let binary_data = json.get("data").and_then(|v| v.as_str()).unwrap_or_default(); | ||
| base64::engine::general_purpose::STANDARD.decode(binary_data).unwrap_or_default() | ||
| } | ||
| _ => data.as_bytes().to_vec(), | ||
| }; | ||
|
|
||
| return SignMessage { | ||
| chain: Chain::Ton, | ||
| sign_type: SignDigestType::TonPersonal, | ||
| data: message_data, | ||
| }; | ||
| } | ||
|
|
||
| SignMessage { | ||
| chain: Chain::Ton, | ||
| sign_type: SignDigestType::TonPersonal, | ||
| data: data.as_bytes().to_vec(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of decode_ton_sign_message has several silent failure modes that could lead to users signing something they did not intend to:
- If base64 decoding fails, it defaults to an empty byte array.
- If an unknown
typeis provided in the JSON, it defaults to signing the entire JSON string. - If the
textfield is missing fortype: "text", it signs an empty string.
These silent failures are risky. It would be safer to fall back to a consistent behavior, like signing the original raw data string, if any part of the structured parsing fails. This ensures that the user is at least signing the data they provided, even if it's not in the expected format.
Here is a suggested refactoring that handles these cases more robustly by falling back to signing the raw data string in case of parsing errors, which is safer than signing empty or incorrect data.
fn decode_ton_sign_message(&self, data: String) -> SignMessage {
if let Ok(json) = serde_json::from_str::<serde_json::Value>(&data) {
let message_data = match json.get("type").and_then(|v| v.as_str()) {
Some("text") => json.get("text").and_then(|v| v.as_str()).map(|s| s.as_bytes().to_vec()),
Some("binary") => json.get("data").and_then(|v| v.as_str()).and_then(|b64| base64::engine::general_purpose::STANDARD.decode(b64).ok()),
_ => None,
};
if let Some(message_data) = message_data {
return SignMessage {
chain: Chain::Ton,
sign_type: SignDigestType::TonPersonal,
data: message_data,
};
}
}
SignMessage {
chain: Chain::Ton,
sign_type: SignDigestType::TonPersonal,
data: data.as_bytes().to_vec(),
}
}| pub data: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize)] |
There was a problem hiding this comment.
For better usability and consistency with other data structures in the crate, consider deriving Clone and PartialEq for WCTonMessage. This will make it easier to use in tests and other logic where copying or comparison is needed.
| #[derive(Debug, Serialize, Deserialize)] | |
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] |
DRadmir
force-pushed
the
ton-walletconnect
branch
from
1c42dd4 to
1cb77e1
Compare
| Cell, | ||
| } | ||
|
|
||
| impl TonSignDataType { |
There was a problem hiding this comment.
use strum as AsRefStr EnumString and also replace "text" in other places with using this type instead
crates/signer/src/ton.rs
Outdated
|
|
||
| pub fn sign_ton_personal_message(input: TonSignDataInput, private_key: Vec<u8>) -> Result<String, SignerError> { | ||
| let private_key = Zeroizing::new(private_key); | ||
| let payload_str = std::str::from_utf8(&input.payload).map_err(|e| SignerError::new(e.to_string()))?; |
There was a problem hiding this comment.
should not be signing here
There was a problem hiding this comment.
should be done on app level
gemstone/src/message/decoder.rs
Outdated
| let Some(payload_type) = json.get("type").and_then(|v| v.as_str()) else { | ||
| return Ok(MessagePreview::Text(string)); | ||
| }; | ||
| let preview = match payload_type { |
DRadmir
force-pushed
the
ton-walletconnect
branch
from
875996a to
a93d4f2
Compare
| ChainType::Ton => Some(WalletConnectCAIP2::Ton.as_ref().to_string()), | ||
| ChainType::Bitcoin | ||
| | ChainType::Ton | ||
| | ChainType::Tron |
There was a problem hiding this comment.
what's the definition of -239 ?
No description provided.