Normalize chains to use kos codec #136
Conversation
…S-8266-kos-rs-normalize-chains-to-use-kos-codec-for-all-chains
… and wallet generator
WalkthroughThis pull request refactors submodule paths, updates build configurations, and significantly expands blockchain transaction handling. Changes include renaming a submodule entry, modifying dependency specifications and build scripts, and adding new modules and functions for encoding transactions for signing and broadcasting for various chains (BTC, KLV, SOL, TRX, ETH, SUBSTRATE, ICP). Additionally, error handling improvements and test enhancements have been applied in mobile and web components, while existing signing functions in core chain implementations have been streamlined or simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant L as Library (encode_for_signing)
participant C as Chain Module (BTC/KLV/SOL/TRX/ETH/SUBSTRATE/ICP)
U->>L: Request transaction signing (send Transaction)
L->>C: Dispatch to chain-specific encode_for_sign()
C->>C: Validate & process transaction data
C->>L: Return signed Transaction
L->>U: Return signed Transaction
sequenceDiagram
participant U as User
participant L as Library (encode_for_broadcast)
participant C as Chain Module (BTC/KLV/SOL/TRX/ETH/SUBSTRATE/ICP)
U->>L: Request transaction broadcast (send Transaction)
L->>C: Dispatch to chain-specific encode_for_broadcast()
C->>C: Validate & update transaction with signature
C->>L: Return broadcast-ready Transaction
L->>U: Return updated 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 (2)
🪧 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 (9)
packages/kos-web/demo/src/components/transaction-signer.ts (1)
345-355: Enhanced error handling in transaction signing.The pattern of checking error types before accessing their properties is consistently applied across the codebase, improving overall robustness and user experience.
Consider extracting this common error handling pattern into a shared utility function to reduce code duplication across these three locations in this file, as well as in the other component files.
+ private handleError(error: unknown): void { + if (error instanceof Error) { + this.showError(error.message); + return; + } + if (typeof error === "string") { + this.showError(error); + return; + } + + this.showError("An unknown error occurred"); + }Then replace the duplicated blocks with calls to this method:
- if (error instanceof Error) { - this.showError(error.message); - return; - } - if (typeof error === "string") { - this.showError(error); - return; - } - - this.showError("An unknown error occurred"); + this.handleError(error);packages/kos/src/chains/btc/mod.rs (1)
177-213: Review the multi-signature chunking logic and alignment with transaction inputs.
- The code signs each 32-byte chunk in
tx.tx_hash, collecting signatures in a single array. Ensure this aligns with the intended number of inputs and expected signatures.- Consider whether you need to validate the total length of
tx.tx_hashto confirm it is a multiple of 32.- The final signature is concatenated, which may be fine for single-key signing or lightweight multi-sign scenarios, but confirm you're not inadvertently merging multiple signatures in ways that downstream code might misinterpret.
Overall, the new signing logic is streamlined and consistent with the approach used in other chain modules.
packages/kos-codec/src/chains/sol/mod.rs (1)
42-93: Test coverage is good; consider adding negative tests.
Current tests verify correct encoding paths. Adding a test with obviously invalid data (e.g., empty blockhash or missing signatures) would confirm that error handling paths work as intended.packages/kos-codec/src/chains/klv/mod.rs (2)
9-30: Consider unifying or centralizing empty-array handling.
The repeated logic of replacing"[]"with[""]could be encapsulated in a helper function to improve clarity and facilitate reuse, especially as bothencode_for_signandencode_for_broadcastrely on the same handling pattern.
48-94: Add an additional test for empty JSON arrays.
Although the existing tests look solid, the code includes special handling for empty arrays ("[]"→[""]). Consider adding a direct test case with literal[]in the input to ensure this transformation works as expected.packages/kos-codec/src/chains/trx/mod.rs (2)
21-31: Validate multi-sig handling.
Appending an additional signature to the transaction’s signature vector is fine if Tron supports multi-sig in this manner. Otherwise, ensure only one signature is set or that the code handles multiple signatures properly.
49-97: Test coverage is good; consider negative cases.
Your tests look thorough for valid flows. A follow-up might be negative or boundary tests (e.g., invalid raw data, missing signature).packages/kos-codec/src/chains/btc/mod.rs (2)
195-209: Document the 64-byte signature assumption.
The helper processes 64-byte raw signature chunks, presumably R and S in compact form. A small comment clarifying that these are pure (R, S) pairs (not DER-encoded) could help maintainers.
210-280: Tests are comprehensive; expand negative/edge coverage.
Consider adding negative tests for mismatched input lengths or unsupported script types. This ensures resilience to malformed data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockpackages/kos-codec/src/protos/generated/klv/google.protobuf.rsis excluded by!**/generated/**packages/kos-codec/src/protos/generated/klv/mod.rsis excluded by!**/generated/**packages/kos-codec/src/protos/generated/klv/proto.rsis excluded by!**/generated/**packages/kos-codec/src/protos/generated/mod.rsis excluded by!**/generated/**packages/kos-codec/src/protos/generated/trx/google.protobuf.rsis excluded by!**/generated/**packages/kos-codec/src/protos/generated/trx/mod.rsis excluded by!**/generated/**packages/kos-codec/src/protos/generated/trx/protocol.rsis excluded by!**/generated/**
📒 Files selected for processing (22)
.gitmodules(1 hunks)packages/kos-codec/Cargo.toml(2 hunks)packages/kos-codec/src/chains/btc/mod.rs(1 hunks)packages/kos-codec/src/chains/klv/mod.rs(1 hunks)packages/kos-codec/src/chains/klv/models.rs(8 hunks)packages/kos-codec/src/chains/mod.rs(1 hunks)packages/kos-codec/src/chains/sol/mod.rs(1 hunks)packages/kos-codec/src/chains/sol/models.rs(0 hunks)packages/kos-codec/src/chains/trx/mod.rs(1 hunks)packages/kos-codec/src/lib.rs(3 hunks)packages/kos-codec/src/protos/mod.rs(1 hunks)packages/kos-mobile/src/lib.rs(3 hunks)packages/kos-web/demo/src/components/cryptography-demo.ts(1 hunks)packages/kos-web/demo/src/components/transaction-signer.ts(3 hunks)packages/kos-web/demo/src/components/wallet-generator.ts(1 hunks)packages/kos/Cargo.toml(0 hunks)packages/kos/src/chains/btc/mod.rs(4 hunks)packages/kos/src/chains/klv/mod.rs(3 hunks)packages/kos/src/chains/sol/mod.rs(2 hunks)packages/kos/src/chains/substrate/mod.rs(0 hunks)packages/kos/src/chains/trx/mod.rs(4 hunks)packages/kos/src/lib.rs(1 hunks)
💤 Files with no reviewable changes (3)
- packages/kos/src/chains/substrate/mod.rs
- packages/kos-codec/src/chains/sol/models.rs
- packages/kos/Cargo.toml
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/kos-codec/src/protos/mod.rs (1)
packages/kos/src/crypto/base64.rs (2)
simple_base64_decode(33-63)simple_base64_encode(2-30)
packages/kos/src/chains/klv/mod.rs (12)
packages/kos/src/crypto/hash.rs (1)
keccak256_digest(34-41)packages/kos/src/chains/sol/mod.rs (1)
get_tx_info(83-85)packages/kos/src/chains/btc/mod.rs (1)
get_tx_info(249-251)packages/kos/src/chains/trx/mod.rs (1)
get_tx_info(139-141)packages/kos/src/chains/sui/mod.rs (1)
get_tx_info(112-114)packages/kos/src/chains/ada/mod.rs (1)
get_tx_info(166-168)packages/kos/src/chains/apt/mod.rs (1)
get_tx_info(103-105)packages/kos/src/chains/atom/mod.rs (1)
get_tx_info(157-159)packages/kos/src/chains/icp/mod.rs (1)
get_tx_info(149-151)packages/kos/src/chains/bch/mod.rs (1)
get_tx_info(195-197)packages/kos/src/chains/mod.rs (1)
get_tx_info(396-396)packages/kos/src/chains/xrp/mod.rs (1)
get_tx_info(169-171)
packages/kos/src/chains/trx/mod.rs (11)
packages/kos/src/chains/klv/mod.rs (1)
get_tx_info(105-107)packages/kos/src/chains/sol/mod.rs (1)
get_tx_info(83-85)packages/kos/src/chains/btc/mod.rs (1)
get_tx_info(249-251)packages/kos/src/chains/sui/mod.rs (1)
get_tx_info(112-114)packages/kos/src/chains/ada/mod.rs (1)
get_tx_info(166-168)packages/kos/src/chains/apt/mod.rs (1)
get_tx_info(103-105)packages/kos/src/chains/atom/mod.rs (1)
get_tx_info(157-159)packages/kos/src/chains/icp/mod.rs (1)
get_tx_info(149-151)packages/kos/src/chains/bch/mod.rs (1)
get_tx_info(195-197)packages/kos/src/chains/mod.rs (1)
get_tx_info(396-396)packages/kos/src/chains/xrp/mod.rs (1)
get_tx_info(169-171)
packages/kos-codec/src/chains/sol/mod.rs (3)
packages/kos-codec/src/chains/trx/mod.rs (2)
encode_for_sign(6-19)encode_for_broadcast(21-31)packages/kos-codec/src/chains/sol/models.rs (4)
decode(45-165)decode(221-241)encode(167-217)encode(243-259)packages/kos-codec/src/lib.rs (1)
encode_for_broadcast(36-57)
packages/kos-mobile/src/lib.rs (2)
packages/kos-codec/src/chains/sol/models.rs (2)
decode(45-165)decode(221-241)packages/kos/src/chains/substrate/models.rs (2)
decode(61-105)decode(111-141)
packages/kos-codec/src/lib.rs (1)
packages/kos-codec/src/chains/btc/mod.rs (2)
encode_for_sign(4-79)encode_for_broadcast(81-193)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (51)
packages/kos-web/demo/src/components/wallet-generator.ts (1)
255-266: Improved error handling implementation.The updated error handling logic properly checks different error types before accessing their properties. This approach is more robust than directly casting to Error type, as it gracefully handles various error representations.
packages/kos-web/demo/src/components/cryptography-demo.ts (1)
129-139: Robust error handling approach.The error handling has been improved to properly check the type of the error before accessing its properties. This prevents potential runtime errors when dealing with different types of errors and ensures users receive appropriate error messages.
packages/kos-web/demo/src/components/transaction-signer.ts (2)
238-248: Consistent error handling pattern.The updated error handling properly checks error types, preventing potential runtime errors from incorrect type assumptions and providing better user feedback.
279-289: Improved error handling for message signing.The error handling pattern is well-implemented and consistent with other methods in the codebase, making error reporting more robust.
packages/kos-codec/src/chains/mod.rs (1)
5-8: Modules added for new chain types - Looks good!The addition of these four new modules (btc, klv, sol, trx) expands the codec's functionality to handle more blockchain types. This aligns well with the PR objective of normalizing chains to use the kos codec.
.gitmodules (1)
1-2: Submodule path correctly updated to kos-codecThe submodule path has been properly updated from
packages/kos/src/protos/trontopackages/kos-codec/src/protos/tron, which is consistent with the restructuring mentioned in the PR summary. This ensures the Tron protocol definitions are available in the kos-codec package.packages/kos/src/lib.rs (1)
2-2: Added extern crate alloc - Good addition for no_std supportThe addition of
extern crate alloc;is appropriate since this module uses#![cfg_attr(feature = "ksafe", no_std)]and needs memory allocation functionality when the standard library isn't available.packages/kos-codec/src/protos/mod.rs (1)
3-3:Details
✅ Verification successful
Updated imports to use kos package - Ensure dependency is configured
The import has been correctly updated to use
kos::crypto::base64instead of the local implementation. This is consistent with the PR's goal of normalizing codec usage.Run the following to verify that kos-codec has a dependency on kos in its Cargo.toml:
🏁 Script executed:
#!/bin/bash # Check if kos-codec properly depends on kos grep -A 10 "dependencies" packages/kos-codec/Cargo.toml | grep "kos"Length of output: 93
Dependency Verified: Import and Cargo.toml Configuration Correct
The updated import in
packages/kos-codec/src/protos/mod.rsnow correctly useskos::crypto::base64::{simple_base64_decode, simple_base64_encode}as intended. The dependency check inpackages/kos-codec/Cargo.tomlconfirms that thekospackage is properly configured with:kos = { workspace = true }No further issues are identified.
packages/kos-codec/Cargo.toml (3)
10-10: Enable a custom build script.Defining a build script in
build.rsis standard practice for tasks like generating code from.protofiles. This looks good.
33-34: Add build-dependency onprost-build.Including
prost-buildensures proto compilation. This aligns well with the newly addedprostdependencies.
27-30:Details
❓ Verification inconclusive
Verify new dependencies and check for security advisories.
The newly added dependencies (
rlp,prost,tiny-json-rs,prost-types) look correct for handling serialization, but please verify they’re free of known vulnerabilities.
🏁 Script executed:
#!/bin/bash # Use cargo-audit if available to check for vulnerabilities in newly added crates cargo install --version ^0.17 cargo-audit || true cargo auditLength of output: 258
Manual Verification Needed for Cargo Audit
The updated dependencies in
packages/kos-codec/Cargo.toml(lines 27-30) appear correctly specified for serialization tasks. However, the initial cargo audit command did not run as expected due to the absence of Cargo in the current environment. Please runcargo auditon a system where Cargo is available to verify that the new dependencies (rlp,prost,tiny-json-rs,prost-types) are free from known vulnerabilities.packages/kos/src/chains/klv/mod.rs (4)
1-1: ImportTxInfointo scope.Bringing
TxInfointo scope suggests you may return or handle transaction metadata. No issues spotted.
4-4: Includekeccak256_digestfor hashing.You’re now using the
keccak256_digestfunction, which is consistent across other chains for generating transaction hashes.
105-106:get_tx_infonot supported.Returning
Err(ChainError::NotSupported)is consistent with other chain modules that don't provide transaction info.
81-81:Details
❓ Verification inconclusive
Signing transaction with
tx.tx_hash.Directly signing
tx.tx_hashmatches the pattern used for other chains. Ensure the transaction hash is computed correctly and is 32 bytes.
🏁 Script executed:
#!/bin/bash # Verifies that tx_hash is always 32 bytes in KLV transactions rg 'tx\.tx_hash' -A 4Length of output: 2785
Ensure 32-byte Transaction Hash Integrity
The current implementation in
packages/kos/src/chains/klv/mod.rsfollows the same pattern as other chains by signingtx.tx_hashdirectly:let sig = self.sign_raw(private_key, tx.tx_hash.clone())?;Please double-check that the transaction hash generation logic guarantees a 32-byte hash for KLV transactions. If there’s any possibility of the hash not being exactly 32 bytes, consider adding an explicit validation check to prevent unexpected errors.
packages/kos-codec/src/lib.rs (4)
2-2: Addedmod protos;.Defining the
protosmodule is presumably for.proto-related files. Please ensure it’s referenced as needed in your build script.
4-4: Import additional chain modules for multi-chain support.This import statement broadens coverage by including
btc,klv,sol, andtrx. Looks consistent with the new encode methods.
28-31: Extendencode_for_signingto handle BTC, KLV, TRX, and SOL.Supporting these chains is a beneficial expansion. Consider adding tests to ensure each chain’s encode logic behaves correctly.
51-54: Extendencode_for_broadcastto handle BTC, KLV, TRX, and SOL.Broadcast logic is now unified for more chains. No immediate concerns; make sure each chain's broadcast path is tested.
packages/kos/src/chains/trx/mod.rs (5)
2-2: ImportTxInfofor TRX chain.Including
TxInfoindicates potential for detailed transaction data, though it's currently not supported for TRX.
7-7: Usealloc::format.Explicitly importing
formatis standard inno_stdcontexts. No issues found.
139-140:get_tx_infonot supported.Returning
Err(ChainError::NotSupported)matches the approach in other chains where transaction decoding is unavailable.
150-150: Usecrate::chains::Chainin tests.This import is straightforward and ensures test code can call chain trait methods.
104-106:Details
❓ Verification inconclusive
Sign transaction using
tx.tx_hash.Copying
tx.tx_hashinto a 32-byte buffer is correct for TRX—just confirmtx.tx_hashis always 32 bytes.
🏁 Script executed:
#!/bin/bash # Ensures tx_hash is consistently 32 bytes for TRX rg 'tx\.tx_hash' -A 3Length of output: 2435
Verify that
tx.tx_hashis always 32 bytes for TRX signingThe current implementation in
packages/kos/src/chains/trx/mod.rs:payload.copy_from_slice(&tx.tx_hash[..]); tx.signature = secp256k1::Secp256K1::sign(&payload, &pvk_bytes)?.to_vec();correctly copies
tx.tx_hashinto a 32-byte buffer for signing. Please ensure that every code path settingtx.tx_hashguarantees it will always be exactly 32 bytes. If that invariant isn’t enforced elsewhere, consider adding an explicit length check or appropriate validations to prevent potential issues.packages/kos/src/chains/sol/mod.rs (2)
6-6: No functional issues detected on this import line.
61-61: Consider validatingtx_hashbefore signing.
The function unconditionally signstx_hashwithout checking if it is empty or malformed. Ensuring thattx_hashis non-empty can prevent cryptographic misuse or accidental signing of an invalid payload.Would you like to add a check similar to the BTC workflow, where an empty
tx_hashyields an error?packages/kos/src/chains/btc/mod.rs (3)
2-3: No issues with modified imports.
13-13: Constant definition looks correct.
DefiningIDmakes the codebase more flexible and self-documenting.
36-36: Use ofIDis consistent with the newly introduced constant.
This approach improves maintainability by avoiding hard-coded values.packages/kos-codec/src/chains/sol/mod.rs (3)
1-5: Module creation is clean and well-structured.
Export statements and module references appear valid.
6-24: Validation checks for Solana transactions look solid.
- Verifying
num_required_signatures == 1is appropriate for single-signature use cases.- Checking the blockhash for all-zero or all-ones patterns prevents signing obviously invalid transactions.
- Assigning the encoded message to
transaction.tx_hashmakes sense in Solana’s context, but confirm this is the desired “hash” reference for subsequent signing steps.
26-40: Signature encoding for broadcast is correct.
- Asserted signature length of 64 matches Ed25519.
- Replacing any prior signatures with
transaction.signatureis straightforward.- Storing the signature in
transaction.tx_hashis consistent with other chain modules that track final states intx_hash.packages/kos-codec/src/chains/klv/mod.rs (2)
1-8: Overall structure looks good.
No evident issues in the module declaration and imports.
32-46: Validate the signature encoding structure.
You are wrapping the Base64-encoded signature in an array with a single element (Some(Vec::from([simple_base64_encode(&transaction.signature)]))). If this chain eventually supports multiple signatures, ensure this design doesn’t conflict with multi-sig requirements.packages/kos-codec/src/chains/trx/mod.rs (3)
1-5: No issues with imports.
All dependencies appear correctly referenced for Tron transaction decoding and hashing.
6-19: Confirm final raw_data consistency.
After computing thetx_hash, the function returns without re-encoding the transaction. If the signed raw data must reflect any modifications, verify that preserving the oldtransaction.raw_datais correct for Tron’s signing flow.
33-47: Fallback decoding approach is clear.
Decoding first as a full Tron transaction, then asRawis a sensible strategy for handling partial/fallback scenarios.packages/kos-codec/src/chains/btc/mod.rs (2)
1-3: Imports are appropriate.
No concerns with the references tobitcoin,secp256k1, orPsbt.
81-193: Broadcast flow is clear.
The approach of finalizing PSBT inputs, drawing from existing signatures, and extracting the final transaction is consistent. Just ensure the chain’s design accommodates legacy vs. segwit script finalization as you have implemented.packages/kos-mobile/src/lib.rs (7)
524-524: Function rename to be more specific about the chainThe function has been appropriately renamed from
should_sign_raw_transactiontoshould_sign_raw_transaction_klvto specify which chain the test is for, improving clarity as part of the normalization process.
554-583: Good addition for TRX chain supportThe new test function properly verifies transaction signing for the TRX chain, asserting all required fields: chain ID, sender address, raw transaction data, and signature.
585-609: Good addition for SOL chain supportThe new test function properly verifies transaction signing for the SOL chain, asserting that the raw transaction data and signature match expected values.
611-635: Good addition for SOL legacy transaction supportThis test expands coverage to include legacy transaction formats for the SOL chain, which is important for backward compatibility.
637-661: Good addition for SOL v0 transaction supportThis test adds coverage for SOL version 0 transactions, ensuring the system supports different transaction formats on the Solana chain.
716-741: Good addition for BTC chain supportThe new test function properly verifies transaction signing for the BTC chain using SegWit transaction format, asserting that the raw transaction data matches expected values.
743-767: Good addition for DASH chain supportThe test function properly verifies transaction signing for the DASH chain, ensuring cross-chain functionality is properly tested.
packages/kos-codec/src/chains/klv/models.rs (5)
8-10: Improved imports with better organizationThe refactoring of imports makes the code cleaner and more maintainable by:
- Directly importing the
protosmodule- Using a more specific path for the
simple_base64_decodefunctionThis helps support the broader effort to normalize chain-specific code.
16-16: Simplified type referencesThe type references have been simplified by removing unnecessary namespace prefixes (removing
crate::chains::klv::models::in favor of direct type names). This improves code readability and maintainability.Also applies to: 36-36, 40-40, 80-80
84-84: Added appropriate compiler annotationThe
#[allow(dead_code)]annotation was added to suppress compiler warnings for unused code in theConversionErrorenum, improving the cleanliness of the build output.
91-91: Simplified TryFrom implementationsAll
TryFromimplementations have been refactored to use direct type references rather than fully qualified paths. This is consistent with the goal of normalizing chains and using the KOS codec, making the code more maintainable and easier to read.Also applies to: 95-95, 129-129, 133-133, 172-172, 175-175, 196-196, 200-200, 212-212, 216-216, 230-230, 234-234
113-113:Details
❓ Verification inconclusive
Verify the base64 function works correctly
The code uses
simple_base64_decodefromkos::crypto::base64instead of the previous source. Make sure that this function handles the same edge cases and provides identical behavior to the previously used function.
🏁 Script executed:
#!/bin/bash # Find where the simple_base64_decode function is used elsewhere to confirm consistent usage rg -A 2 'simple_base64_decode' --type rustLength of output: 15107
Subject: Verify Consistent Handling of Base64 Edge Cases
The new implementation using
simple_base64_decodefromkos::crypto::base64is now used uniformly across modules (e.g., inwallet.rs,lib.rs,substrate/mod.rs, etc.). However, please verify that its behavior—especially for edge cases such as trailing padding and malformed input—matches that of the previous implementation. Specifically:
- Check that inputs with trailing
'='characters are trimmed and decoded correctly.- Ensure error responses for invalid input remain consistent with legacy behavior.
- Consider adding or reviewing unit tests covering these edge cases to confirm no regressions occur.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/kos-codec/src/chains/mod.rs (1)
5-9: Add or confirm test coverage for new modules
These lines introduce modules for BTC, ETH, KLV, SOL, and TRX. Confirm that each is covered by unit or integration tests to ensure reliability. If test coverage is missing, consider adding it.packages/kos-codec/src/lib.rs (1)
2-2: Consider verifying usage ofprotosmodule throughout the codebase.
If this module contains generated protobuf definitions or wrappers, ensure it's integrated into the workflow (for instance, in build scripts or code referencing). Otherwise, it might be redundant.packages/kos-codec/src/chains/eth/mod.rs (2)
1-2: Newmod models;inclusion.
Ensure relevant data structures or conversions are defined inmodelsand used here. If not needed, consider removing the empty module reference.
7-33:encode_for_signmethod for Ethereum.
- Clears signature as expected before generating the RLP.
- Sets
chain_idfor legacy transactions.- Hashes the raw RLP for signing.
Overall, it’s correct.Consider verifying whether different EVM transaction types beyond “Legacy” or “London” might require separate chain ID handling in the future.
packages/kos-mobile/src/lib.rs (1)
636-661: Version 0 Solana transaction test.
Again, thorough coverage for multiple transaction versions on Solana.You may want to expand negative tests (e.g., malformed raw data) to confirm robust error handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
packages/kos-codec/src/chains/eth/mod.rs(1 hunks)packages/kos-codec/src/chains/eth/models.rs(1 hunks)packages/kos-codec/src/chains/mod.rs(1 hunks)packages/kos-codec/src/lib.rs(3 hunks)packages/kos-mobile/src/lib.rs(3 hunks)packages/kos/src/chains/eth/mod.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/kos/src/chains/eth/mod.rs (13)
packages/kos/src/chains/klv/mod.rs (1)
get_tx_info(105-107)packages/kos/src/chains/btc/mod.rs (1)
get_tx_info(249-251)packages/kos/src/chains/sol/mod.rs (1)
get_tx_info(83-85)packages/kos/src/chains/trx/mod.rs (1)
get_tx_info(139-141)packages/kos/src/chains/substrate/mod.rs (1)
get_tx_info(203-216)packages/kos/src/chains/sui/mod.rs (1)
get_tx_info(112-114)packages/kos/src/chains/ada/mod.rs (1)
get_tx_info(166-168)packages/kos/src/chains/apt/mod.rs (1)
get_tx_info(103-105)packages/kos/src/chains/atom/mod.rs (1)
get_tx_info(157-159)packages/kos/src/chains/icp/mod.rs (1)
get_tx_info(149-151)packages/kos/src/chains/mod.rs (1)
get_tx_info(396-396)packages/kos/src/chains/bch/mod.rs (1)
get_tx_info(195-197)packages/kos/src/chains/xrp/mod.rs (1)
get_tx_info(169-171)
packages/kos-codec/src/lib.rs (1)
packages/kos-codec/src/chains/btc/mod.rs (2)
encode_for_sign(4-79)encode_for_broadcast(81-193)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (17)
packages/kos-codec/src/chains/eth/models.rs (1)
1-1: Verify import path correctness
Switching fromcrate::crypto::bignum::U256tokos::crypto::bignum::U256helps unify references underkos. Please verify that the new path is correct and doesn’t introduce any unexpected version or scoping issues.packages/kos/src/chains/eth/mod.rs (3)
2-2: Looks good
ImportingTxInfoalongsideChainand other types is consistent with the shared interface. No issues noted.
122-127: Confirm transaction hash usage
Signingtx.tx_hash.clone()aligns with the simplified logic, but please verify thattx_hashis indeed the final pre-sign hash (e.g., EIP-155 or analogous approach). If chain IDs or other fields must be included, ensure they’re embedded in this hash.
176-177: Consistent placeholder
ReturningChainError::NotSupportedforget_tx_infomatches the pattern in other chain implementations. No issues.packages/kos-codec/src/lib.rs (3)
4-4: All chain modules are imported correctly.
These imports centralize chain-specific logic for signing and broadcasting. No immediate issues here.
28-32: New chain types for signing appear consistent.
Each chain type references a correspondingencode_for_signfunction. Ensure that required chain options are provided when needed (e.g., mandatory fields inChainOptions) to avoid runtime errors.
52-56: Broadcast paths align well with the newly supported chains.
The approach of referencing each chain’s broadcast method is consistent. Double-check that any chain demanding thepublic_keyparameter is indeed provided.packages/kos-codec/src/chains/eth/mod.rs (3)
3-6: Import statements look valid.
They referenceEthereumTransactionand standard utilities for EVM. Looks good for supporting new Ethereum logic.
35-45:encode_for_broadcastsets signature and re-encodes.
The flow of assigningsignatureand generating the final RLP is logical. It’s good that the function leavestx_hashalone here, since it’s set inencode_for_sign.
47-120: Test suite thoroughly covers Legacy and London transactions.
- Checks the computed
tx_hashinencode_for_sign.- Verifies raw data and signature match expected output in
encode_for_broadcast.
The coverage is solid and consistent with changes.packages/kos-mobile/src/lib.rs (7)
524-552: KLV signing test appears to validate end-to-end logic.
- Generates the wallet from mnemonic.
- Signs the raw transaction.
- Asserts the final raw and signature.
This test is comprehensive and well-named.
553-583: TRX signing test checks Tron transaction flow.
The final raw bytes and signature are asserted. Looks solid.
Ensurechain_id = 1corresponds to Tron in your environment. Some references suggest Tron might use different chain IDs.
584-609: SOL signing test.
Handles a raw transaction, then re-serializes with an updated signature. Everything lines up with typical Solana flow.
610-635: Legacy Solana transaction test.
Nicely ensures backward-compatibility or older transaction formats.
715-741: BTC signing test.
Uses chain’s BTC ID. Confirms the final raw data is properly generated after signing.
743-767: Dash signing test closely follows the BTC pattern.
The final raw includes ECDSA signatures, which are validated. Good approach.Double-check that the partial segwit logic used in the BTC flow doesn’t inadvertently carry over to Dash.
769-791: ETH signing test with custom chain ID.
Verifies a London transaction. This test also setschain_id = 1in options while the base chain is3. Everything looks flexible.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/kos-codec/src/chains/substrate/mod.rs (1)
8-28: Robust payload sizing logic inencode_for_sign.The fallback to hash the input when size exceeds 256 bytes adds flexibility for large extrinsics. Ensure any chain with special size constraints is accounted for in this logic.
Optionally define a constant for the 256-byte threshold to clarify its significance:
+ const MAX_DIRECT_SIGN_SIZE: usize = 256; - if full_unsigned_payload_scale_bytes.len() > 256 { + if full_unsigned_payload_scale_bytes.len() > MAX_DIRECT_SIGN_SIZE {packages/kos-codec/src/chains/substrate/models.rs (5)
10-11: Consider documenting why dead code is allowed.The
#[allow(dead_code)]annotation suggests some fields might not be used in all contexts. Consider adding documentation explaining which fields might be unused and in what scenarios.
25-55: Inconsistency between comment and implementation into_bytes.The comment on line 27 mentions "version" as part of the format, but the implementation doesn't appear to include a version byte in the encoded output. Consider either updating the comment or ensuring the version is properly encoded if required.
Additionally, cloning
self.callandself.erais unnecessary asextend()can work with references:- encoded.extend(self.call.clone()); - encoded.extend(&self.era.clone()); + encoded.extend(&self.call); + encoded.extend(&self.era);
35-40: Consider extracting app_id and mode logic to a helper method.This conditional logic is duplicated in the
encode_with_signaturemethod. Consider extracting it to a helper method to improve maintainability.
47-52: Clarify the conditional logic with better documentation.The comment "Use the metadata_hash if it is not set for AVAIL transactions" is a bit confusing. Consider rephrasing to something like "Include metadata_hash only for non-AVAIL transactions (when app_id is not set)".
57-91: Ensure consistent vector extension methods inencode_with_signature.The method uses a mix of
extend(),extend_from_slice(), andpush()methods. Consider using consistent methods for better code readability:- encoded.extend_from_slice(&self.era); - encoded.extend_from_slice(&Compact(self.nonce).encode()); - encoded.extend_from_slice(&Compact(self.tip).encode()); + encoded.extend(&self.era); + encoded.extend(Compact(self.nonce).encode()); + encoded.extend(Compact(self.tip).encode());Also, when encoding with app_id:
- encoded.extend_from_slice(Compact(app_id).encode().as_slice()); + encoded.extend(Compact(app_id).encode());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/kos-codec/Cargo.toml(2 hunks)packages/kos-codec/src/chains/mod.rs(1 hunks)packages/kos-codec/src/chains/substrate/mod.rs(1 hunks)packages/kos-codec/src/chains/substrate/models.rs(1 hunks)packages/kos-codec/src/lib.rs(3 hunks)packages/kos-mobile/src/lib.rs(3 hunks)packages/kos/src/chains/substrate/mod.rs(4 hunks)packages/kos/src/chains/substrate/models.rs(0 hunks)
💤 Files with no reviewable changes (1)
- packages/kos/src/chains/substrate/models.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/kos-codec/src/lib.rs
- packages/kos-mobile/src/lib.rs
- packages/kos-codec/Cargo.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/kos/src/chains/substrate/mod.rs (1)
packages/kos/src/crypto/hash.rs (1)
blake2b_64_digest(61-68)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: format
🔇 Additional comments (11)
packages/kos-codec/src/chains/mod.rs (1)
5-10: Modules addition looks consistent.The newly introduced modules for various blockchains (btc, eth, klv, sol, substrate, trx) align with the PR objective to normalize multiple chain codecs. No immediate concerns about these declarations.
packages/kos/src/chains/substrate/mod.rs (4)
2-3: Imports look correct.The updated import statements properly include
TxInfoandblake2b_64_digest. They match the usage below where hashing is required for transaction signing.
8-8: Ensure that tx_hash is the intended data for signing.Replacing the detailed extrinsic construction with a direct hash signing approach is fine as long as
tx_hashis correctly prepared upstream (in the codec). Verify thattx_hashis the data users expect to sign.Would you like me to generate a script to check whether
tx_hashis consistently populated viaencode_for_signprior to callingsign_tx?
129-129: NotSupported error for get_tx_info.Returning
ChainError::NotSupporteddisables transaction decoding. It is consistent if all decoding is meant to be handled externally, but confirm that no code depends onget_tx_inforeturning meaningful data.
139-139: Test imports appear valid.The inclusion of
ChainOptionsfor test usage is correct, enabling custom chain option parsing viaoptions_from_browser_json.packages/kos-codec/src/chains/substrate/mod.rs (4)
1-7: Modules and imports are logically structured.Including
modelsand referencingExtrinsicPayload,KosCodedAccount, and the required hashing function keeps the codec’s scope self-contained. Good modular organization.
30-43:encode_for_broadcastis concise and clear.Encodes the signed extrinsic by combining public key, signature, and payload. Does not appear to have errors in data handling. The direct usage of
.unwrap()for decoding public_key is acceptable if upstream guarantees valid hex.
45-98:unwrap_extrinsicgracefully handles varying Substrate options.• Validates and converts array fields safely.
• Cleanly adds optional metadata fields based onchain_id.
• Approaches error conditions via meaningfulChainError::InvalidOptionsandMissingOptions.
Keep the switch onaccount.chain_idupdated if new chain variants require different logic in future.
100-481: Extensive test coverage.• Covers multiple chains (Dot, Ksm, Avail, Reef, Karura, Acala).
• Verifies correct hash generation and final raw data encoding.
• Ensures we do not regress on transaction serialization.
No obvious issues found; the coverage is commendable.packages/kos-codec/src/chains/substrate/models.rs (2)
1-7: Good use of constants for transaction format flags.The constants are well-defined with clear names that indicate their purpose in the Substrate transaction encoding process.
84-88: Good implementation of compact-encoded length prefix.The code correctly implements the Substrate transaction format by prefixing the encoded transaction with its compact-encoded length.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/kos-mobile/src/lib.rs (1)
525-855: 🛠️ Refactor suggestionAdd error case tests for transaction signing.
All the transaction signing tests cover only the happy path. Consider adding tests for error scenarios such as:
- Invalid private keys
- Invalid raw transaction data
- Missing required options for specific chains
- Mismatched chain IDs between account and options
These tests would help ensure robust error handling in the transaction signing flow.
🧹 Nitpick comments (5)
packages/kos/src/chains/util.rs (1)
76-95: Good test coverage for new serialization utilities.The tests validate both the happy path and binary roundtrip integrity. Consider adding tests for edge cases like empty vectors and single-byte vectors to ensure complete coverage.
#[cfg(test)] mod tests { use super::*; #[test] fn test_hex_string_to_vec() { let hex_str = "0x1234567890abcdef"; let result = hex_string_to_vec(hex_str).unwrap(); assert_eq!(result, vec![0x12, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef]); } #[test] fn test_hex_strings_to_bytes() { let hex_strings = vec![hex::decode("db41e41de474e2cb6d997ae5aa5de9aa81512a19d1337881363a3c481431935992a118ba863b6d00612c638b5caf7bac65cb2cf31a7d30f9c5473fcb97bf620bc006bf0760963c13c1c1478adbc326b96338060f03487ebd1c3b261dbccd8daf").unwrap(), hex::decode("db41e41de8163a3c481431935992a118ba863b6d00612c638b5caf7bac65cb2c7ebd1c3b261dbccd8daf").unwrap()]; let result = byte_vectors_to_bytes(&hex_strings); let decoded = bytes_to_byte_vectors(result).unwrap(); assert_eq!(hex_strings, decoded); } + + #[test] + fn test_edge_cases() { + // Test empty vector + let empty: Vec<Vec<u8>> = vec![]; + let serialized = byte_vectors_to_bytes(&empty); + let deserialized = bytes_to_byte_vectors(serialized).unwrap(); + assert_eq!(empty, deserialized); + + // Test vector with empty byte arrays + let empty_arrays = vec![vec![], vec![]]; + let serialized = byte_vectors_to_bytes(&empty_arrays); + let deserialized = bytes_to_byte_vectors(serialized).unwrap(); + assert_eq!(empty_arrays, deserialized); + + // Test single-byte arrays + let single_bytes = vec![vec![1], vec![2]]; + let serialized = byte_vectors_to_bytes(&single_bytes); + let deserialized = bytes_to_byte_vectors(serialized).unwrap(); + assert_eq!(single_bytes, deserialized); + } }packages/kos/src/chains/icp/mod.rs (1)
84-96: Improved transaction signing using binary serialization.The implementation has been simplified to directly process binary data without JSON serialization/deserialization. The code properly maintains security by zeroing out private key material after use.
Consider adding more context to the error handling to aid in debugging:
- let icp_hashes = bytes_to_byte_vectors(tx.tx_hash.clone())?; + let icp_hashes = bytes_to_byte_vectors(tx.tx_hash.clone()) + .map_err(|e| ChainError::InvalidData(format!("Failed to decode ICP transaction hash: {:?}", e)))?;packages/kos-codec/src/chains/icp/mod.rs (2)
19-26: Optimize the hash processing loop.The current implementation handles each hash individually. Consider using a more efficient approach by directly mapping the vector.
- let icp_hashes = container.hashes; - - let mut hashes = Vec::new(); - - for hash_hex in icp_hashes { - hashes.push(hex::decode(hash_hex).unwrap()); - } + let hashes = container.hashes + .into_iter() + .map(|hash_hex| hex::decode(hash_hex).map_err(|_| ChainError::DecodeHash)) + .collect::<Result<Vec<Vec<u8>>, ChainError>>()?;
32-41: Consider strengthening the signature encoding logic.The current implementation directly maps signatures to hex strings without validation. Consider adding checks for signature validity or handling potential corner cases.
For example, you might want to check if signatures are empty or have expected lengths before encoding them. This would provide better error messages and prevent issues downstream.
packages/kos-mobile/src/lib.rs (1)
554-855: Consider refactoring test code to reduce duplication.The test cases for different chains follow a very similar pattern. Consider extracting common testing logic into a helper function to reduce code duplication and make future maintenance easier.
You could create a parametrized test helper like:
fn test_sign_transaction( chain_id: u32, raw_tx: &str, options: Option<TransactionChainOptions>, expected_sender: &str, expected_signature_prefix: &str, // Or use a validation function ) { let account = generate_wallet_from_mnemonic( "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about".to_string(), chain_id, 0, false, ) .unwrap(); let transaction = sign_transaction(account, raw_tx.to_string(), options).unwrap(); assert_eq!(transaction.chain_id, chain_id, "The chain_id doesn't match"); assert_eq!( transaction.sender, expected_sender, "The sender doesn't match" ); assert!( transaction.signature.starts_with(expected_signature_prefix), "The signature doesn't match expected pattern" ); }Then use it in individual tests with specific parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/kos-codec/src/chains/icp/mod.rs(1 hunks)packages/kos-codec/src/chains/klv/mod.rs(1 hunks)packages/kos-codec/src/chains/mod.rs(1 hunks)packages/kos-codec/src/lib.rs(3 hunks)packages/kos-mobile/src/lib.rs(4 hunks)packages/kos/Cargo.toml(0 hunks)packages/kos/src/chains/icp/mod.rs(3 hunks)packages/kos/src/chains/mod.rs(1 hunks)packages/kos/src/chains/util.rs(1 hunks)
💤 Files with no reviewable changes (1)
- packages/kos/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kos-codec/src/chains/klv/mod.rs
- packages/kos-codec/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/kos-mobile/src/lib.rs (2)
packages/kos-codec/src/chains/eth/models.rs (1)
decode(26-41)packages/kos-codec/src/chains/sol/models.rs (2)
decode(45-165)decode(221-241)
🔇 Additional comments (16)
packages/kos-codec/src/chains/mod.rs (1)
5-11: Excellent module organization to support multiple blockchain types.The addition of these new modules (
btc,eth,icp,klv,sol,substrate,trx) follows a consistent pattern with existing declarations. This modular approach provides a clean separation of encoding logic for each blockchain, making the codebase more maintainable.packages/kos/src/chains/mod.rs (1)
20-20: Changed import to remove tiny_json_rs dependency.The import now directly includes
FromUtf8Errorfromalloc::stringinstead of relying on tiny_json_rs. This aligns with the PR's objective to normalize chains to use the kos codec and remove JSON serialization from core chain implementations.packages/kos/src/chains/util.rs (2)
25-39: Well-implemented binary serialization utility.This utility function efficiently serializes a vector of byte vectors using a compact binary format with explicit length prefixes. The approach is more performant than JSON serialization and well-suited for blockchain transaction data.
40-74: Robust deserialization with proper error handling.The implementation includes thorough bounds checking to prevent buffer overruns, with appropriate error handling for malformed inputs. This is crucial for securely processing potentially untrusted blockchain data.
packages/kos/src/chains/icp/mod.rs (2)
1-1: Updated imports to use new serialization utilities.The imports now include the necessary utility functions for binary serialization, aligning with the PR's objective to normalize chain transaction encoding.
130-130: Improved error handling clarity.Replaced
todo!()placeholder with an explicit error type, which is more informative for API consumers. This aligns with other chain implementations that have similarly updated theirget_tx_infomethods.packages/kos-mobile/src/lib.rs (10)
309-310: LGTM! Extracting signature from encoded transaction.This change ensures consistent signature extraction across all chains, aligning with the kos-codec pattern.
554-584: LGTM! Comprehensive TRX transaction signing test.The test properly validates the TRX transaction signing flow, ensuring compatibility with the new codec implementation.
585-610: LGTM! SOL transaction signing test.The test properly validates the Solana transaction signing flow with standard transaction format.
611-636: LGTM! SOL legacy transaction signing test.The test properly validates the Solana legacy transaction signing flow.
637-662: LGTM! SOL v0 transaction signing test.The test properly validates the Solana v0 transaction signing flow.
716-742: LGTM! BTC transaction signing test.The test properly validates the Bitcoin transaction signing flow, including correct handling of input scripts and amounts.
743-768: LGTM! DASH transaction signing test.The test properly validates the DASH transaction signing flow, which uses similar pattern to BTC but with different inputs.
770-792: LGTM! ETH transaction signing test.The test properly validates Ethereum transaction signing with appropriate chain ID options.
793-834: LGTM! Substrate/DOT transaction signing test.The test properly validates Substrate transaction signing with appropriate options for this complex chain type.
835-855: LGTM! ICP transaction signing test.The test properly validates ICP transaction signing, directly testing the new codec module added in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kos-hardware/src/lib.rs (1)
339-380: Improved transaction data serialization with proper type handlingThe refactoring of
rs_tx_info_to_jsonenhances the code by:
- Using a structured
TransactionDetailsstruct with derived serialization- Creating a local
TransactionTypeenum with proper mapping from the originalTxType- Implementing custom serialization for the enum to ensure consistent JSON output
This approach provides better type safety and makes the JSON serialization process more maintainable and explicit compared to direct encoding.
However, consider adding a comment explaining why this local serialization approach is used instead of implementing serialization directly on the original types, which would help future maintainers understand the design decision.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/kos-hardware/src/lib.rs(2 hunks)
🔇 Additional comments (1)
packages/kos-hardware/src/lib.rs (1)
3-3: Properly organized imports for JSON serializationThe added imports set up the foundation for the refactored JSON serialization strategy. Using explicit imports from
tiny_json_rsmakes dependencies clear and improves maintainability by showing exactly which components are needed.Also applies to: 14-19
…S-8266-kos-rs-normalize-chains-to-use-kos-codec-for-all-chains
gustavocbritto
deleted the
KRS-8266-kos-rs-normalize-chains-to-use-kos-codec-for-all-chains
branch
3 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores