add Solana network support for legacy and versioned transactions #108
Conversation
WalkthroughThe changes update the support status for the Solana (SOL) blockchain across documentation and code. The README now marks SOL as fully supported rather than under development. In the core chain module, new error variants and corresponding error code assignments are added, with SOL’s support flag updated to true. The SOL-specific module now implements transaction signing and message signing functions, including comprehensive tests, while a new models file introduces data structures and encoding/decoding methods for Solana transactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SOL as SOL Chain Handler
participant Validator as Transaction Validator
participant Signer as Signature Service
Client->>SOL: Call sign_tx(private_key, transaction)
SOL->>SOL: Decode raw transaction data
SOL->>Validator: Validate header, account keys, & blockhash
Validator-->>SOL: Return validation result
SOL->>SOL: Encode transaction message
SOL->>Signer: Invoke sign_raw(encoded message, private_key)
Signer-->>SOL: Return signature
SOL->>SOL: Verify signature length & update transaction details
SOL-->>Client: Return signed transaction
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (10)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/kos/src/chains/sol/mod.rs (1)
57-87: Cautious blockhash validity checks.Checking for all-zero and all-one blockhash arrays helps detect invalid or placeholder values. Consider clarifying in comments whether all-ones is truly invalid or simply a test scenario. Otherwise, no functional issues noted.
You could add a brief comment explaining why all-ones is treated as invalid, for maintainability:
if sol_tx.message.recent_blockhash.iter().all(|&x| x == 0) || sol_tx.message.recent_blockhash.iter().all(|&x| x == 1) { + // Zero or 0xFF... blockhash is treated as invalid or placeholder return Err(ChainError::InvalidBlockhash); }🧰 Tools
🪛 GitHub Actions: KOS-RS Checks
[error] 86-86: Function sign_message has incorrect formatting.
packages/kos/src/chains/sol/models.rs (2)
260-268: Consider using a standard varint or existing crate utility.
While your customencode_varintlogic appears correct, it duplicates common functionality provided by well-known libraries. If code size or external dependencies are not a concern, an established varint/Protobuf library can reduce maintenance overhead.🧰 Tools
🪛 GitHub Actions: KOS-RS Checks
[error] 266-266: Buffer push operation has incorrect formatting.
1-42: Unit test coverage recommendation.
The introduced structs (MessageHeader,CompiledInstruction,MessageAddressTableLookup,Message, andSolanaTransaction) are central to Solana transaction handling. Comprehensive tests can uncover corner cases (e.g., malformed input, minimal account sets, empty instructions). Consider adding or extending test suites to validate decoding/encoding thoroughly.Would you like help drafting some unit tests for these structs and methods?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)packages/kos/src/chains/mod.rs(4 hunks)packages/kos/src/chains/sol/mod.rs(4 hunks)packages/kos/src/chains/sol/models.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: KOS-RS Checks
packages/kos/src/chains/mod.rs
[error] 64-64: InvalidTransactionHeader, InvalidAccountLength, InvalidBlockhash, InvalidSignatureLength are not formatted correctly.
[error] 138-138: ChainError::DecodeHash formatting issue.
[error] 247-247: ChainError::InvalidTransactionHeader formatting issue.
packages/kos/src/chains/sol/models.rs
[error] 117-117: Variable compiled_address_lookup_tables has incorrect formatting.
[error] 145-145: Push operation for compiled_address_lookup_tables has incorrect formatting.
[error] 266-266: Buffer push operation has incorrect formatting.
packages/kos/src/chains/sol/mod.rs
[error] 54-54: Function sign_tx has incorrect formatting.
[error] 86-86: Function sign_message has incorrect formatting.
🔇 Additional comments (6)
README.md (1)
49-49: Great update on supported networks.This change correctly promotes Solana from “Coming soon” to fully supported. No issues found here.
packages/kos/src/chains/mod.rs (1)
565-565: Validation of “supported: true” for Solana.Solana is marked as supported. This aligns with the README change and the new transaction signing logic in
sol/mod.rs. No issues here.packages/kos/src/chains/sol/mod.rs (3)
29-29: Fixed decimals value.Returning
9for Solana’s decimals is accurate (1 SOL = 10^9 lamports). This seems correct and consistent with common practice.
127-153: Comprehensive test transaction.The test fixture ensures coverage of common Solana transaction fields (header, account keys, etc.). This is a good move toward robust testing. No concerns here.
188-215: Consistent signing results.Two identical raw transactions yield the same signature and hash, ensuring deterministic signing. This test is valuable for highlighting potential race or concurrency errors. The logic looks solid.
packages/kos/src/chains/sol/models.rs (1)
120-120:❓ Verification inconclusive
Fix pipeline formatting violations.
The pipeline reports formatting errors with the variablecompiled_address_lookup_tablesand certainpushoperations. These lines might contain spacing or style problems.Try running the following script to automatically apply Rust’s standard formatting; confirm that the pipeline passes afterward:
Also applies to: 148-148, 266-266
🏁 Script executed:
#!/bin/bash cargo fmt --all cargo clippyLength of output: 224
Pipeline Formatting Issue: Manual Verification Required
The automated commands to run Rust’s formatting (i.e.,cargo fmt --allandcargo clippy) failed due to the absence of the Rust toolchain in the current environment. This prevents us from obtaining reliable output regarding the reported style issues. Please run these commands in your local development environment to verify and automatically correct the formatting violations—particularly with the declaration ofcompiled_address_lookup_tablesinpackages/kos/src/chains/sol/models.rs(line 120) as well as the associatedpushoperations at lines 148 and 266.Once you confirm that the formatting passes locally, update or reformat the code as needed to ensure it complies with Rust's formatting standards.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
packages/kos/src/chains/sol/models.rs (3)
107-109:⚠️ Potential issueAdd bounds checking before accessing instruction data.
The code copies instruction data without verifying that there are enough bytes available in the input.
+ if position + data_len > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for instruction data")); + } let mut data = vec![0u8; data_len]; data.copy_from_slice(&input[position..position + data_len]); position += data_len;
125-127:⚠️ Potential issueAdd bounds checking before accessing address lookup table public key.
The code reads the address lookup table public key without verifying that there are enough bytes available.
+ if position + 32 > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for address lookup table public key")); + } let mut address_lookup_table_pubkey: Vec<u8> = vec![0u8; 32]; address_lookup_table_pubkey.copy_from_slice(&input[position..position + 32]); position += 32;
221-222:⚠️ Potential issueAdd bounds checking before accessing input.
The code accesses
input[position]without first checking that the input slice has at least one element. This could lead to a panic if an empty buffer is provided.+ if input.is_empty() { + return Err(prost::DecodeError::new("Input buffer is empty")); + } + let num_signatures = input[position] as usize; position += 1;
🧹 Nitpick comments (10)
packages/kos/src/chains/sol/models.rs (10)
119-119: Fix inconsistent formatting with whitespace.The declaration of
compiled_address_lookup_tableshas inconsistent whitespace formatting before the type annotation colon.- let mut compiled_address_lookup_tables : Vec<MessageAddressTableLookup> = vec![]; + let mut compiled_address_lookup_tables: Vec<MessageAddressTableLookup> = vec![];
183-184: Check for overflow when converting to u8.The code assumes that the number of account keys is less than 256, but this isn't enforced or checked. If there are more than 255 account keys, this could silently lead to incorrect data being encoded.
- output.push(self.account_keys.len() as u8); + if self.account_keys.len() > 255 { + // Handle this case appropriately, perhaps by truncating or returning an error + output.push(255); + } else { + output.push(self.account_keys.len() as u8); + }
192-193: Check for overflow when converting to u8.Similar to the account keys, the code assumes that the number of instructions is less than 256, which should be verified.
- output.push(self.instructions.len() as u8); + if self.instructions.len() > 255 { + // Handle this case appropriately + output.push(255); + } else { + output.push(self.instructions.len() as u8); + }
203-204: Check for overflow when converting to u8.Another instance where the length is assumed to be less than 256 without verification.
- output.push(self.address_table_lookups.len() as u8); + if self.address_table_lookups.len() > 255 { + // Handle this case appropriately + output.push(255); + } else { + output.push(self.address_table_lookups.len() as u8); + }
241-241: Avoid potential overflow in encode_varint call.There's a potential, though unlikely, issue if
self.signatures.len()is extremely large. The function assumes the conversion to u64 is safe, which it typically is, but explicit handling would be more robust.- let signature_count = encode_varint(self.signatures.len() as u64); + // Safe conversion as Vec lengths can't reach u64::MAX in practice + let signature_count = encode_varint(self.signatures.len() as u64);
171-171: Simplify string length check and string comparison.The current check
self.version.len() > 0 && self.version != String::from(MESSAGE_VERSION_LEGACY)is unnecessarily complex. We can simplify it by directly comparing with the constant.- if self.version.len() > 0 && self.version != String::from(MESSAGE_VERSION_LEGACY) { + if self.version != MESSAGE_VERSION_LEGACY {
202-202: Simplify string length check and string comparison.Same issue as in line 171.
- if self.version.len() > 0 && self.version != String::from(MESSAGE_VERSION_LEGACY) { + if self.version != MESSAGE_VERSION_LEGACY {
28-36: Consider adding Default implementation for Message.Implementing the Default trait for the Message struct would make it easier to create new instances. This is especially useful for testing and when constructing messages programmatically.
#[derive(Debug)] pub struct Message { pub version: String, pub header: MessageHeader, pub account_keys: Vec<Vec<u8>>, pub recent_blockhash: [u8; 32], pub instructions: Vec<CompiledInstruction>, pub address_table_lookups: Vec<MessageAddressTableLookup>, } + +impl Default for Message { + fn default() -> Self { + Self { + version: String::from(MESSAGE_VERSION_LEGACY), + header: MessageHeader { + num_required_signatures: 0, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + account_keys: Vec::new(), + recent_blockhash: [0u8; 32], + instructions: Vec::new(), + address_table_lookups: Vec::new(), + } + } +}
38-42: Consider adding Default implementation for SolanaTransaction.Similar to the Message struct, implementing Default for SolanaTransaction would be helpful for testing and construction.
#[derive(Debug)] pub struct SolanaTransaction { pub signatures: Vec<Vec<u8>>, pub message: Message, } + +impl Default for SolanaTransaction { + fn default() -> Self { + Self { + signatures: Vec::new(), + message: Message::default(), + } + } +}
259-267: Add test coverage for encode_varint function.The encode_varint function is crucial for encoding transaction data but lacks explicit test coverage. Consider adding unit tests to verify its correctness for various input values, including edge cases like 0, values less than 0x80, values requiring multiple bytes, and large values.
Would you like me to generate some unit tests for the encode_varint function?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kos/src/chains/sol/models.rs(1 hunks)
🔇 Additional comments (1)
packages/kos/src/chains/sol/models.rs (1)
232-232: Guard against insufficient input before message parsing.Before calling
Message::decode, confirm that there's enough remaining buffer to accommodate at least 3 bytes for the message header. Otherwise, an out-of-bounds error is possible when the message tries to parse its header.+ if position >= input.len() { + return Err(prost::DecodeError::new("No bytes left to decode message")); + } + let message = Message::decode(&input[position..])?;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
packages/kos/src/chains/mod.rs (1)
142-147:⚠️ Potential issueFix typo for "InvalidAccountLength" message and unify style.
The display message at line 146 has a leading "i", which is likely a typo. Removing it also aligns the text with other error messages' style.
ChainError::InvalidAccountLength => { - write!(f, "iInvalid account length") + write!(f, "invalid account length") }packages/kos/src/chains/sol/models.rs (6)
75-79:⚠️ Potential issueAdd bounds checking before accessing account key bytes.
There is no verification that there are enough bytes remaining in the input for each account key. This could lead to an out-of-bounds panic.
for _ in 0..num_accounts { + if position + 32 > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for account key")); + } let mut key = vec![0u8; 32]; key.copy_from_slice(&input[position..position + 32]); account_keys.push(key); position += 32; }
83-85:⚠️ Potential issueAdd bounds checking before accessing recent blockhash.
There is no verification that there are enough bytes remaining in the input for the recent blockhash.
+if position + 32 > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for recent blockhash")); +} let mut recent_blockhash = [0u8; 32]; recent_blockhash.copy_from_slice(&input[position..position + 32]); position += 32;
100-102:⚠️ Potential issueAdd bounds checking before accessing account indexes.
The code copies account indexes without verifying that there are enough bytes available in the input.
+if position + num_accounts > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for account indexes")); +} let mut accounts = vec![0u8; num_accounts]; accounts.copy_from_slice(&input[position..position + num_accounts]); position += num_accounts;
169-173:⚠️ Potential issueAvoid potential panic when parsing version string.
The current implementation uses
unwrap()on the result ofparse::<u8>(), which would panic if the version string isn't a valid number. This could happen if someone manually constructs aMessagewith an invalid version.-if self.version.len() > 0 && self.version != String::from(MESSAGE_VERSION_LEGACY) { +if !self.version.is_empty() && self.version != MESSAGE_VERSION_LEGACY { let v_string = self.version[1..].to_string(); - let v = v_string.parse::<u8>().unwrap(); - output.push(v + 128); + if let Ok(v) = v_string.parse::<u8>() { + output.push(v + 128); + } else { + // Default to v0 if the version can't be parsed + output.push(128); + } }This also fixes three pipeline failures about string comparison style and length comparison.
🧰 Tools
🪛 GitHub Actions: KOS-RS Checks
[error] 169-169: length comparison to zero. Help: using
!is_emptyis clearer and more explicit:!self.version.is_empty().
[error] 169-169: this creates an owned instance just for comparison. Help: try:
*MESSAGE_VERSION_LEGACY.
223-228:⚠️ Potential issueAdd bounds checking before reading signatures.
The code attempts to read signature bytes without verifying that there are enough bytes available in the input buffer. This could lead to a panic with out-of-bounds access.
let mut signatures = Vec::with_capacity(num_signatures); for _ in 0..num_signatures { + if position + 64 > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for signature")); + } let mut signature = vec![0u8; 64]; signature.copy_from_slice(&input[position..position + 64]); signatures.push(signature); position += 64; }
219-231:⚠️ Potential issueGuard against insufficient input before message parsing.
Before calling
Message::decode, confirm that there's enough remaining buffer to accommodate at least 3 bytes for the message header. Otherwise, an out-of-bounds error is possible when the message tries to parse its header.let num_signatures = input[position] as usize; position += 1; +if position >= input.len() { + return Err(prost::DecodeError::new("No bytes left to decode message")); +} let message = Message::decode(&input[position..])?;
🧹 Nitpick comments (2)
packages/kos/src/chains/sol/models.rs (2)
119-120: Fix string comparison style.Using
String::from()creates an owned instance just for comparison. Use a direct reference comparison instead.-if version == String::from(MESSAGE_VERSION_V0) { +if version == MESSAGE_VERSION_V0 {This also fixes one of the pipeline failures.
🧰 Tools
🪛 GitHub Actions: KOS-RS Checks
[error] 120-120: this creates an owned instance just for comparison. Help: try:
*MESSAGE_VERSION_V0.
200-200: Fix redundant string comparison pattern.Similar to the previous issue, use better string comparison style and avoid empty length check.
-if self.version.len() > 0 && self.version != String::from(MESSAGE_VERSION_LEGACY) { +if !self.version.is_empty() && self.version != MESSAGE_VERSION_LEGACY {This fixes two more pipeline failures.
🧰 Tools
🪛 GitHub Actions: KOS-RS Checks
[error] 200-200: length comparison to zero. Help: using
!is_emptyis clearer and more explicit:!self.version.is_empty().
[error] 200-200: this creates an owned instance just for comparison. Help: try:
*MESSAGE_VERSION_LEGACY.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
packages/kos/src/chains/mod.rs(4 hunks)packages/kos/src/chains/sol/mod.rs(4 hunks)packages/kos/src/chains/sol/models.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: KOS-RS Checks
packages/kos/src/chains/sol/models.rs
[error] 120-120: this creates an owned instance just for comparison. Help: try: *MESSAGE_VERSION_V0.
[error] 169-169: length comparison to zero. Help: using !is_empty is clearer and more explicit: !self.version.is_empty().
[error] 169-169: this creates an owned instance just for comparison. Help: try: *MESSAGE_VERSION_LEGACY.
[error] 200-200: length comparison to zero. Help: using !is_empty is clearer and more explicit: !self.version.is_empty().
[error] 200-200: this creates an owned instance just for comparison. Help: try: *MESSAGE_VERSION_LEGACY.
🔇 Additional comments (11)
packages/kos/src/chains/mod.rs (3)
64-67: Good addition of new error variants for Solana.The new ChainError variants provide specific error handling for different aspects of Solana transaction processing, which will improve error reporting and debugging.
247-250: Harmonize the ordering of the error codes.In
to_u32(), the mapping order doesn't match the enum definition. Ensure consistency to avoid confusion for maintainers or automated checks:Suggested fix reflecting the enum order (if we keep the enum order as-is):
- ChainError::InvalidAccountLength => 26, - ChainError::InvalidBlockhash => 27, - ChainError::InvalidSignatureLength => 28, - ChainError::InvalidTransactionHeader => 29 + ChainError::InvalidTransactionHeader => 26, + ChainError::InvalidAccountLength => 27, + ChainError::InvalidBlockhash => 28, + ChainError::InvalidSignatureLength => 29
565-565: Great update to mark SOL as supported.This change aligns with the PR objective to add Solana network support for legacy and versioned transactions. The chain is now correctly marked as supported.
packages/kos/src/chains/sol/mod.rs (5)
1-1: Good modular organization with separate models module.Creating a separate module for Solana transaction models keeps the code organized and maintainable.
29-29: Correct implementation of get_decimals for SOL.Solana uses 9 decimal places for its native token (lamports to SOL), so this implementation is correct.
57-91: Well-implemented transaction signing with comprehensive validation.The transaction signing implementation includes proper validation checks for:
- The transaction header's signature requirements
- Account key presence
- Valid blockhash
- Signature length
This implementation correctly updates the transaction with the signature and handles error cases appropriately.
93-95: Good implementation of sign_message.Reusing the sign_raw method for message signing avoids code duplication and ensures consistent signing behavior.
127-215: Comprehensive test coverage for transaction signing.The tests cover critical aspects of transaction signing:
- Creating and signing test transactions
- Validating signature and hash properties
- Ensuring consistent results when signing identical transactions
- Testing with real-world transaction examples
This thorough testing approach helps ensure the implementation is reliable.
packages/kos/src/chains/sol/models.rs (3)
1-46: Well-structured data models for Solana transactions.The model definitions follow the Solana transaction format specification with clear separation of concerns between different components like headers, instructions, and address table lookups.
48-55: Good initial bounds checking for the header.The initial check for minimum message length prevents potential out-of-bounds access for the header bytes.
257-265: Good implementation of variable-length integer encoding.The
encode_varintfunction correctly implements the variable-length integer encoding scheme, which is efficient for representing values that are typically small but can be large.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/kos/src/chains/sol/models.rs (2)
47-168:⚠️ Potential issuePotential out-of-bounds in decode
Although there's a check for 3 bytes of header data, there is no thorough verification that
position + (32 * num_accounts)is withininput.len()before copying account keys. Similarly, for instructions, the code does not fully ensure that enough bytes remain for each declared account index and data length. This can cause slicing beyond the input buffer if the message is malformed.Consider adding checks like:
+ if position + (32 * num_accounts) > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for account keys")); + } for _ in 0..num_accounts { let mut key = vec![0u8; 32]; key.copy_from_slice(&input[position..position + 32]); position += 32; }
223-244:⚠️ Potential issuePotential out-of-bounds for signatures
This code reads each signature in 64-byte chunks without verifying that the buffer is large enough. A malformed input could cause an out-of-bounds panic.
Example fix:
+ if position + 64 > input.len() { + return Err(prost::DecodeError::new("Not enough bytes for signature")); + } let mut signature = vec![0u8; 64]; signature.copy_from_slice(&input[position..position + 64]); signatures.push(signature); position += 64;
🧹 Nitpick comments (1)
packages/kos/src/chains/sol/mod.rs (1)
127-152: Strengthen test coverage
The function is well-structured but could benefit from more negative or boundary-case tests (e.g., invalid instruction data).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
packages/kos/src/chains/mod.rs(4 hunks)packages/kos/src/chains/sol/mod.rs(4 hunks)packages/kos/src/chains/sol/models.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kos/src/chains/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (15)
packages/kos/src/chains/sol/mod.rs (6)
1-1: No issues with module import.
29-29: Correct Solana decimals
Returning 9 for the Solana chain is accurate.
57-91: Single-signature assumption
This implementation enforces exactly one required signature. If multi-signature usage is a future requirement, consider extending support fornum_required_signatures > 1.
93-95: Straightforward sign_message
Delegating tosign_rawhere is concise and correct.
154-183: Well-structured test
Covers typical usage, including derivation and signature checks.
185-216: Consistent signature test
Validates that signing the same transaction with the same key yields consistent signatures and tx_hash.packages/kos/src/chains/sol/models.rs (9)
1-9: Header structure
TheMessageHeaderdefinition is straightforward.
11-16: Instruction structure
CompiledInstructionis clearly laid out.
18-26: Address table lookup
MessageAddressTableLookupis well-defined for capturing address references.
28-36: Message structure
Fields align well with Solana transaction requirements.
38-42: Transaction container
SolanaTransactionholds signatures and the message cleanly.
44-45: Version constants
No issues found.
170-220: encode method
Properly constructs the byte output while handling version checks.
246-262: SolanaTransaction encode
Constructs the output buffer with signatures and message data in sequence.
265-273: Varint encoding
Standard approach to varint is implemented with no apparent issues.
Summary by CodeRabbit
Documentation
New Features
Tests
Bug Fixes