Fix/substrate sign tx #89
Conversation
WalkthroughThe pull request introduces significant changes to the Substrate chain implementation in the Kos SDK. Modifications enhance the transaction signing process by adding a new Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/kos/src/chains/substrate/mod.rs (2)
219-220: Check for decoding errors in test.Although this is test code, consider handling errors or adding an assertion to confirm the decoded data's integrity.
221-227: Add an assertion to verify the signature in your test.After calling
dot.sign_tx, it may be beneficial to assert the resulting transaction’s correctness (e.g., checkingtx.signaturelength or hash) to ensure the signing process behaves as expected.packages/kos/src/chains/substrate/models.rs (2)
139-154: Remove the underscore prefix for_metadata_hashor make it private.If
_metadata_hashis intended for internal usage only, consider making it a private field. Otherwise, drop the underscore prefix for consistency.
219-271: Add explanatory documentation for encoded flags and fields.Lines such as
encoded.push(0x00)andencoded.push(0x01)might not be self-explanatory. Consider adding doc comments to clarify which parts of the Substrate format these bytes represent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/substrate/models.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (7)
packages/kos/src/chains/substrate/mod.rs (5)
3-3: Import statement looks good.
6-6: Use of blake2b hashing imports is correct.
165-166: Test imports look appropriate.
214-214: Verify usage of an empty derivation path.When using
dot.get_path(0, false), the function might return an empty string, which leads to no derivation path. Confirm that this is intentional in your tests.
217-217: Use ofunwrapis acceptable in tests.packages/kos/src/chains/substrate/models.rs (2)
1-2: Import statements look correct.Also applies to: 5-5
156-217: Verify potential off-by-one usage indestination.copy_from_slice(&input[1..33]).You’re discarding byte
input[0]for the destination, which might be intentional if that byte encodes a type. Double-check that this is correct in your extrinsic format.- destination.copy_from_slice(&input[1..33]); + // If the first byte truly is part of the destination, consider: + // destination.copy_from_slice(&input[0..32]);✅ Verification successful
The current implementation is correct
The extra byte being skipped is a type identifier prefix, which is a standard pattern in Substrate's address encoding scheme. The destination field in the struct is defined as
[u8; 32], and the implementation correctly extracts these 32 bytes while skipping the type prefix byte.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar implementations and documentation echo "=== Searching for similar from_raw implementations ===" ast-grep --pattern 'fn from_raw($$$) { $$$destination.copy_from_slice($$$)$$$}' echo -e "\n=== Searching for extrinsic format documentation ===" rg -i "extrinsic.*format|payload.*format" -A 5 -B 5 echo -e "\n=== Searching for address type handling ===" rg "address.*type|account.*type" -A 3 -B 3Length of output: 2340
Script:
#!/bin/bash # Search for tests and encoding documentation echo "=== Searching for ExtrinsicPayload tests ===" rg "test.*ExtrinsicPayload" -A 10 -B 2 echo -e "\n=== Searching for SCALE encoding comments ===" rg -i "scale.*encoding|encode.*address|address.*encode" -A 3 -B 3 echo -e "\n=== Searching for address prefix comments ===" rg "prefix.*byte|address.*prefix|account.*prefix" -A 3 -B 3Length of output: 3533
Script:
#!/bin/bash # Search for ExtrinsicPayload struct definition echo "=== Searching for ExtrinsicPayload struct definition ===" rg "struct ExtrinsicPayload" -A 15 -B 2 echo -e "\n=== Searching for destination field type documentation ===" ast-grep --pattern 'struct ExtrinsicPayload { $$$ destination: $_, $$$ }'Length of output: 1562
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/kos/src/chains/substrate/mod.rs (1)
112-132:⚠️ Potential issueImprove error handling in the signing process.
The implementation has several areas that need attention:
- Unsafe public key conversion using
unwrap()- Potential memory leak of private key
- Missing documentation for the signing logic
Apply these improvements:
let signature = { let full_unsigned_payload_scale_bytes = tx.raw_data.clone(); + // Clean up the cloned private key after signing + let signature = { + let result = if full_unsigned_payload_scale_bytes.len() > 256 { + self.sign_raw( + private_key.clone(), + blake2b_digest(&full_unsigned_payload_scale_bytes).to_vec(), + ) + } else { + self.sign_raw(private_key.clone(), full_unsigned_payload_scale_bytes) + }; + result + }?; - let public_key: [u8; 32] = self.get_pbk(private_key)?.try_into().unwrap(); + let pbk_vec = self.get_pbk(private_key)?; + let public_key: [u8; 32] = pbk_vec + .try_into() + .map_err(|_| ChainError::Custom("Invalid public key length".to_string()))?;
🧹 Nitpick comments (1)
packages/kos/src/chains/substrate/mod.rs (1)
219-231: Add assertions for transaction validity.The test creates a transaction from base64-encoded data but lacks assertions about the transaction's validity before signing.
Add validation:
let raw_data = simple_base64_decode("BQMADCRBuM7b/Hou3AlouaU1gZlp0+ngmYaAurtYJyh/wHCRAXUDYAAA/E0PABoAAACRsXG7FY4tOEj6I6nxwlGC+44gMTssHrSSGdp6cM6Qw36KXLq5dgOoEvZRpzirvfO3HDN6fM3bEwtF1XTUSlrGAA==").unwrap(); +// Verify raw data length and content +assert!(!raw_data.is_empty(), "Raw data should not be empty"); +let payload = ExtrinsicPayload::from_raw(raw_data.clone()).expect("Should be valid payload"); + let tx = Transaction { raw_data, signature: Vec::new(),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/substrate/models.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/kos/src/chains/substrate/mod.rs (2)
112-126: Well-implemented payload size optimization.The implementation intelligently handles large payloads by hashing them when they exceed 256 bytes, which is a good practice for maintaining consistent performance and security.
However, consider adding a constant for the payload size threshold:
+const MAX_PAYLOAD_SIZE: usize = 256; + // If payload is longer than 256 bytes, we hash it and sign the hash instead: -if full_unsigned_payload_scale_bytes.len() > 256 { +if full_unsigned_payload_scale_bytes.len() > MAX_PAYLOAD_SIZE {
112-134: Consider extracting payload size handling logic.The current implementation of payload size handling is embedded within the
sign_txmethod. Consider extracting this logic into a separate method for better maintainability and reusability:+impl Substrate { + fn prepare_payload_for_signing(&self, payload: Vec<u8>) -> Vec<u8> { + const MAX_PAYLOAD_SIZE: usize = 256; + if payload.len() > MAX_PAYLOAD_SIZE { + blake2b_digest(&payload).to_vec() + } else { + payload + } + } +} fn sign_tx(&self, private_key: Vec<u8>, mut tx: Transaction) -> Result<Transaction, ChainError> { let extrinsic = ExtrinsicPayload::from_raw(tx.raw_data.clone())?; let signature = { let full_unsigned_payload_scale_bytes = tx.raw_data.clone(); - if full_unsigned_payload_scale_bytes.len() > 256 { - self.sign_raw( - private_key.clone(), - blake2b_digest(&full_unsigned_payload_scale_bytes).to_vec(), - )? - } else { - self.sign_raw(private_key.clone(), full_unsigned_payload_scale_bytes)? - } + let prepared_payload = self.prepare_payload_for_signing(full_unsigned_payload_scale_bytes); + self.sign_raw(private_key.clone(), prepared_payload)? };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kos/src/chains/substrate/mod.rs(4 hunks)
🔇 Additional comments (3)
packages/kos/src/chains/substrate/mod.rs (3)
3-3: LGTM! Required imports are properly added.The new imports for
ExtrinsicPayloadandblake2b_digestsupport the enhanced transaction signing functionality.Also applies to: 6-6
128-132: LGTM! Proper error handling implemented for public key conversion.The implementation now safely handles public key conversion errors, addressing the concerns raised in previous reviews.
222-234: Enhance test coverage with additional assertions.While the current test verifies basic functionality, consider adding:
- Verification of the signature's validity
- Test cases for payloads > 256 bytes to verify the hashing logic
- Error cases (e.g., invalid raw data)
Run this script to check existing test coverage:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kos/src/chains/substrate/mod.rs (1)
285-317: Document test data and add more test cases.The test uses hardcoded test data without documentation of its source or expected values.
Consider:
- Adding comments explaining the source and validity of test data
- Adding test cases for error conditions (invalid options, large payloads)
- Adding test cases for different Substrate chains
#[test] fn sign_tx() { + // Test data from Kusama transaction: + // https://kusama.subscan.io/extrinsic/[TODO: add reference transaction] let dot = super::Substrate::new(27, 2, "Kusama", "KSM");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kos/src/chains/mod.rs(5 hunks)packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/substrate/models.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (6)
packages/kos/src/chains/mod.rs (2)
266-275: LGTM! The SUBSTRATE variant is well-structured.The new SUBSTRATE variant in ChainOptions includes all necessary fields for Substrate transaction signing with appropriate types.
382-382: LGTM! Substrate chains are now supported.The changes enable support for multiple Substrate chains (DOT, KSM, REEF, SDN, ASTR, CFG, KILT, ALTAIR, and AVAIL) by setting their supported field to true.
Also applies to: 389-389, 403-403, 410-410, 417-417, 424-424, 438-438, 445-445, 585-585
packages/kos/src/chains/substrate/models.rs (3)
6-9: LGTM! Constants are well-defined.The constants are appropriately defined with correct values for Substrate transaction encoding.
161-188: LGTM! The encoding logic is correct and well-documented.The method properly implements the Substrate transaction format with clear documentation.
144-158: 🛠️ Refactor suggestionConsider improving field types and documentation.
The struct has some potential issues:
- The
metadata_hashfield isu8which seems insufficient for a hash value.- The
modefield lacks documentation explaining its purpose and valid values.Consider:
- Using
[u8; 32]orVec<u8>formetadata_hash.- Adding documentation for the
modefield.pub struct ExtrinsicPayload { pub call: Vec<u8>, pub era: Vec<u8>, pub nonce: u32, pub tip: u8, + /// Mode of the extrinsic. Valid values: [TODO: document valid values] pub mode: u8, pub spec_version: u32, pub transaction_version: u32, pub genesis_hash: [u8; 32], pub block_hash: [u8; 32], - pub metadata_hash: u8, + pub metadata_hash: [u8; 32], }Likely invalid or redundant comment.
packages/kos/src/chains/substrate/mod.rs (1)
112-174: LGTM! The sign_tx implementation is robust and secure.The method includes several good practices:
- Proper error handling for missing or invalid options
- Secure handling of large payloads by hashing them before signing
- Safe conversion of hash values with proper error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/kos/src/chains/mod.rs (1)
Line range hint
759-761: Implement custom Substrate chain support.The
get_chain_by_paramsfunction returnsNoneforCustomSubstratevariant, which might limit the ability to work with custom Substrate chains. Consider implementing this functionality similar to how custom EVM chains are handled.CustomChainType::CustomEth(chaincode) => Some(Box::new(eth::ETH::new_eth_based( 0, chaincode, format!("ETH {}", chaincode).as_str(), format!("Eth Based {}", chaincode).as_str(), ))), - CustomChainType::CustomSubstrate(_) => None, + CustomChainType::CustomSubstrate(chaincode) => Some(Box::new(substrate::Substrate::new( + 0, + chaincode, + format!("Substrate {}", chaincode).as_str(), + format!("Substrate Chain {}", chaincode).as_str(), + ))), CustomChainType::CustomCosmos(_) => None,
♻️ Duplicate comments (1)
packages/kos-mobile/src/lib.rs (1)
76-102: 🛠️ Refactor suggestionImprove error handling in substrate transaction options.
Similar to the web version, this function uses
unwrap_or_default()which silently handles errors.-fn new_substrate_transaction_options( +fn new_substrate_transaction_options( call: String, era: String, nonce: u32, tip: u8, block_hash: String, genesis_hash: String, spec_version: u32, transaction_version: u32, -) -> TransactionChainOptions { - let call = hex_string_to_vec(call.as_str()).unwrap_or_default(); - let era = hex_string_to_vec(era.as_str()).unwrap_or_default(); - let block_hash = hex_string_to_vec(block_hash.as_str()).unwrap_or_default(); - let genesis_hash = hex_string_to_vec(genesis_hash.as_str()).unwrap_or_default(); +) -> Result<TransactionChainOptions, KOSError> { + let call = hex_string_to_vec(call.as_str()) + .map_err(|e| KOSError::KOSDelegate(format!("Invalid call hex: {}", e)))?; + let era = hex_string_to_vec(era.as_str()) + .map_err(|e| KOSError::KOSDelegate(format!("Invalid era hex: {}", e)))?; + let block_hash = hex_string_to_vec(block_hash.as_str()) + .map_err(|e| KOSError::KOSDelegate(format!("Invalid block hash hex: {}", e)))?; + let genesis_hash = hex_string_to_vec(genesis_hash.as_str()) + .map_err(|e| KOSError::KOSDelegate(format!("Invalid genesis hash hex: {}", e)))?; - TransactionChainOptions::Substrate { + Ok(TransactionChainOptions::Substrate { call, era, nonce, tip, block_hash, genesis_hash, spec_version, transaction_version, - } + }) }
🧹 Nitpick comments (3)
packages/kos/src/chains/util.rs (1)
17-20: Add input validation for hex string.The function should validate that the input string contains valid hexadecimal characters after trimming the "0x" prefix.
pub fn hex_string_to_vec(hex: &str) -> Result<Vec<u8>, ChainError> { let hex = hex.trim_start_matches("0x"); + if !hex.chars().all(|c| c.is_ascii_hexdigit()) { + return Err(ChainError::InvalidHex); + } hex::decode(hex).map_err(|_| ChainError::InvalidHex) }packages/kos/src/chains/substrate/mod.rs (1)
253-294: Enhance test coverage with additional assertions.While the test verifies signature and raw data lengths, it could be improved by adding more assertions.
Add these assertions to verify the transaction content:
assert_eq!(signed_tx.signature.len(), 65); assert_eq!(signed_tx.raw_data.len(), 143); +// Verify signature format +assert_eq!(signed_tx.signature[0], 1u8, "Signature version should be 1"); +assert!(signed_tx.signature[1..].len() == 64, "Signature payload should be 64 bytes"); + +// Verify the transaction was properly encoded +assert!(signed_tx.raw_data[0] < 128, "First byte should be a valid version");packages/kos/src/chains/mod.rs (1)
271-280: Consider using a larger type for the tip field.The
tipfield is currently defined asu8, which limits the maximum tip to 255. Consider usingu128to align with Substrate'sBalancetype for flexibility with larger tips.SUBSTRATE { call: Vec<u8>, era: Vec<u8>, nonce: u32, - tip: u8, + tip: u128, block_hash: Vec<u8>, genesis_hash: Vec<u8>, spec_version: u32, transaction_version: u32, },Add documentation for the SUBSTRATE variant.
Consider adding documentation to explain the purpose and requirements of each field, especially for fields like
eraandspec_versionthat have specific formats or constraints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/kos-mobile/src/lib.rs(3 hunks)packages/kos-web/src/wallet.rs(2 hunks)packages/kos/src/chains/mod.rs(9 hunks)packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/util.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (5)
packages/kos-web/src/wallet.rs (1)
64-69: LGTM!The function is simple and correctly creates EVM chain options with the provided chain ID.
packages/kos-mobile/src/lib.rs (1)
64-74: LGTM!The Substrate variant in the TransactionChainOptions enum is well-structured with all necessary fields.
packages/kos/src/chains/substrate/mod.rs (1)
112-174: LGTM! Well-structured implementation with proper error handling.The implementation correctly:
- Extracts and validates transaction options
- Handles payload length optimization (hashing if > 256 bytes)
- Uses proper error handling with Result type
packages/kos/src/chains/mod.rs (2)
61-61: LGTM: Error handling for hex validation.The new
InvalidHexerror variant is well-integrated into the error handling system with appropriate Display implementation and error code.Also applies to: 127-129, 220-220
387-387: Verify the base parameter values for Substrate chains.Some chains share the same base parameter value:
- SDN and ASTR both use base=5
- REEF and AVAIL both use base=42
Please verify that these values are correct, as sharing the same base parameter might cause issues with chain-specific functionality.
Also applies to: 394-394, 408-408, 415-415, 422-422, 429-429, 443-443, 450-450, 590-590
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kos-web/src/wallet.rs (1)
71-104: Consider using a builder pattern or parameter struct.The method takes 8 parameters which makes it harder to maintain and use. Consider introducing a
SubstrateSignOptionsstruct to encapsulate these parameters:#[wasm_bindgen] pub struct SubstrateSignOptions { call: String, era: String, nonce: u32, tip: u8, block_hash: String, genesis_hash: String, spec_version: u32, transaction_version: u32, } #[wasm_bindgen] impl TransactionChainOptions { #[wasm_bindgen(js_name = "newSubstrateSignOptions")] pub fn new_substrate_sign_options(options: SubstrateSignOptions) -> Result<TransactionChainOptions, Error> { let call = hex_string_to_vec(options.call.as_str()) .map_err(|e| Error::WalletManager(format!("Invalid call hex: {}", e)))?; // ... rest of the conversions ... Ok(TransactionChainOptions { data: ChainOptions::SUBSTRATE { call, era, nonce: options.nonce, tip: options.tip, block_hash, genesis_hash, spec_version: options.spec_version, transaction_version: options.transaction_version, }, }) } }This would:
- Improve code readability
- Make parameter management easier
- Allow for future extensions without breaking changes
- Provide a more ergonomic API for JavaScript consumers
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kos-web/src/wallet.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (3)
packages/kos-web/src/wallet.rs (3)
9-9: LGTM!The import is correctly placed and the utility function is used appropriately in the code.
64-69: LGTM!The implementation is clean, focused, and correctly handles EVM chain requirements. The WASM binding ensures proper JavaScript interoperability.
71-104: LGTM! Proper error handling implemented.The implementation correctly handles hex string conversions with appropriate error messages, following the previous review suggestions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/kos/src/chains/substrate/models.rs (1)
6-9: Document the constants and their purpose.The constants are used in the transaction encoding process but lack documentation explaining their purpose and values.
+/// Flag indicating that the transaction is signed (0b1000_0000) const SIGNED_FLAG: u8 = 0b1000_0000; +/// Version of the transaction format (4) const TRANSACTION_VERSION: u8 = 4; +/// Type identifier for public key in transaction encoding (0x00) const PUBLIC_KEY_TYPE: u8 = 0x00; +/// Type identifier for signature in transaction encoding (0x01) const SIGNATURE_TYPE: u8 = 0x01;packages/kos-mobile/src/lib.rs (1)
64-74: Document the enum variant fields.The Substrate variant lacks documentation explaining its fields and their purposes.
+ /// Options specific to Substrate chain transactions Substrate { + /// The encoded call data call: Vec<u8>, + /// The era phase and period for transaction mortality era: Vec<u8>, + /// The transaction nonce nonce: u32, + /// The transaction tip for block producers tip: u8, + /// The hash of the block this transaction is valid for block_hash: Vec<u8>, + /// The hash of the genesis block of the chain genesis_hash: Vec<u8>, + /// The runtime specification version spec_version: u32, + /// The transaction format version transaction_version: u32, + /// Optional application-specific identifier (used in AVAIL chain) app_id: Option<u32>, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/kos-mobile/src/lib.rs(3 hunks)packages/kos-web/src/wallet.rs(2 hunks)packages/kos/src/chains/mod.rs(9 hunks)packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/substrate/models.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (8)
packages/kos-web/src/wallet.rs (1)
71-106:⚠️ Potential issueImprove error handling in hex string conversions.
The method uses
unwrap_or_default()which silently handles errors by returning empty vectors. This could mask issues and make debugging difficult.- let call = hex_string_to_vec(call.as_str()).unwrap_or_default(); - let era = hex_string_to_vec(era.as_str()).unwrap_or_default(); - let block_hash = hex_string_to_vec(block_hash.as_str()).unwrap_or_default(); - let genesis_hash = hex_string_to_vec(genesis_hash.as_str()).unwrap_or_default(); + let call = hex_string_to_vec(call.as_str()) + .map_err(|e| Error::WalletManager(format!("Invalid call hex: {}", e)))?; + let era = hex_string_to_vec(era.as_str()) + .map_err(|e| Error::WalletManager(format!("Invalid era hex: {}", e)))?; + let block_hash = hex_string_to_vec(block_hash.as_str()) + .map_err(|e| Error::WalletManager(format!("Invalid block hash hex: {}", e)))?; + let genesis_hash = hex_string_to_vec(genesis_hash.as_str()) + .map_err(|e| Error::WalletManager(format!("Invalid genesis hash hex: {}", e)))?;Likely invalid or redundant comment.
packages/kos/src/chains/substrate/mod.rs (4)
112-176: LGTM! The transaction signing implementation is robust and secure.The function has several good practices:
- Proper error handling for options extraction and array conversions
- Secure handling of large payloads by hashing them before signing
- Clear separation of concerns between signature generation and transaction finalization
254-298: LGTM! The test provides good coverage with real-world data.The test effectively validates:
- Transaction signing with real Polkadot transaction data
- Signature and raw data length assertions
301-345: LGTM! The test provides good coverage for Kusama chain.The test effectively validates transaction signing with real Kusama transaction data.
348-396: LGTM! The test provides good coverage for Avail chain.The test effectively validates:
- Transaction signing with real Avail transaction data
- Hex string parsing for nonce, spec_version, and transaction_version
packages/kos/src/chains/mod.rs (3)
61-61: LGTM! The error variant is properly integrated.The
InvalidHexerror variant is well-integrated with proper error message and unique error code.Also applies to: 127-129, 220-220
271-281: LGTM! The SUBSTRATE variant is well-structured.The variant includes all necessary fields for Substrate transactions with appropriate types.
388-388: LGTM! Chain support is properly enabled for multiple Substrate chains.Support is consistently enabled for DOT, KSM, REEF, SDN, ASTR, CFG, KILT, ALTAIR, and AVAIL chains.
Also applies to: 395-395, 409-409, 416-416, 423-423, 430-430, 444-444, 451-451, 590-591
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/kos/src/chains/substrate/models.rs (3)
6-9: Document the constants.Add documentation to explain the purpose and usage of each constant in the transaction encoding process.
+/// Flag indicating a signed transaction (0b1000_0000). const SIGNED_FLAG: u8 = 0b1000_0000; +/// Version of the transaction format (4). const TRANSACTION_VERSION: u8 = 4; +/// Type identifier for public key (0x00). const PUBLIC_KEY_TYPE: u8 = 0x00; +/// Type identifier for signature (0x01). const SIGNATURE_TYPE: u8 = 0x01;
144-159: Consider using a larger type for thetipfield.The
tipfield is currentlyu8, which limits the maximum tip value to 255. Consider using a larger type likeu128or the chain's native Balance type to allow for more meaningful transaction prioritization.pub struct ExtrinsicPayload { pub call: Vec<u8>, pub era: Vec<u8>, pub nonce: u32, - pub tip: u8, + pub tip: u128, pub mode: u8, pub spec_version: u32, pub transaction_version: u32, pub genesis_hash: [u8; 32], pub block_hash: [u8; 32], pub metadata_hash: u8, pub app_id: Option<u32>, }
164-189: Optimize memory allocation and cloning.The method could be optimized by:
- Pre-calculating and pre-allocating the required capacity
- Avoiding unnecessary cloning
pub fn to_bytes(&self) -> Vec<u8> { - let mut encoded = Vec::new(); + // Calculate total length for better memory allocation + let total_length = self.call.len() + + self.era.len() + + Compact(self.nonce).encoded_size() + + Compact(self.tip).encoded_size() + + if self.app_id.is_some() { + Compact(self.app_id.unwrap()).encoded_size() + } else { + 1 // mode + } + + self.spec_version.encoded_size() + + self.transaction_version.encoded_size() + + 32 + // genesis_hash + 32 + // block_hash + if self.app_id.is_none() { 1 } else { 0 }; // metadata_hash + + let mut encoded = Vec::with_capacity(total_length); - encoded.extend(self.call.clone()); + encoded.extend_from_slice(&self.call); - encoded.extend(&self.era.clone()); + encoded.extend_from_slice(&self.era); encoded.extend(Compact(self.nonce).encode()); encoded.extend(Compact(self.tip).encode());packages/kos-web/src/wallet.rs (1)
72-107: Consider using the builder pattern.The method has many parameters which makes it harder to use and maintain. Consider implementing a builder pattern to improve readability and maintainability.
#[wasm_bindgen] pub struct SubstrateSignOptionsBuilder { call: Option<String>, era: Option<String>, nonce: Option<u32>, tip: Option<u8>, block_hash: Option<String>, genesis_hash: Option<String>, spec_version: Option<u32>, transaction_version: Option<u32>, app_id: Option<u32>, } #[wasm_bindgen] impl SubstrateSignOptionsBuilder { #[wasm_bindgen(constructor)] pub fn new() -> Self { Self { call: None, era: None, nonce: None, tip: None, block_hash: None, genesis_hash: None, spec_version: None, transaction_version: None, app_id: None, } } pub fn call(mut self, call: String) -> Self { self.call = Some(call); self } // Add other setters... pub fn build(self) -> Result<TransactionChainOptions, Error> { let call = hex_string_to_vec(self.call.ok_or(Error::WalletManager("call is required".to_string()))?.as_str()) .map_err(|e| Error::WalletManager(format!("Invalid call hex: {}", e)))?; // Convert other fields... Ok(TransactionChainOptions { data: ChainOptions::SUBSTRATE { call, // Other fields... }, }) } }packages/kos/src/chains/substrate/mod.rs (1)
317-519: Add negative test cases.The test suite is comprehensive for positive cases but lacks tests for error conditions. Consider adding tests for:
- Invalid hex strings
- Missing or malformed fields in browser JSON
- Invalid signature lengths
- Invalid public key lengths
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/kos-web/src/wallet.rs(2 hunks)packages/kos/Cargo.toml(1 hunks)packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/substrate/models.rs(2 hunks)packages/kos/src/chains/util.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kos/src/chains/util.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (4)
packages/kos/src/chains/substrate/models.rs (1)
191-224: LGTM!The implementation is well-structured with:
- Clear documentation
- Proper capacity pre-allocation
- Efficient use of extend_from_slice
packages/kos-web/src/wallet.rs (1)
65-70: LGTM!The implementation is simple and correct.
packages/kos/src/chains/substrate/mod.rs (1)
107-178: LGTM!The implementation is well-structured with:
- Proper error handling
- Clear pattern matching
- Efficient payload handling for large transactions
packages/kos/Cargo.toml (1)
64-64: LGTM!The serde dependency is correctly specified with:
- Pinned version
- Derive feature enabled
Summary by CodeRabbit
New Features
ExtrinsicPayloadstruct with advanced encoding and decoding capabilities.SUBSTRATEvariant inChainOptionsfor detailed chain configurations.Improvements
serdedependency for improved serialization and deserialization capabilities.