feat: implement comprehensive keyring improvements and bug fixes#235
Merged
sidhujag merged 107 commits intosyscoin:mainfrom Jul 25, 2025
Merged
feat: implement comprehensive keyring improvements and bug fixes#235sidhujag merged 107 commits intosyscoin:mainfrom
sidhujag merged 107 commits intosyscoin:mainfrom
Conversation
added 30 commits
June 2, 2025 18:08
### Major Features & Fixes: - Fix network synchronization logic for HD signer recreation across UTXO networks - Remove unnecessary crypto mocks to enable real deterministic operations - Implement proper SLIP44 parameter handling for multi-network support - Fix state contamination in vault storage causing wrong seed phrase usage ### Network Synchronization Enhancements: - Add `shouldUpdateHDSigner` method with proper network comparison logic - Fix detection of testnet/mainnet status, SLIP44/chainId, and blockbook URL mismatches - Remove Syscoin special case in getSysRpc for consistent network configuration - Handle custom UTXO networks and edge cases (Bitcoin ↔ Syscoin switches) ### Crypto Operations Improvements: - Remove MockHDSigner to use real syscoinjs-lib HDSigner functionality - Enable real deterministic crypto operations (bip39, hdkey, bip84) - Fix SLIP44 parameter passing in getSyscoinSigners - Correct SLIP44 values to BIP44 standards (Syscoin testnet: 5700 → 1) ### Test Infrastructure Overhaul: - Implement dynamic vault mocking with proper state tracking - Remove 200+ lines of unnecessary mock code - Fix test isolation with proper beforeEach cleanup - Update test expectations to match real crypto derivation - Maintain only network-dependent mocks (fetchBackendAccount, etc.) ### Bug Fixes: - Fix state contamination bug where createKeyringVault used wrong seed phrases - Resolve HD signer recreation issues for network parameter changes - Fix account derivation test failures due to mocked vs real crypto differences - Correct network configuration for Bitcoin networks in syscoinjs-lib integration ### Production Improvements: - Enhanced defensive programming for missing network properties - Better error handling for HD signer initialization failures - Improved network validation and parameter consistency - Graceful fallbacks instead of throwing errors for edge cases ### Test Results: - Increase passing tests from 105/110 to 110/110 (100% pass rate) - Enable real cryptographic operations throughout test suite - Maintain comprehensive coverage while reducing mock complexity - Fix 5 previously failing tests related to network sync and account derivation ### Breaking Changes: - None - all changes maintain backward compatibility - Improved error handling may surface previously hidden issues ### Dependencies: - Requires updated syscoinjs-lib with SLIP44 parameter fix - Compatible with existing network configurations - Maintains compatibility with Trezor/Ledger hardware wallets
avoid fetching needlessly for every account
also check against pwd which wasnt validated when calling getPrivateKeyByAccountId
work with trezor/ledger hd wallets in a unified way
fee rate was divided by 10^8 when its already coming in at sys/byte making it incredibly small, the fee estimate from blockbook is sat/kb so we divide by 10^2 not 10^8. Rpc caching helps reduce extraneous calls to the same backend many times quickly. Explorer urls had extra slashes when we add them dynamically through the code
…let support - Add new derivation-paths utility module for dynamic BIP44/84 path generation - Remove syscoin-simplified.ts and consolidate transaction logic in syscoin.ts - Fix typo: 'sucess' → 'success' in keyring manager return values - Update Ledger and Trezor integrations to use dynamic coin detection - Enhance address validation with network-aware coin support - Improve transaction creation with better PSBT handling - Update dependencies in yarn.lock files across packages - Remove hardcoded derivation paths in favor of dynamic generation - Fix fee estimation calls by removing undefined parameter
- Remove coinselectsyscoin dependency for manual fee calculation - Update syscoinjs-lib to ^1.0.234, syscointx-js to ^1.0.112, coinselectsyscoin to ^1.1.3 - Simplify fee estimation by using syscoinjs-lib's native fee calculation - Add proper structured error handling with ISyscoinTransactionError interface - Add isMax parameter support for "send max" functionality - Remove complex calculateTransactionSize method in favor of library-provided fees - Fix getTransactionPSBT to return both psbt and fee objects - Update test mocks to match new API structure This modernizes the transaction handling to rely on the updated syscoinjs-lib for fee calculation rather than manual computation, improving accuracy and reducing complexity.
- Add dedicated caches for Ethereum and UTXO accounts to prevent cross-chain interference - Improve chain type switching logic with proper account regeneration - Optimize balance fetching by removing automatic updates (delegated to Pali) - Add updateUTXOAddressFormats() to update addresses without recreating HD signers - Fix account retrieval from cache when switching networks - Improve error handling for mnemonic decryption and account operations - Persist account caches to encrypted storage for session continuity This change ensures proper account isolation between different chain types and improves performance by reducing unnecessary HD signer recreations and network calls.
added 12 commits
July 20, 2025 10:45
…ion Issue Fixed the bug where importing a Trezor account for EVM networks incorrectly identified it as "Syscoin Legacy": Root Cause: For EVM networks, Trezor's getAccountInfo returns the Ethereum address as the descriptor, but the code was incorrectly using this as the address without proper validation. Solution: Modified _createTrezorAccount to properly get the Ethereum address using trezorSigner.getAddress() for EVM networks This ensures the address is properly formatted as an Ethereum address (starts with 0x) Updated tests to include the necessary mocks for the new flow The fixes ensure that: Ledger devices automatically reconnect when connection is lost, preventing the "signTransaction is undefined" error Trezor accounts imported for EVM networks are correctly identified as Ethereum accounts, not "Syscoin Legacy" Found other issues related to ledger account indexes not using getNextAccountId when importing which wasn't previously working properly with account index gaps. Gaps can be created when you remove accounts which aren't the last. All tests are passing, and the implementation follows best practices for hardware wallet integration.
verify utxo for trezor, reconnect ledger, proper signing based on slip44, import fix for trezor for evm
…ger getutxoaddress (for verification) lookup coin from appname to support any utxo network from our coins.ts list. Remove unused getUtxos (fetch from backend)
Member
Author
|
@cursor review |
🚨 Bugbot couldn't runSomething went wrong. Try again by commenting "Cursor review" or "bugbot run", or contact support (requestId: serverGenReqId_e9effe4c-f2b5-4627-8c8a-5c419bbba1f6). |
added 11 commits
July 23, 2025 19:24
…ry logic - Add HardwareWalletManager for centralized connection management - Implement connection pooling for Ledger devices - Add real-time device status monitoring with 5s intervals - Implement exponential backoff retry logic (3 retries, 1s-10s delays) - Add automatic cleanup of idle connections (5min timeout) - Handle device disconnections gracefully with proper event handling - Update hardware wallet dependencies to latest stable versions - @ledgerhq/hw-app-eth: ^6.34.8 -> ^6.45.12 - @ledgerhq/hw-transport-webhid: ^6.27.19 -> ^6.30.4 - ledger-bitcoin: ^0.2.3 -> ^0.4.0 - @trezor/connect-webextension: ^9.3.0 -> ^9.6.2 - Refactor LedgerKeyring for improved reliability - Integrate with HardwareWalletManager for all operations - Fix semantic issue with EVM derivation paths (accountIndex vs addressIndex) - Add executeWithRetry wrapper to all public methods - Fix getEvmAddressAndPubKey to properly return publicKey - Refactor TrezorKeyring for consistency - Integrate with HardwareWalletManager for initialization - Add executeWithRetry wrapper to all public methods - Remove duplicate initialization checks - Improve error propagation for retry mechanism - Clean up unused code - Remove unused ISysTrezorController stub from pali-wallet - Remove unused getTransport method from LedgerKeyring - Remove unused Signature interface from TrezorKeyring BREAKING CHANGE: LedgerKeyring and TrezorKeyring now require proper cleanup via destroy() method to close connections and stop monitoring
…th for ledger device interaction
trezor was inconsistent with path scheme. getAddress used only by evm now needs to swap accountIndex and 0 to derive address properly for account. Other placements of setHdPath's just removed duplicate code. Removed getAccountDerivationPath as it was incorrectly referring to 4 levels deep instead of 3, use setHdPath instead (in getAccountInfo)
don't call getAccountInfo on eth, just use getAddress, dispose connection properly and reduce retries, check for connection and stop retrying
…ublicKey so device is ready
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Major Features & Fixes:
Network Synchronization Enhancements:
shouldUpdateHDSignermethod with proper network comparison logicCrypto Operations Improvements:
Test Infrastructure Overhaul:
Bug Fixes:
Production Improvements:
Test Results:
Breaking Changes:
Dependencies: