feat(zetaclient): Revamp TSS package #3170
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request encompass a comprehensive revamp of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TSS
participant Logger
Client->>TSS: Sign request
TSS->>Logger: Log request details
TSS->>TSS: Validate input
TSS->>Client: Return signature
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
swift1337
added
UPGRADE_LIGHT_TESTS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3170 +/- ##
===========================================
- Coverage 62.74% 62.43% -0.32%
===========================================
Files 425 428 +3
Lines 30231 30404 +173
===========================================
+ Hits 18969 18982 +13
- Misses 10425 10576 +151
- Partials 837 846 +9
|
swift1337
force-pushed
the
feat/zetaclient/tss-improvements
branch
from
7c8af2b to
cdd27a8
Compare
swift1337
changed the title
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14567965 | Triggered | Generic Password | c5b10c6 | cmd/zetaclientd/start.go | View secret |
| 14567965 | Triggered | Generic Password | ade21aa | cmd/zetaclientd/start.go | View secret |
| 14567965 | Triggered | Generic Password | c5b10c6 | cmd/zetaclientd/start.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
swift1337
force-pushed
the
feat/zetaclient/tss-improvements
branch
from
dd8d09b to
b5c367a
Compare
swift1337
added
UPGRADE_LIGHT_TESTS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Outside diff range and nitpick comments (72)
zetaclient/chains/bitcoin/observer/rpc_status.go (1)
Line range hint
32-45: Add test coverage for the modified TSS address retrieval.The static analysis indicates that the modified TSS address retrieval logic lacks test coverage. This is critical infrastructure code that should be thoroughly tested.
Would you like me to help generate comprehensive test cases for this modified functionality? The test should cover:
- Successful TSS BTC address retrieval
- Error handling for TSS PubKey failures
- Error handling for AddressBTC failures
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: zetaclient/chains/bitcoin/observer/rpc_status.go#L32
Added line #L32 was not covered by testszetaclient/tss/crypto_test.go (2)
13-15: Add function documentation and consider test isolationThe test function should include documentation describing its purpose and test cases. Also, consider isolating the cosmos configuration setup.
Apply this diff to improve the test structure:
+// TestPubKey verifies the functionality of public key operations including: +// - Bech32 string parsing +// - Address derivation for EVM and BTC chains +// - ECDSA public key conversion func TestPubKey(t *testing.T) { + t.Parallel() // Allow parallel execution cmd.SetupCosmosConfig()
21-44: Document test vectors and enhance error handling coverageThe test case is well-structured with the AAA pattern, but could benefit from:
- Documentation of test vectors
- Error case handling for BTC address generation
- Validation of the ECDSA public key components
Apply this diff to enhance the test:
t.Run("Valid", func(t *testing.T) { // ARRANGE + // Test vector representing a known valid public key and its derived addresses + const ( + samplePubKey = `zetapub1addwnpepqtadxdyt037h86z60nl98t6zk56mw5zpnm79tsmvspln3hgt5phdc79kvfc` + expectedEVM = "0x70e967acfcc17c3941e87562161406d41676fd83" + expectedBTC = "bc1qm24wp577nk8aacckv8np465z3dvmu7ry45el6y" + ) - const sample = `zetapub1addwnpepqtadxdyt037h86z60nl98t6zk56mw5zpnm79tsmvspln3hgt5phdc79kvfc` // ACT - pk, err := NewPubKeyFromBech32(sample) + pk, err := NewPubKeyFromBech32(samplePubKey) // ASSERT require.NoError(t, err) assert.NotEmpty(t, pk) addrEVM := pk.AddressEVM() + // Test BTC address generation for both mainnet and testnet addrBTC, err := pk.AddressBTC(chains.BitcoinMainnet.ChainId) require.NoError(t, err) + + // Verify invalid chain ID handling + _, err = pk.AddressBTC(999999) + require.Error(t, err) - assert.Equal(t, sample, pk.Bech32String()) - assert.Equal(t, "0x70e967acfcc17c3941e87562161406d41676fd83", strings.ToLower(addrEVM.Hex())) - assert.Equal(t, "bc1qm24wp577nk8aacckv8np465z3dvmu7ry45el6y", addrBTC.String()) + assert.Equal(t, samplePubKey, pk.Bech32String()) + assert.Equal(t, expectedEVM, strings.ToLower(addrEVM.Hex())) + assert.Equal(t, expectedBTC, addrBTC.String()) // Check that NewPubKeyFromECDSA works pk2, err := NewPubKeyFromECDSA(*pk.ecdsaPubKey) require.NoError(t, err) require.Equal(t, pk.Bech32String(), pk2.Bech32String()) + + // Verify ECDSA public key components + require.True(t, pk.ecdsaPubKey.Curve.IsOnCurve(pk.ecdsaPubKey.X, pk.ecdsaPubKey.Y)) })zetaclient/tss/config_test.go (1)
16-23: Consider adding more edge cases to the test suite.The current test cases cover basic scenarios well. Consider adding the following cases to improve test coverage:
- Large number of files (e.g., 100) to verify performance with scale
- Invalid directory path to verify error handling
- Directory with mixed valid and invalid files to verify robustness
}{ {name: "2 keyshare files", n: 2}, {name: "10 keyshare files", n: 10}, {name: "No keyshare files", n: 0}, + {name: "Large number of files", n: 100}, }cmd/zetaclientd/utils.go (1)
110-120: Add documentation and error handling while preserving the efficient implementation.The current implementation efficiently uses pre-allocation and functional filtering. However, it could benefit from better documentation and error handling.
+// btcChainIDsFromContext returns a slice of chain IDs for all Bitcoin chains in the AppContext. +// Returns an empty slice if no Bitcoin chains are found or if the context is nil. func btcChainIDsFromContext(app *zctx.AppContext) []int64 { + if app == nil { + return []int64{} + } var ( btcChains = app.FilterChains(zctx.Chain.IsBitcoin) btcChainIDs = make([]int64, len(btcChains)) ) for i, chain := range btcChains { + if chain == nil { + continue + } btcChainIDs[i] = chain.ID() } return btcChainIDs }zetaclient/testutils/mocks/tss.go (4)
17-17: Consider using a more descriptive type alias name.The type alias
testcould be more descriptive. Consider usingTestingTorTestContextto better convey its purpose.-type test = require.TestingT +type TestingT = require.TestingT
48-80: Improve error handling and control flow in FakePubKey method.The method has several opportunities for improvement:
- The return
nilafterFailNow()is unreachable- The nested if statements could be flattened for better readability
Consider this refactoring:
func (tss *TSS) FakePubKey(pk any) *TSS { if pk == nil { tss.fakePubKey = nil return tss } if zpk, ok := pk.(zetatss.PubKey); ok { tss.fakePubKey = &zpk return tss } raw, ok := pk.(string) if !ok { + require.FailNowf(tss.t, "invalid type", "invalid type for fake pub key (%v)", pk) + } - require.True(tss.t, ok, "invalid type for fake pub key (%v)", pk) if strings.HasPrefix(raw, "zetapub") { zpk, err := zetatss.NewPubKeyFromBech32(raw) require.NoError(tss.t, err) tss.fakePubKey = &zpk return tss } if strings.HasPrefix(raw, "0x") { zpk, err := zetatss.NewPubKeyFromECDSAHexString(raw) require.NoError(tss.t, err) tss.fakePubKey = &zpk return tss } - tss.t.Errorf("invalid fake pub key format: %s", raw) - tss.t.FailNow() - - return nil + require.FailNowf(tss.t, "invalid format", "invalid fake pub key format: %s", raw) + return tss // unreachable but keeps compiler happy }
82-89: Document ignored parameters in Sign method.The method ignores several parameters which should be documented for clarity.
-func (tss *TSS) Sign(_ context.Context, digest []byte, _, _ uint64, _ int64) ([65]byte, error) { +func (tss *TSS) Sign( + _ context.Context, // unused in mock + digest []byte, + _ uint64, // chainID unused in mock + _ uint64, // blockHeight unused in mock + _ int64, // timestamp unused in mock +) ([65]byte, error) {
91-111: Clarify backwards compatibility comment and document ignored parameters.The comment about backwards compatibility needs more context, and ignored parameters should be documented.
-// just for backwards compatibility (ideally we should remove this) +// The paused state check is maintained for backwards compatibility with existing tests. +// TODO: Consider removing this check in a future cleanup. func (tss *TSS) SignBatch( - _ context.Context, + _ context.Context, // unused in mock digests [][]byte, - _, _ uint64, - _ int64, + _ uint64, // chainID unused in mock + _ uint64, // blockHeight unused in mock + _ int64, // timestamp unused in mock ) ([][65]byte, error) {zetaclient/tss/config.go (2)
16-20: Add documentation for exported constantsAs these constants are part of the public API, they should be documented with clear descriptions of their purpose and any constraints.
Apply this diff:
const ( + // Port defines the default TSS server port Port = 6668 + // Version represents the TSS protocol version following semantic versioning Version = "0.14.0" + // Algo specifies the signature algorithm used for TSS operations Algo = tsscommon.ECDSA )
1-116: Consider architectural improvementsThe file demonstrates good separation of concerns and error handling. However, consider these architectural improvements:
- Move constants to a dedicated
constants.gofile as they might grow over time- Consider introducing a configuration struct to encapsulate TSS-related settings
- Add interfaces for better testability and mock generation
Example configuration struct:
type TSSConfig struct { Port int Version string Algo string PreParamsPath string KeysPath string } func NewTSSConfig() *TSSConfig { return &TSSConfig{ Port: Port, Version: Version, Algo: Algo, } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-26: zetaclient/tss/config.go#L23-L26
Added lines #L23 - L26 were not covered by tests
[warning] 28-31: zetaclient/tss/config.go#L28-L31
Added lines #L28 - L31 were not covered by tests
[warning] 33-33: zetaclient/tss/config.go#L33
Added line #L33 was not covered by tests
[warning] 39-42: zetaclient/tss/config.go#L39-L42
Added lines #L39 - L42 were not covered by tests
[warning] 44-49: zetaclient/tss/config.go#L44-L49
Added lines #L44 - L49 were not covered by tests
[warning] 51-54: zetaclient/tss/config.go#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 56-56: zetaclient/tss/config.go#L56
Added line #L56 was not covered by tests
[warning] 68-69: zetaclient/tss/config.go#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 90-91: zetaclient/tss/config.go#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 95-96: zetaclient/tss/config.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 103-104: zetaclient/tss/config.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 111-113: zetaclient/tss/config.go#L111-L113
Added lines #L111 - L113 were not covered by testszetaclient/testutils/constant.go (1)
Line range hint
13-24: Enhance constant organization and documentationWhile the constants are functionally grouped, there are opportunities to improve organization and documentation:
- The TSS-related constants could be grouped under a clear section header
- Consider maintaining consistent alignment for related constants
- Each group should have a clear section comment explaining its purpose
Consider restructuring as follows:
+// TSS Constants +// These constants define the TSS addresses and public keys used across different networks + // TSSAddressEVMMainnet is the Ethereum TSS address for mainnet operations -TSSAddressEVMMainnet = "0x70e967acFcC17c3941E87562161406d41676FD83" -TSSAddressBTCMainnet = "bc1qm24wp577nk8aacckv8np465z3dvmu7ry45el6y" -TSSPubKeyMainnet = "zetapub1addwnpepqtadxdyt037h86z60nl98t6zk56mw5zpnm79tsmvspln3hgt5phdc79kvfc" +TSSAddressEVMMainnet = "0x70e967acFcC17c3941E87562161406d41676FD83" +TSSAddressBTCMainnet = "bc1qm24wp577nk8aacckv8np465z3dvmu7ry45el6y" +TSSPubKeyMainnet = "zetapub1addwnpepqtadxdyt037h86z60nl98t6zk56mw5zpnm79tsmvspln3hgt5phdc79kvfc"🧰 Tools
🪛 Gitleaks (8.21.1)
16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
zetaclient/chains/solana/signer/withdraw.go (1)
Line range hint
93-97: Consider implementing dynamic compute budget optimization.The current implementation uses a fixed fee of 5K lamports. Consider implementing dynamic compute budget and priority fees based on network conditions for optimal transaction processing.
Key considerations for implementation:
- Monitor network congestion
- Adjust compute unit limit based on instruction complexity
- Implement priority fee calculation based on urgency
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: zetaclient/chains/solana/signer/withdraw.go#L46
Added line #L46 was not covered by testszetaclient/chains/bitcoin/signer/signer_keysign_test.go (3)
38-40: Enhance test setup with additional assertionsWhile the changes improve the initialization pattern, consider adding assertions to validate the TSS signer's state:
suite.testSigner = mocks.NewTSSFromPrivateKey(suite.T(), privateKey.ToECDSA()) addr, err := suite.testSigner.PubKey().AddressBTC(chains.BitcoinTestnet.ChainId) suite.Require().NoError(err) +suite.Require().NotEmpty(addr) +suite.Require().True(len(addr) > 0) + +// Verify the signer is properly initialized +pubKey := suite.testSigner.PubKey() +suite.Require().NotNil(pubKey) suite.T().Logf("segwit addr: %s", addr)
Line range hint
150-160: Improve error handling and remove magic numbersThe signature generation and validation could be enhanced:
- Add granular error handling for signature component extraction
- Replace magic numbers in the Sign method with named constants
- Add signature validation before using it
+const ( + SignAttempts = 10 + SignTimeout = 10 +) + -sig65B, err := tss.Sign(ctx, witnessHash, 10, 10, 0) +sig65B, err := tss.Sign(ctx, witnessHash, SignAttempts, SignTimeout, 0) +if err != nil { + return "", fmt.Errorf("failed to sign witness hash: %w", err) +} + R := &btcec.ModNScalar{} -R.SetBytes((*[32]byte)(sig65B[:32])) +if overflow := R.SetBytes((*[32]byte)(sig65B[:32])); overflow { + return "", fmt.Errorf("R component overflow") +} + S := &btcec.ModNScalar{} -S.SetBytes((*[32]byte)(sig65B[32:64])) +if overflow := S.SetBytes((*[32]byte)(sig65B[32:64])); overflow { + return "", fmt.Errorf("S component overflow") +} + sig := btcecdsa.NewSignature(R, S) + +// Validate signature before using it +if !sig.Verify(witnessHash, tss.PubKey()) { + return "", fmt.Errorf("invalid signature") +}
161-163: Consider adding public key validationThe witness construction could benefit from additional validation:
-pkCompressed := tss.PubKey().Bytes(true) +pubKey := tss.PubKey() +if pubKey == nil { + return "", fmt.Errorf("nil public key") +} +pkCompressed := pubKey.Bytes(true) +if len(pkCompressed) == 0 { + return "", fmt.Errorf("invalid compressed public key") +} txWitness := wire.TxWitness{append(sig.Serialize(), byte(hashType)), pkCompressed}zetaclient/chains/solana/signer/withdraw_spl.go (1)
Line range hint
134-137: Consider implementing the compute budget optimization.The TODO comment references issue #2599 regarding compute budget optimization. This could impact transaction efficiency and costs.
Consider implementing the compute budget optimization by:
- Determining optimal compute unit limits based on transaction complexity
- Implementing dynamic compute unit pricing based on network conditions
- Adding configuration options for these parameters
Would you like assistance in implementing these optimizations?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: zetaclient/chains/solana/signer/withdraw_spl.go#L60
Added line #L60 was not covered by testszetaclient/chains/evm/signer/sign.go (3)
Line range hint
109-122: Consider adding transaction metadata comments.The
SignCancelmethod performs a critical operation (nonce increment) by sending a self-transaction. Consider adding a comment in the transaction data to indicate this is an intentional cancellation transaction.func (signer *Signer) SignCancel(ctx context.Context, txData *OutboundData) (*ethtypes.Transaction, error) { txData.gas.Limit = evm.EthTransferGasLimit tx, _, _, err := signer.Sign( ctx, - nil, + []byte("nonce-increment-cancellation"), signer.TSS().PubKey().AddressEVM(), zeroValue, // zero out the amount to cancel the tx txData.gas, txData.nonce, txData.height, )
Line range hint
1-236: Standardize error handling patterns.The file uses two different error wrapping patterns:
errors.Wrap(err, "message")fmt.Errorf("message: %w", err)Consider standardizing on one approach throughout the file for consistency. The
errors.Wrappattern is more concise and provides the same functionality.
Line range hint
109-122: Review gas limit settings.Both
SignCancelandSignGasWithdrawuse the sameEthTransferGasLimit. While this works, consider:
SignCancelcould potentially use a lower gas limit as it's a simpler operation.- Other methods might benefit from explicit gas limits based on their complexity.
Consider defining operation-specific gas limits in a configuration file or constants, taking into account the complexity of each operation.
Also applies to: 124-141
zetaclient/tss/service_test.go (3)
32-115: Consider enhancing test coverage with additional scenarios.While the current test coverage is good, consider adding:
- Negative test cases for Sign and SignBatch (e.g., invalid block height, context cancellation)
- Concurrent signing tests to verify thread safety
- Documentation for each test scenario using Go's test documentation format
Example test documentation format:
t.Run("Sign/InvalidBlockHeight", func(t *testing.T) { // Given a TSS service and an invalid block height // When attempting to sign a message // Then it should return an appropriate error })
239-241: Enhance key generation robustness in keySignerMock.The current key generation method might produce collisions if messages contain the string representation of the delimiter.
Consider using a more robust serialization:
func (m *keySignerMock) key(req keysign.Request) string { - return fmt.Sprintf("%s-%d:[%+v]", req.PoolPubKey, req.BlockHeight, req.Messages) + return fmt.Sprintf("%s:%d:%x", req.PoolPubKey, req.BlockHeight, + crypto.Keccak256([]byte(strings.Join(req.Messages, ",")))) }
202-229: Add signature validation in mock sign method.The mock sign method should validate the generated signatures to ensure they match the input messages.
Add validation after signature generation:
func (m *keySignerMock) sign(req keysign.Request) keysign.Response { // ... existing code ... + // Validate signatures + for _, sig := range signatures { + digest, _ := base64DecodeString(sig.Msg) + sigBytes := make([]byte, 65) + r, _ := base64DecodeString(sig.R) + s, _ := base64DecodeString(sig.S) + v, _ := base64DecodeString(sig.RecoveryID) + copy(sigBytes[0:32], r) + copy(sigBytes[32:64], s) + copy(sigBytes[64:], v) + + pubkey, err := crypto.Ecrecover(digest, sigBytes) + require.NoError(m.t, err) + require.Equal(m.t, crypto.FromECDSAPub(&m.privateKey.PublicKey), pubkey) + } return keysign.Response{ Signatures: signatures, Status: tsscommon.Success, } }zetaclient/chains/evm/signer/sign_test.go (1)
18-19: LGTM! Consider extracting the TSS mock setup.The simplified TSS mock instantiation is consistently applied across all test functions. However, since this setup is repeated in multiple test functions, consider extracting it into a helper function to reduce duplication.
+func setupTSSAndSigner(t *testing.T) (*mocks.TSS, *EvmSigner, error) { + tss := mocks.NewTSS(t) + evmSigner, err := getNewEvmSigner(t, tss) + return tss, evmSigner, err +}Also applies to: 102-103, 140-141, 178-179, 215-216
zetaclient/chains/evm/signer/signer_test.go (3)
39-44: Document the testing.T parameter and mock behaviorThe function signature change to include
*testing.Tis good, but the function could benefit from improved documentation.Add documentation explaining the testing context usage:
// getNewEvmSigner creates a new EVM chain signer for testing +// +// Parameters: +// - t: testing context for proper test failure reporting and mock setup +// - tss: optional TSS signer interface, if nil a new mock TSS will be created
70-70: Consider explicit mock behavior setupThe mock TSS creation could be more explicit about its behavior expectations.
Consider setting up explicit mock behavior:
- tss = mocks.NewTSS(t) + mockTSS := mocks.NewTSS(t) + // Set up expected behavior for common operations + mockTSS.On("GetZetaConnectorAddress").Return(ConnectorAddress) + tss = mockTSS
Line range hint
1-270: Consider future improvements to test infrastructureWhile not directly related to the current changes, consider these future improvements:
- Add error handling in test utility functions
- Move test data setup to fixtures
- Add table-driven tests for better test coverage
zetaclient/chains/ton/signer/signer.go (2)
Line range hint
177-196: Add documentation for signature format and error casesThe SignMessage method would benefit from additional documentation:
- Document the expected format of the 65-byte signature (R, S, V components)
- Document potential error cases from TSS signing
Add the following documentation:
// SignMessage signs TON external message using TSS +// The resulting signature is a 65-byte array containing: +// - R: first 32 bytes +// - S: next 32 bytes +// - V: recovery ID in the last byte +// Returns error if: +// - message hashing fails +// - TSS signing fails func (s *Signer) SignMessage(ctx context.Context, msg Signable, zetaHeight, nonce uint64) error {
Line range hint
208-219: Enhance signature caching implementationConsider the following improvements to the signature caching mechanism:
- Make the cache size configurable
- Add metrics for cache hits/misses
- Add debug logging for cache operations
Example implementation:
// because signed msg might fail due to high nonce, // we need to make sure that signature is cached to avoid redundant TSS calls func (s *Signer) getSignature(hash [32]byte) ([65]byte, bool) { sig, ok := s.signaturesCache.Get(hash) + if s.Logger().IsDebug() { + s.Logger().Debug(). + Str("operation", "cache_get"). + Bool("hit", ok). + Bytes("hash", hash[:]). + Msg("Signature cache lookup") + } if !ok { return [65]byte{}, false }zetaclient/chains/evm/signer/signer_admin_test.go (1)
40-40: Consider extracting the EVM address retrieval into a helper functionWhile the change from
tss.EVMAddress()totss.PubKey().AddressEVM()is good and more explicit, the pattern is repeated across multiple test cases. Consider extracting this into a test helper function to reduce duplication.+// getEvmAddress returns the EVM address from the TSS public key +func getEvmAddress(tss *mocks.TSS) common.Address { + return tss.PubKey().AddressEVM() +} func verifyTxSender(t *testing.T, tx *types.Transaction, expectedSender common.Address, signer types.Signer) { - verifyTxSender(t, tx, tss.PubKey().AddressEVM(), evmSigner.EvmSigner()) + verifyTxSender(t, tx, getEvmAddress(tss), evmSigner.EvmSigner()) }Also applies to: 60-60, 75-75, 89-89, 121-121, 177-177, 236-236, 252-252, 309-309
zetaclient/chains/bitcoin/observer/observer_test.go (1)
Line range hint
126-156: Consider adding TSS-specific test casesGiven the TSS package revamp, consider adding test cases to verify:
- TSS initialization with different configurations
- Error handling when TSS operations fail
- Edge cases specific to the new TSS implementation
Example test case structure:
{ name: "should handle TSS initialization failure", chain: chain, btcClient: btcClient, chainParams: params, coreClient: nil, tss: mocks.NewTSSWithError(t, "initialization error"), errorMessage: "TSS initialization failed", },contrib/localnet/docker-compose.yml (2)
115-116: Ensure consistent profiling configuration across zetaclientsThe profiling configuration is only applied to
zetaclient0. For consistency in development and testing, consider applying similar configuration to other zetaclients (1,2,3) with different ports.Example configuration for other clients:
zetaclient1: environment: + - PPROF_ADDR=0.0.0.0:6062 ports: + - "6062:6062" # pprof zetaclient2: environment: + - PPROF_ADDR=0.0.0.0:6063 ports: + - "6063:6063" # pprof zetaclient3: environment: + - PPROF_ADDR=0.0.0.0:6064 ports: + - "6064:6064" # pprof
Line range hint
1-7: Enhance documentation for profiling capabilitiesThe file header comments should be updated to include information about the newly added profiling capabilities.
# This docker-compose file configures the localnet environment # it contains the following services: # - ZetaChain with 2 nodes (zetacore0, zetacore1). When profile set to stress, 4 nodes will be created. -# - A observer set with 2 clients (zetaclient0, zetaclient1). When profile set to stress, 4 clients will be created. +# - A observer set with 2 clients (zetaclient0 [pprof:6061], zetaclient1). When profile set to stress, 4 clients will be created. # - An Ethereum node (eth) # - A secondary optional Ethereum node (eth2) enabled when profile is set to eth2 # - A Bitcoin node (bitcoin)zetaclient/chains/bitcoin/signer/signer_test.go (2)
47-48: Consider using test fixtures for private keysWhile the current implementation works, using hardcoded private keys in tests can make maintenance difficult. Consider moving this to a test fixtures file or using a deterministic key generation function for tests.
- tss := mocks.NewTSSFromPrivateKey(c, privateKey) + tss := mocks.NewTSSFromPrivateKey(c, testutils.GetTestPrivateKey())
Line range hint
256-357: Enhance test coverage with edge casesThe test suite is comprehensive but could benefit from additional test cases:
- Maximum transaction value handling
- Dust amount handling (amounts below minimum required)
- Fee estimation edge cases
Consider adding these test cases:
{ name: "should fail on dust amount", tx: wire.NewMsgTx(wire.TxVersion), to: to, total: 0.00000100, // 100 satoshis amount: 0.00000090, nonce: 10000, fees: big.NewInt(2000), fail: true, message: "amount too small", }, { name: "should fail on maximum bitcoin value", tx: wire.NewMsgTx(wire.TxVersion), to: to, total: 21000000.0, // Maximum bitcoin supply amount: 21000001.0, // Exceeds maximum nonce: 10000, fees: big.NewInt(2000), fail: true, message: "amount exceeds maximum", },zetaclient/orchestrator/bootstap_test.go (1)
Line range hint
1-500: Consider enhancing test coverage with additional scenariosWhile the current test coverage is comprehensive, consider adding these scenarios:
- Error cases for chain initialization
- Race conditions in dynamic chain updates
- Edge cases for unsupported chains
Example test case structure:
t.Run("Should handle chain initialization errors", func(t *testing.T) { // ARRANGE invalidChain := chains.Chain{ChainId: -1} // Invalid chain mustUpdateAppContextChainParams(t, app, []chains.Chain{invalidChain}) // ACT _, err := CreateSignerMap(ctx, tss, baseLogger, ts) // ASSERT assert.Error(t, err) assert.Contains(t, err.Error(), "invalid chain") })zetaclient/chains/evm/observer/observer_test.go (2)
120-120: Consider adding error handling for FakePubKey.The change to use
NewTSS(t)withFakePubKeyis good, but consider handling potential errors from theFakePubKeycall to maintain test robustness.- tss = mocks.NewTSS(t).FakePubKey(testutils.TSSPubKeyMainnet) + var err error + tss = mocks.NewTSS(t) + tss, err = tss.FakePubKey(testutils.TSSPubKeyMainnet) + require.NoError(t, err)
Line range hint
184-229: Consider adding TSS-specific test cases.Given that this PR revamps the TSS package, consider adding test cases that specifically verify the observer's interaction with the TSS signer. This could include:
- Testing TSS initialization failures
- Verifying TSS signing behavior
- Testing TSS pubkey validation
Example test case structure:
{ name: "should handle TSS initialization failure", evmCfg: config.EVMConfig{ Endpoint: "http://localhost:8545", }, chainParams: params, evmClient: evmClient, evmJSONRPC: mocks.NewMockJSONRPCClient(), tss: func() interfaces.TSSSigner { mockTSS := mocks.NewTSS(t) mockTSS.On("Initialize").Return(fmt.Errorf("initialization error")) return mockTSS }(), fail: true, message: "initialization error", },zetaclient/chains/bitcoin/signer/signer.go (1)
Line range hint
341-517: Consider refactoring large functions for better maintainabilityThe
TryProcessOutboundfunction is complex and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Input validation and preparation
- Transaction building
- Broadcasting and tracking
- Error handling
This would improve:
- Code maintainability
- Testing capabilities
- Error handling clarity
Example structure:
func (s *Signer) TryProcessOutbound(...) { + if err := s.validateOutboundInput(ctx, cctx, params); err != nil { + logger.Error().Err(err).Msg("invalid input") + return + } + + tx, err := s.buildOutboundTransaction(ctx, params, btcObserver, height) + if err != nil { + logger.Error().Err(err).Msg("failed to build transaction") + return + } + + if err := s.broadcastAndTrack(ctx, tx, params, zetacoreClient); err != nil { + logger.Error().Err(err).Msg("failed to broadcast") + return + } }zetaclient/chains/bitcoin/observer/observer.go (2)
Line range hint
341-394: Consider refactoring FetchUTXOs for better maintainability.The
FetchUTXOsmethod handles multiple responsibilities:
- TSS address retrieval
- UTXO fetching and filtering
- State management
- Telemetry updates
Consider breaking this down into smaller, focused functions for better maintainability and testability.
func (ob *Observer) FetchUTXOs(ctx context.Context) error { defer ob.handlePanic() if err := ob.refreshPendingNonce(ctx); err != nil { return fmt.Errorf("failed to refresh pending nonce: %w", err) } - bh, err := ob.btcClient.GetBlockCount() - if err != nil { - return fmt.Errorf("btc: error getting block height : %v", err) - } - maxConfirmations := int(bh) + maxConfirmations, err := ob.getMaxConfirmations() + if err != nil { + return err + } - tssAddr, err := ob.TSS().PubKey().AddressBTC(ob.Chain().ChainId) - if err != nil { - return fmt.Errorf("error getting bitcoin tss address") - } + tssAddr, err := ob.getTSSAddress() + if err != nil { + return err + } utxos, err := ob.btcClient.ListUnspentMinMaxAddresses(0, maxConfirmations, []btcutil.Address{tssAddr}) if err != nil { return err } - sort.SliceStable(utxos, func(i, j int) bool { - if utxos[i].Amount == utxos[j].Amount { - if utxos[i].TxID == utxos[j].TxID { - return utxos[i].Vout < utxos[j].Vout - } - return utxos[i].TxID < utxos[j].TxID - } - return utxos[i].Amount < utxos[j].Amount - }) + ob.sortUTXOs(utxos) - utxosFiltered := make([]btcjson.ListUnspentResult, 0) - for _, utxo := range utxos { - if utxo.Amount < bitcoin.DefaultDepositorFee { - continue - } - if utxo.Confirmations == 0 { - if !ob.isTssTransaction(utxo.TxID) { - continue - } - } - utxosFiltered = append(utxosFiltered, utxo) - } + utxosFiltered := ob.filterSpendableUTXOs(utxos) ob.updateUTXOState(utxosFiltered) return nil } func (ob *Observer) handlePanic() { if err := recover(); err != nil { ob.logger.UTXOs.Error().Msgf("BTC FetchUTXOs: caught panic error: %v", err) } } func (ob *Observer) getMaxConfirmations() (int, error) { bh, err := ob.btcClient.GetBlockCount() if err != nil { return 0, fmt.Errorf("btc: error getting block height : %v", err) } return int(bh), nil } func (ob *Observer) getTSSAddress() (btcutil.Address, error) { addr, err := ob.TSS().PubKey().AddressBTC(ob.Chain().ChainId) if err != nil { return nil, fmt.Errorf("error getting bitcoin tss address") } return addr, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 356-356: zetaclient/chains/bitcoin/observer/observer.go#L356
Added line #L356 was not covered by tests
Line range hint
341-341: Consider adding context timeout for FetchUTXOs.The
FetchUTXOsmethod accepts a context but doesn't utilize it for timeout or cancellation. Consider adding context checks to ensure the operation can be gracefully cancelled.func (ob *Observer) FetchUTXOs(ctx context.Context) error { + if err := ctx.Err(); err != nil { + return fmt.Errorf("context error: %w", err) + } defer func() { if err := recover(); err != nil { ob.logger.UTXOs.Error().Msgf("BTC FetchUTXOs: caught panic error: %v", err) } }()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 356-356: zetaclient/chains/bitcoin/observer/observer.go#L356
Added line #L356 was not covered by testszetaclient/chains/solana/observer/outbound_test.go (1)
88-88: Maintain consistency in TSS mock initializationLine 88 uses
FakePubKey(tssPubKeyTest)while line 162 doesn't. Consider using consistent mock initialization patterns across test cases.Apply this change:
- tssOther := mocks.NewTSS(t) + tssOther := mocks.NewTSS(t).FakePubKey("0x0different_pub_key_for_test")Also applies to: 162-162
zetaclient/chains/bitcoin/observer/outbound_test.go (3)
38-40: Consider using a test logger with configurable output levelThe current logger implementation uses default settings. For better test debugging capabilities, consider configuring the log level explicitly.
-logger := zerolog.New(zerolog.NewTestWriter(t)) +logger := zerolog.New(zerolog.NewTestWriter(t)).Level(zerolog.DebugLevel).With().Timestamp().Logger()
Line range hint
252-400: Enhance test coverage for edge cases in UTXO selectionThe UTXO selection tests cover basic scenarios but could benefit from additional edge cases:
- Maximum number of inputs (considering Bitcoin's input limit)
- Dust amount handling
- Fee calculation edge cases
Would you like me to generate additional test cases for these scenarios?
Line range hint
402-518: Consider parameterizing UTXO consolidation testsThe consolidation tests use multiple similar test cases with different parameters. Consider using table-driven tests for better maintainability and coverage.
func TestUTXOConsolidation(t *testing.T) { testCases := []struct { name string utxoCap int amount float64 nonce uint64 rank uint16 expectedUtxos int expectedAmount float64 }{ { name: "should not consolidate", utxoCap: 10, amount: 0.01, nonce: 1, rank: 10, expectedUtxos: 2, expectedAmount: 0.01002, }, // Add more test cases... } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Test implementation }) } }zetaclient/chains/evm/signer/signer.go (3)
Line range hint
391-486: Consider refactoring complex conditional logicThe
SignOutboundFromCCTXmethod contains complex nested if-else conditions that handle different coin types and protocol versions. This complexity makes the code harder to maintain and test.Consider refactoring using a strategy pattern:
+type SigningStrategy interface { + Sign(ctx context.Context, cctx *crosschaintypes.CrossChainTx, outboundData *OutboundData) (*ethtypes.Transaction, error) +} + +type ComplianceStrategy struct{} +type AdminCommandStrategy struct{} +type V2ProtocolStrategy struct{} +// ... other strategies + +func getSigningStrategy(cctx *crosschaintypes.CrossChainTx) SigningStrategy { + if compliance.IsCctxRestricted(cctx) { + return &ComplianceStrategy{} + } + if cctx.InboundParams.CoinType == coin.CoinType_Cmd { + return &AdminCommandStrategy{} + } + // ... other strategy selections +}This refactoring would:
- Improve testability by isolating each signing strategy
- Make the code more maintainable by removing nested conditions
- Make it easier to add new signing strategies in the future
Line range hint
487-553: Enhance error handling in BroadcastOutboundWhile the exponential backoff implementation is good, the error handling could be improved:
Consider these improvements:
- Extract backoff logic to a reusable utility:
func withExponentialBackoff(ctx context.Context, maxRetries int, initialBackoff time.Duration, op func() error) error { backoff := initialBackoff for i := 0; i < maxRetries; i++ { if err := op(); err == nil { return nil } time.Sleep(backoff) backoff *= 2 } return fmt.Errorf("operation failed after %d retries", maxRetries) }
- Add context timeout for the entire broadcast operation:
ctx, cancel := context.WithTimeout(ctx, 32*time.Second) // Sum of all backoff periods defer cancel()
Line range hint
1-24: Enhance package documentationThe package documentation could be more comprehensive to help developers understand the complex signing flows.
Consider adding detailed package documentation:
-// Package signer implements the ChainSigner interface for EVM chains +// Package signer implements the ChainSigner interface for EVM chains. +// +// The package provides functionality for: +// - Transaction signing using TSS (Threshold Signature Scheme) +// - Outbound transaction processing and broadcasting +// - Support for multiple coin types (Gas, ERC20, Zeta) +// - Protocol version compatibility (V1, V2) +// +// Key types: +// - Signer: Main implementation of ChainSigner for EVM chains +// - OutboundData: Contains transaction data for outbound processingzetaclient/chains/base/observer_test.go (1)
315-324: Consider extracting address prefixes as constantsThe switch statement improves readability, but the address prefixes ("bc" for Bitcoin and "0x" for Ethereum) could be extracted as constants to improve maintainability and reusability.
+const ( + btcAddressPrefix = "bc" + ethAddressPrefix = "0x" +) switch tt.addrExpected { case "": require.Equal(t, "", addr) case "btc": - require.True(t, strings.HasPrefix(addr, "bc")) + require.True(t, strings.HasPrefix(addr, btcAddressPrefix)) case "eth": - require.True(t, strings.HasPrefix(addr, "0x")) + require.True(t, strings.HasPrefix(addr, ethAddressPrefix)) default: t.Fail() }zetaclient/chains/bitcoin/observer/outbound.go (1)
Line range hint
563-571: Enhance input validation in checkTSSVin.The TSS input verification is critical for security. Consider adding the following validations:
- Check if the witness script format is correct (P2WPKH)
- Validate signature length in witness[0]
pubKeyTss := hex.EncodeToString(ob.TSS().PubKey().Bytes(true)) for i, vin := range vins { // The length of the Witness should be always 2 for SegWit inputs. if len(vin.Witness) != 2 { return fmt.Errorf("checkTSSVin: expected 2 witness items, got %d", len(vin.Witness)) } + // Validate signature length + if len(vin.Witness[0]) < 70 || len(vin.Witness[0]) > 74 { + return fmt.Errorf("checkTSSVin: invalid signature length %d", len(vin.Witness[0])) + } if vin.Witness[1] != pubKeyTss { return fmt.Errorf("checkTSSVin: witness pubkey %s not match TSS pubkey %s", vin.Witness[1], pubKeyTss) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 563-563: zetaclient/chains/bitcoin/observer/outbound.go#L563
Added line #L563 was not covered by testszetaclient/testutils/mocks/zetacore_client.go (1)
866-892: Consider adding method documentation.While this is a generated file, it would be helpful to add documentation for the
PostVoteTSSmethod describing its purpose and parameters, especially since it's part of the TSS (Threshold Signature Scheme) functionality which is being revamped.Add documentation above the method:
+// PostVoteTSS mocks the method for voting on TSS-related operations. +// Parameters: +// - ctx: The context for the operation +// - tssPubKey: The TSS public key +// - keyGenZetaHeight: The block height at which key generation occurred +// - status: The status of the TSS operation +// Returns a transaction hash and any error encountered. func (_m *ZetacoreClient) PostVoteTSS(ctx context.Context, tssPubKey string, keyGenZetaHeight int64, status chains.ReceiveStatus) (string, error) {zetaclient/chains/evm/observer/outbound_test.go (2)
445-446: Consider extracting TSS mock initialization to a helper function.The TSS mock initialization with a fake public key is a common setup pattern that could be reused across test cases.
+func newTSSMockWithKey(t *testing.T, pubKey string) *mocks.TSS { + return mocks.NewTSS(t).FakePubKey(pubKey) +} + // In test function -tss := mocks.NewTSS(t).FakePubKey(testutils.TSSPubKeyMainnet) +tss := newTSSMockWithKey(t, testutils.TSSPubKeyMainnet)
479-479: Maintain consistency in TSS mock initialization.For better test clarity, consider explicitly indicating that this is a TSS mock without a public key, making the test's intention more apparent.
-tss := mocks.NewTSS(t) +// Using the helper function suggested above +tss := newTSSMockWithKey(t, "") // explicitly indicate no pubkey for negative testzetaclient/chains/evm/observer/inbound.go (2)
617-618: Enhance error message and optimize address comparisonThe error message could be more descriptive, and the address comparison could be optimized by storing the TSS address in a variable to avoid multiple conversions.
+ tssAddr := ob.TSS().PubKey().AddressEVM() - if ethcommon.HexToAddress(tx.To) != ob.TSS().PubKey().AddressEVM() { - return "", fmt.Errorf("tx.To %s is not TSS address", tx.To) + if ethcommon.HexToAddress(tx.To) != tssAddr { + return "", fmt.Errorf("invalid recipient address: expected TSS address %s, got %s", tssAddr.Hex(), tx.To)
Line range hint
456-458: Address TODO for inbound observation controlThe current method of disabling inbound observation through code comments is not ideal. A more robust configuration-based approach should be implemented.
Would you like me to help implement a configuration-based solution for controlling inbound observation? This could include:
- Adding a configuration flag in the chain parameters
- Implementing a dynamic control mechanism through the admin API
- Adding proper logging and metrics for the disabled state
changelog.md (1)
27-27: Enhance changelog entry for TSS package revamp.The changelog entry should provide more context about the TSS package changes and maintain consistency with other entries by ending with a period.
Apply this diff to improve the entry:
-* [3170](https://github.com/zeta-chain/node/pull/3170) - revamp TSS package in zetaclient +* [3170](https://github.com/zeta-chain/node/pull/3170) - revamp TSS package in zetaclient to improve maintainability and simplify logic.zetaclient/tss/healthcheck.go (1)
63-70: Enhance panic handling with detailed contextWhile the deferred function recovers from panics, adding more context to the error logging can aid in debugging.
Consider including the stack trace or additional identifiers:
defer func() { if r := recover(); r != nil { logger.Error(). Str("peer_id", peerID.String()). + Bytes("stack", debug.Stack()). Interface("panic", r). Msg("panic during ping") } }()This provides more information about the panic, facilitating easier troubleshooting.
zetaclient/tss/crypto.go (6)
61-68: Pass Pointers for Efficiency and ConsistencyIn the
NewPubKeyFromECDSAfunction, theecdsa.PublicKeyis passed by value and then its address is taken within the function. Passing large structs by value can be inefficient and may lead to unintended copies.Modify the function to accept a pointer to
ecdsa.PublicKey:-func NewPubKeyFromECDSA(pk ecdsa.PublicKey) (PubKey, error) { +func NewPubKeyFromECDSA(pk *ecdsa.PublicKey) (PubKey, error) { compressed := elliptic.MarshalCompressed(pk.Curve, pk.X, pk.Y) return PubKey{ cosmosPubKey: &secp256k1.PubKey{Key: compressed}, - ecdsaPubKey: &pk, + ecdsaPubKey: pk, }, nil }This change avoids unnecessary copying and aligns with Go conventions for handling mutable data structures.
86-86: Handle Errors Explicitly When Calling FunctionsThe
NewPubKeyFromECDSAHexStringfunction callsNewPubKeyFromECDSA(*pk)but does not handle any potential errors that might arise ifNewPubKeyFromECDSAchanged to return an error in the future.To future-proof the code, consider capturing the returned error:
- return NewPubKeyFromECDSA(*pk) + pubKey, err := NewPubKeyFromECDSA(pk) + if err != nil { + return PubKey{}, errors.Wrap(err, "unable to create PubKey from ECDSA") + } + return pubKey, nilThis ensures that any errors are properly handled and propagated.
Would you like assistance in updating the related code to handle errors appropriately?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-86: zetaclient/tss/crypto.go#L86
Added line #L86 was not covered by tests
31-35: Avoid Aliasing Standard Library FunctionsAssigning standard library functions to local variables like
base64Decodeandbase64DecodeStringcan reduce code clarity and make the codebase harder to maintain.Consider using the standard library functions directly for improved readability:
-var ( - base64Decode = base64.StdEncoding.Decode - base64DecodeString = base64.StdEncoding.DecodeString - base64EncodeString = base64.StdEncoding.EncodeToString -)This makes the code more explicit and easier for other developers to understand without needing to trace variable assignments.
215-220: Optimize Digest Combination for Better PerformanceThe
combineDigestsfunction concatenates all digest strings into a single string before hashing. This approach can be inefficient, especially with large datasets.Consider using a hash function that can process each digest incrementally:
func combineDigests(digestList []string) []byte { h := chainhash.DoubleHashB(nil) for _, digest := range digestList { h = chainhash.DoubleHashB(append(h, []byte(digest)...)) } return h }This method avoids creating large intermediate strings and can improve performance.
I can help refactor this function if you would like to implement this optimization.
51-52: Increase Test Coverage for Error ScenariosLines 51-52 involve error handling when decompressing a public key. Currently, these lines are not covered by tests.
Include unit tests that provide invalid or malformed public keys to trigger this error path. This will ensure that the function behaves correctly under failure conditions.
Would you like assistance in creating unit tests to cover these cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-52: zetaclient/tss/crypto.go#L51-L52
Added lines #L51 - L52 were not covered by tests
76-84: Add Unit Tests for Hex String Parsing ErrorsThe error handling in
NewPubKeyFromECDSAHexString(lines 76-79 and 81-84) is not covered by tests. Testing these paths will enhance code reliability.Develop unit tests that pass invalid hex strings or corrupted public keys to verify that the function handles errors gracefully.
I'm available to help write these tests if needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-79: zetaclient/tss/crypto.go#L76-L79
Added lines #L76 - L79 were not covered by tests
[warning] 81-84: zetaclient/tss/crypto.go#L81-L84
Added lines #L81 - L84 were not covered by testszetaclient/tss/service.go (2)
91-110: Avoid failing service initialization due to metrics setupIn the
WithMetricsfunction, an error returned fromzetacore.GetKeyGen(ctx)causes the entire service initialization to fail. This means that if there's an issue retrieving the keygen information, the TSS service won't start, which may not be desirable. Consider handling this error more gracefully, such as by logging a warning and proceeding without metrics, to ensure the service remains operational even if metrics setup encounters issues.
291-295: Ensure digests slice is not empty before accessingWhen selecting a digest for indexing blame data, the code accesses
digests[0]without checking its length in the else branch. Although the length ofdigestsis validated earlier, adding an explicit check here improves code safety and readability.Apply this diff to add an explicit check:
} else { - digest = digests[0] + if len(digests) > 0 { + digest = digests[0] + } else { + return errors.New("no digests available for blame indexing") + } }cmd/zetaclientd/start.go (1)
33-37: Document the newly introduced environment variablesThe addition of
envFlagPostBlameandenvPprofAddrintroduces important configuration options. Ensure these environment variables are thoroughly documented in the application's configuration guide to assist users in proper setup and utilization.zetaclient/tss/setup.go (1)
275-275: Simplify theisEmptyPeerIDfunction for clarityThe function
isEmptyPeerIDchecks for multiple representations of an empty Peer ID. Streamlining this check enhances readability and reduces potential errors.Apply this diff to simplify the function:
- func isEmptyPeerID(id string) bool { - return id == "" || id == "0" || id == "000000000000000000000000000000" || id == peer.ID("").String() - } + func isEmptyPeerID(id string) bool { + return id == "" || id == peer.ID("").String() + }zetaclient/tss/keygen.go (5)
89-89: Ensure consistent access to thezetacoreinstanceIn the
KeygenCeremonyfunction, the code uses the variablezcdirectly to callGetTSS(ctx). For consistency and clarity, consider using thezetacoreinstance from thekeygenCeremonystruct throughout the method.Proposed change:
-return zc.GetTSS(ctx) +return ceremony.zetacore.GetTSS(ctx)
129-129: UseMsginstead ofMsgfwhen no formatting is requiredThe logging statements use
Msgfwithout any formatting directives. ReplaceMsgfwithMsgto improve clarity and reduce unnecessary overhead.Proposed fixes:
At line 129:
- Msgf("Waiting for keygen block to arrive or new keygen block to be set") + Msg("Waiting for keygen block to arrive or new keygen block to be set")At line 134:
- Msgf("Waiting for keygen finalization") + Msg("Waiting for keygen finalization")Also applies to: 134-134
259-261: Handle context cancellation in loops to enhance responsivenessIn the
waitForBlockfunction, the loop includes atime.Sleepwithout checking for context cancellation. To allow for graceful shutdowns and to avoid unnecessary delays when the context is canceled, consider monitoring the context within the loop.Suggested change:
Replace the
time.Sleepwith aselectstatement to handle context cancellation:-for { - time.Sleep(time.Second) +for { + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(time.Second): + // continue the loop + }
280-280: Unexported functionTestKeySignmay not need to be exportedThe function
TestKeySignis exported but may not need to be accessed outside the package. If it is intended for internal use, consider making it unexported to reduce the package's API surface and potential misuse.Suggested change:
-func TestKeySign(keySigner KeySigner, tssPubKeyBec32 string, logger zerolog.Logger) error { +func testKeySign(keySigner KeySigner, tssPubKeyBec32 string, logger zerolog.Logger) error {
271-271: Check for potential errors when writing to hash functionsWhile the
hasher.Writemethod currently does not return an error, future changes to the standard library may introduce error returns. To make the code resilient to such changes, consider checking the return value.Suggested change:
-hasher.Write(bytes) +if _, err := hasher.Write(bytes); err != nil { + return "", errors.Wrap(err, "failed to write bytes to hasher") +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (58)
changelog.md(1 hunks)cmd/zetaclientd/initconfig.go(2 hunks)cmd/zetaclientd/start.go(4 hunks)cmd/zetaclientd/utils.go(3 hunks)contrib/localnet/docker-compose.yml(1 hunks)zetaclient/chains/base/observer.go(1 hunks)zetaclient/chains/base/observer_test.go(7 hunks)zetaclient/chains/base/signer_test.go(2 hunks)zetaclient/chains/bitcoin/observer/observer.go(1 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(2 hunks)zetaclient/chains/bitcoin/observer/outbound.go(1 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(6 hunks)zetaclient/chains/bitcoin/observer/rpc_status.go(1 hunks)zetaclient/chains/bitcoin/signer/signer.go(2 hunks)zetaclient/chains/bitcoin/signer/signer_keysign_test.go(3 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(4 hunks)zetaclient/chains/evm/observer/inbound.go(2 hunks)zetaclient/chains/evm/observer/inbound_test.go(1 hunks)zetaclient/chains/evm/observer/observer_test.go(4 hunks)zetaclient/chains/evm/observer/outbound.go(2 hunks)zetaclient/chains/evm/observer/outbound_test.go(2 hunks)zetaclient/chains/evm/signer/sign.go(1 hunks)zetaclient/chains/evm/signer/sign_test.go(12 hunks)zetaclient/chains/evm/signer/signer.go(2 hunks)zetaclient/chains/evm/signer/signer_admin_test.go(14 hunks)zetaclient/chains/evm/signer/signer_test.go(6 hunks)zetaclient/chains/interfaces/interfaces.go(3 hunks)zetaclient/chains/solana/observer/observer_test.go(1 hunks)zetaclient/chains/solana/observer/outbound.go(1 hunks)zetaclient/chains/solana/observer/outbound_test.go(5 hunks)zetaclient/chains/solana/signer/whitelist.go(1 hunks)zetaclient/chains/solana/signer/withdraw.go(1 hunks)zetaclient/chains/solana/signer/withdraw_spl.go(1 hunks)zetaclient/chains/ton/observer/inbound_test.go(1 hunks)zetaclient/chains/ton/observer/observer_test.go(2 hunks)zetaclient/chains/ton/observer/outbound.go(2 hunks)zetaclient/chains/ton/signer/signer.go(1 hunks)zetaclient/chains/ton/signer/signer_test.go(2 hunks)zetaclient/orchestrator/bootstap_test.go(2 hunks)zetaclient/testutils/constant.go(1 hunks)zetaclient/testutils/mocks/tss.go(1 hunks)zetaclient/testutils/mocks/tss_signer.go(0 hunks)zetaclient/testutils/mocks/zetacore_client.go(1 hunks)zetaclient/tss/concurrent_keysigns_tracker.go(0 hunks)zetaclient/tss/concurrent_keysigns_tracker_test.go(0 hunks)zetaclient/tss/config.go(1 hunks)zetaclient/tss/config_test.go(1 hunks)zetaclient/tss/crypto.go(1 hunks)zetaclient/tss/crypto_test.go(1 hunks)zetaclient/tss/generate.go(0 hunks)zetaclient/tss/healthcheck.go(1 hunks)zetaclient/tss/keygen.go(1 hunks)zetaclient/tss/readme.md(1 hunks)zetaclient/tss/service.go(1 hunks)zetaclient/tss/service_test.go(1 hunks)zetaclient/tss/setup.go(1 hunks)zetaclient/tss/tss_signer.go(0 hunks)zetaclient/tss/tss_signer_test.go(0 hunks)
💤 Files with no reviewable changes (6)
- zetaclient/testutils/mocks/tss_signer.go
- zetaclient/tss/concurrent_keysigns_tracker.go
- zetaclient/tss/concurrent_keysigns_tracker_test.go
- zetaclient/tss/generate.go
- zetaclient/tss/tss_signer.go
- zetaclient/tss/tss_signer_test.go
🧰 Additional context used
📓 Path-based instructions (49)
cmd/zetaclientd/initconfig.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/start.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/utils.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/rpc_status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer_keysign_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/sign.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/sign_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer_admin_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/interfaces/interfaces.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/whitelist.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/withdraw_spl.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/bootstap_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/constant.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/tss.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/zetacore_client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/config.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/config_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/crypto.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/crypto_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/healthcheck.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/keygen.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/service.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/service_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/setup.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/observer.go
[warning] 356-356: zetaclient/chains/bitcoin/observer/observer.go#L356
Added line #L356 was not covered by tests
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 563-563: zetaclient/chains/bitcoin/observer/outbound.go#L563
Added line #L563 was not covered by tests
zetaclient/chains/bitcoin/observer/rpc_status.go
[warning] 32-32: zetaclient/chains/bitcoin/observer/rpc_status.go#L32
Added line #L32 was not covered by tests
zetaclient/chains/bitcoin/signer/signer.go
[warning] 307-307: zetaclient/chains/bitcoin/signer/signer.go#L307
Added line #L307 was not covered by tests
zetaclient/chains/evm/observer/outbound.go
[warning] 505-505: zetaclient/chains/evm/observer/outbound.go#L505
Added line #L505 was not covered by tests
[warning] 508-517: zetaclient/chains/evm/observer/outbound.go#L508-L517
Added lines #L508 - L517 were not covered by tests
zetaclient/chains/solana/signer/whitelist.go
[warning] 34-34: zetaclient/chains/solana/signer/whitelist.go#L34
Added line #L34 was not covered by tests
zetaclient/chains/solana/signer/withdraw.go
[warning] 46-46: zetaclient/chains/solana/signer/withdraw.go#L46
Added line #L46 was not covered by tests
zetaclient/chains/solana/signer/withdraw_spl.go
[warning] 60-60: zetaclient/chains/solana/signer/withdraw_spl.go#L60
Added line #L60 was not covered by tests
zetaclient/chains/ton/observer/outbound.go
[warning] 178-178: zetaclient/chains/ton/observer/outbound.go#L178
Added line #L178 was not covered by tests
[warning] 195-195: zetaclient/chains/ton/observer/outbound.go#L195
Added line #L195 was not covered by tests
zetaclient/tss/config.go
[warning] 23-26: zetaclient/tss/config.go#L23-L26
Added lines #L23 - L26 were not covered by tests
[warning] 28-31: zetaclient/tss/config.go#L28-L31
Added lines #L28 - L31 were not covered by tests
[warning] 33-33: zetaclient/tss/config.go#L33
Added line #L33 was not covered by tests
[warning] 39-42: zetaclient/tss/config.go#L39-L42
Added lines #L39 - L42 were not covered by tests
[warning] 44-49: zetaclient/tss/config.go#L44-L49
Added lines #L44 - L49 were not covered by tests
[warning] 51-54: zetaclient/tss/config.go#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 56-56: zetaclient/tss/config.go#L56
Added line #L56 was not covered by tests
[warning] 68-69: zetaclient/tss/config.go#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 90-91: zetaclient/tss/config.go#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 95-96: zetaclient/tss/config.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 103-104: zetaclient/tss/config.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 111-113: zetaclient/tss/config.go#L111-L113
Added lines #L111 - L113 were not covered by tests
zetaclient/tss/crypto.go
[warning] 51-52: zetaclient/tss/crypto.go#L51-L52
Added lines #L51 - L52 were not covered by tests
[warning] 71-74: zetaclient/tss/crypto.go#L71-L74
Added lines #L71 - L74 were not covered by tests
[warning] 76-79: zetaclient/tss/crypto.go#L76-L79
Added lines #L76 - L79 were not covered by tests
[warning] 81-84: zetaclient/tss/crypto.go#L81-L84
Added lines #L81 - L84 were not covered by tests
[warning] 86-86: zetaclient/tss/crypto.go#L86
Added line #L86 was not covered by tests
[warning] 100-100: zetaclient/tss/crypto.go#L100
Added line #L100 was not covered by tests
[warning] 110-110: zetaclient/tss/crypto.go#L110
Added line #L110 was not covered by tests
🪛 Gitleaks (8.21.1)
zetaclient/chains/solana/observer/outbound_test.go
39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
zetaclient/testutils/constant.go
16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
zetaclient/tss/readme.md
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... ``` ## Package Structure - setup.go: Initializes the go-tss TSS server and t...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ...* wrapper of this package. - keygen.go: Manages the key generation ceremony, cr...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~29-~29: Loose punctuation mark.
Context: ...reating keys used by TSS. - service.go: Implements the Service struct, offe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...or TSS operations. ## Links - go-tss: https://github.com/zeta-chain/go-tss - ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.35.0)
zetaclient/tss/readme.md
34-34: null
Bare URL used
(MD034, no-bare-urls)
35-35: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (56)
zetaclient/chains/bitcoin/observer/rpc_status.go (1)
32-32: Verify the new TSS interface usage across the codebase.
The change from BTCAddress to PubKey().AddressBTC() aligns with the TSS package revamp. Let's verify this pattern is consistently applied across the codebase.
✅ Verification successful
All TSS interface changes are consistently applied across the codebase
The verification shows:
- No instances of the old
.BTCAddress()pattern were found - The new
.PubKey().AddressBTC()pattern is consistently used across all Bitcoin-related components:- Bitcoin observers (mainnet and testnet)
- Bitcoin signers
- Base observer implementation
- TSS setup code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of the new TSS interface pattern
# across the codebase
# Check for any remaining direct BTCAddress calls
echo "Checking for old BTCAddress pattern usage:"
rg "\.BTCAddress\(" --type go
# Check for consistent usage of new pattern
echo -e "\nVerifying new PubKey().AddressBTC pattern usage:"
rg "\.PubKey\(\)\.AddressBTC\(" --type go
Length of output: 1344
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: zetaclient/chains/bitcoin/observer/rpc_status.go#L32
Added line #L32 was not covered by tests
zetaclient/tss/crypto_test.go (1)
1-11: LGTM: Well-organized imports
The imports are clean, necessary, and properly organized.
zetaclient/chains/solana/observer/observer_test.go (1)
35-35: LGTM! Standardized TSS mock instantiation.
The change from NewTSSMainnet() to NewTSS(t) aligns with the TSS package revamp and standardizes mock creation across tests. The test context (t) parameter enables better error reporting.
Let's verify this standardization across other test files:
✅ Verification successful
TSS mock standardization verified across codebase
The verification confirms that all TSS mock instantiations consistently use the new NewTSS(t) pattern. No instances of the old NewTSSMainnet() were found, indicating complete standardization across all test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of NewTSS(t) across test files
# Expect: All TSS mock instantiations use the new pattern
# Search for old TSS mock instantiation
echo "Checking for old NewTSSMainnet() usage:"
rg "NewTSSMainnet\(\)"
# Search for new TSS mock instantiation
echo "Checking for new NewTSS(t) usage:"
rg "NewTSS\(t\)"
Length of output: 3246
zetaclient/chains/base/signer_test.go (2)
43-45: LGTM! Consistent mock usage.
The TSS mock instantiation is consistent with the updated pattern and maintains clear test assertions.
Line range hint 15-22: LGTM! Improved test context handling.
The updated signature properly utilizes the testing context, enabling better test failure reporting and consistency with Go testing patterns.
Let's verify consistent usage of the new TSS mock pattern across test files:
✅ Verification successful
LGTM! The TSS mock implementation is consistent across the codebase.
The changes to createSigner function are correct and follow best practices:
- The new signature properly includes the testing context parameter
- The TSS mock instantiation uses the new
NewTSS(t)pattern which is consistently used across the codebase - The mock implementation in
zetaclient/testutils/mocks/tss.gois well-structured with proper testing assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old TSS mock patterns
# Expect: No occurrences of old patterns (NewTSSMainnet, NewTSSAthens3)
rg "NewTSSMainnet|NewTSSAthens3" --type go
Length of output: 44
Script:
#!/bin/bash
# Let's verify the TSS mock usage patterns more comprehensively
# 1. Find all test files that might use TSS mocks
# 2. Check the pattern of TSS mock initialization
# First, find all test files
fd "_test\.go$" | while read -r file; do
echo "=== $file ==="
# Look for TSS-related patterns in test files
rg "NewTSS|tss.*mock|mock.*tss" -i "$file" -C 2
done
Length of output: 103056
Script:
#!/bin/bash
# Let's verify the TSS mock pattern in the mocks package
# to ensure the new pattern is properly defined
# First check the mock implementation
echo "=== Checking TSS mock implementation ==="
rg -A 5 "type TSS struct" zetaclient/mocks/
# Then check the constructor patterns
echo "=== Checking TSS mock constructors ==="
rg "func New(TSS|Mock)" zetaclient/mocks/
Length of output: 383
Script:
#!/bin/bash
# Let's check the entire codebase for the mock pattern
# First, find the mock package location
fd -t d "mock"
# Then check for TSS-related files in those directories
echo "=== Checking TSS mock files ==="
fd -t f "tss.*\.go"
# Finally, look for NewTSS constructor pattern in go files
echo "=== Checking NewTSS implementations ==="
rg "func.*NewTSS.*testing\.T" --type go
Length of output: 1781
Script:
#!/bin/bash
# Let's examine the TSS mock implementation and its usage pattern
echo "=== Checking TSS mock implementation ==="
cat zetaclient/testutils/mocks/tss.go
# Also check the test file where the mock is being updated
echo "=== Checking test implementation ==="
cat zetaclient/chains/base/signer_test.go
Length of output: 4903
zetaclient/testutils/mocks/tss.go (2)
19-35: LGTM: Well-structured TSS mock implementation.
The TSS struct and its constructors are well-designed with appropriate error handling and initialization.
125-127: Consider documenting potential side effects of init function.
The init function sets up Cosmos config which might have side effects on other tests. Consider documenting these effects and whether this setup is necessary for all test scenarios.
zetaclient/chains/solana/signer/whitelist.go (2)
Line range hint 18-23: LGTM: Well-structured method signature with clear documentation.
The method signature and documentation effectively communicate the purpose and parameters of the whitelist message creation and signing process.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: zetaclient/chains/solana/signer/whitelist.go#L34
Added line #L34 was not covered by tests
34-34: Verify consistent removal of optionalPubkey parameter.
The removal of the empty string parameter aligns with the PR objectives. Let's verify this change is consistent across all TSS Sign calls.
✅ Verification successful
All TSS().Sign calls consistently use the updated signature format
The verification results show that all TSS().Sign calls across the codebase have been updated to use the new signature format with exactly 5 parameters (ctx, hash, height, nonce, chainId). No instances of the old signature format with the additional optionalPubkey parameter were found.
Locations verified:
- zetaclient/chains/solana/signer/withdraw_spl.go
- zetaclient/chains/solana/signer/whitelist.go
- zetaclient/chains/solana/signer/withdraw.go
- zetaclient/chains/evm/signer/signer.go
- zetaclient/chains/ton/signer/signer.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of TSS().Sign calls with the old signature
# Search for any Sign calls that might still have the extra parameter
rg -U "TSS\(\)\.Sign\([^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*\)"
# Search for all current Sign calls to verify consistent usage
rg -U "TSS\(\)\.Sign\([^)]+\)"
Length of output: 716
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: zetaclient/chains/solana/signer/whitelist.go#L34
Added line #L34 was not covered by tests
zetaclient/testutils/constant.go (1)
16-16: Verify the mainnet public key usage across the codebase
The addition of TSSPubKeyMainnet looks good. Let's verify its usage to ensure consistent integration with the TSS package revamp.
✅ Verification successful
TSSPubKeyMainnet constant is properly integrated and consistently used
The verification shows the constant is used appropriately across test files and mocks, primarily for:
- TSS mock initialization in various chain-specific tests
- Test data fixtures in
testdata/cctx/ - Crypto validation tests
The public key value appears only in test-related files and is properly centralized in the new constant, indicating good refactoring practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of TSSPubKeyMainnet across the codebase
# Search for references to the new constant
rg "TSSPubKeyMainnet" --type go
# Search for the actual public key value to ensure it's not hardcoded elsewhere
rg "zetapub1addwnpepqtadxdyt037h86z60nl98t6zk56mw5zpnm79tsmvspln3hgt5phdc79kvfc" --type go
Length of output: 3167
🧰 Tools
🪛 Gitleaks (8.21.1)
16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
zetaclient/chains/solana/signer/withdraw.go (1)
46-46: LGTM! Verify TSS interface implementation.
The removal of the optional pubkey parameter aligns with the TSS package revamp objectives.
Let's verify the TSS interface implementation across the codebase:
✅ Verification successful
The previous attempt had syntax issues. I've modified the search patterns to:
- Add return type placeholder in ast-grep pattern
- Fix the regex for old signature pattern to properly match function calls with 6 parameters
- Added a broader search for Sign method calls to understand the usage pattern
The results show consistent usage of the new Sign signature across different chains (Bitcoin, TON, EVM, Solana), but we should verify the interface definition and ensure no old implementations remain.
The previous search patterns might have been too strict. Let's try more flexible patterns to locate the TSS interface definition and its implementations, focusing on files that are likely to contain TSS-related code.
TSS interface modification verified successfully
The TSS interface and its implementations across the codebase have been thoroughly verified:
- The interface definition in
zetaclient/chains/interfaces/interfaces.goshows the correct signature:
Sign(ctx context.Context, data []byte, height, nonce uint64, chainID int64) ([65]byte, error)-
All implementations (Bitcoin, TON, EVM, Solana) consistently use the new signature without the optional pubkey parameter.
-
No legacy implementations with the old signature (containing optionalPubkey) were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TSS Sign method implementations match the new signature
# Search for TSS Sign method implementations
ast-grep --pattern 'Sign(ctx context.Context, msg []byte, height uint64, nonce uint64, chainID int64)'
# Search for any remaining calls with the old signature
rg 'Sign.*,\s*"")\s*$' -g '*.go'
Length of output: 220
Script:
#!/bin/bash
# Search for TSS Sign method implementations
ast-grep --pattern 'Sign(ctx context.Context, msg []byte, height uint64, nonce uint64, chainID int64) ('
# Search for any remaining calls with old signature pattern
rg -U 'Sign\([^)]+,[^)]+,[^)]+,[^)]+,[^)]+,[^)]+\)' -g '*.go'
# Find all TSS Sign method calls for context
rg -U 'Sign\(ctx,' -g '*.go' -A 1
Length of output: 9191
Script:
#!/bin/bash
# Let's verify the TSS interface definition
rg -U 'type TSS interface' -g '*.go' -A 10
# And check for any old implementations that might still have the old signature
rg -U 'func.*Sign.*optionalPubkey' -g '*.go'
Length of output: 91
Script:
#!/bin/bash
# Search for TSS interface with more flexible pattern
rg -U 'interface.*TSS' -g '*.go' -A 10
# Search for any file containing TSS type or interface
fd -e go | xargs grep -l "TSS"
# Look for Sign method in potential TSS implementation files
rg -U 'func.*Sign.*\(' -g '*tss*.go'
Length of output: 34783
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: zetaclient/chains/solana/signer/withdraw.go#L46
Added line #L46 was not covered by tests
cmd/zetaclientd/initconfig.go (2)
10-10: LGTM: Clean import addition
The new import aligns with the PR's objective of moving TSS-related functionality to a dedicated package.
77-79: Enhance error message with format requirements
While the validation logic is correct, consider enhancing the error message to include the expected peer address format for better user experience.
if opts.peer != "" {
- if _, err := zetatss.MultiAddressFromString(opts.peer); err != nil {
- return errors.Wrap(err, "invalid peer address")
+ if _, err := zetatss.MultiAddressFromString(opts.peer); err != nil {
+ return errors.Wrap(err, "invalid peer address format. Expected format: /ip4/<ip>/tcp/<port>/p2p/<peer_id>")
}
}Let's verify the validation rules in the new implementation:
✅ Verification successful
Enhance error message with format requirements
The suggestion to improve the error message is valid. The implementation of MultiAddressFromString in zetaclient/tss/config.go shows that it properly validates the peer address format using the multiaddr package, with an example format shown in the code comment: /ip4/172.0.2.1/tcp/6668/p2p/16Uiu2HAmACG5DtqmQsHtXg4G2sLS65ttv84e7MrL4kapkjfmhxAp.
- Update the error message in
cmd/zetaclientd/initconfig.goto include the expected format:
return errors.Wrap(err, "invalid peer address. Expected format: /ip4/<ip>/tcp/<port>/p2p/<peer_id>")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if MultiAddressFromString maintains the same validation rules as the old validatePeer function
# Search for MultiAddressFromString implementation and its tests
ast-grep --pattern 'func MultiAddressFromString($$$) {
$$$
}'
# Look for any test cases
rg -A 5 'TestMultiAddressFromString'
Length of output: 102
Script:
#!/bin/bash
# Let's try a broader search for the implementation and usage of MultiAddressFromString
# Search for MultiAddressFromString in the codebase
rg -B 2 -A 5 "MultiAddressFromString"
# Search for any related test files
fd "test.*\.go" --exec grep -l "MultiAddressFromString" {}
# Look for the old validatePeer function to compare validation rules
rg -B 2 -A 5 "validatePeer"
Length of output: 1597
zetaclient/chains/solana/signer/withdraw_spl.go (1)
60-60: LGTM: TSS signature update aligns with PR objectives.
The removal of the empty string argument from signer.TSS().Sign() aligns with the PR's goal of removing the unused optionalPubkey parameter.
Let's verify this change is consistent across other TSS sign calls:
✅ Verification successful
TSS signature update is consistent across the codebase
All TSS sign calls across different chain implementations (TON, EVM, and Solana) follow the same updated signature pattern with five parameters: context, hash bytes, height, nonce, and chain ID. No instances of the old signature format were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining TSS().Sign calls that might still use the old signature
rg -U "TSS\(\)\.Sign\([^)]*,[^)]*\)" --type go
Length of output: 669
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: zetaclient/chains/solana/signer/withdraw_spl.go#L60
Added line #L60 was not covered by tests
zetaclient/chains/evm/signer/sign.go (1)
114-114: Verify TSS interface changes across the codebase.
The change from EVMAddress() to PubKey().AddressEVM() reflects the new TSS interface design. Let's ensure this change is consistently applied across the codebase.
✅ Verification successful
Interface change is consistently applied across the codebase
The change from EVMAddress() to PubKey().AddressEVM() is correctly implemented in the TSS interface. The search results confirm:
- All TSS-related EVM address derivations now use
PubKey().AddressEVM() - Old
EVMAddress()occurrences are only present in test runners, config, and e2e test files where they represent user/account addresses, not TSS addresses - The change is consistently applied across all chain observers and signers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old EVMAddress() method
rg "EVMAddress\(\)" --type go
# Search for the new pattern to verify consistent usage
rg "PubKey\(\)\.AddressEVM\(\)" --type go
Length of output: 20335
zetaclient/chains/ton/signer/signer_test.go (2)
146-146: LGTM: Mock instantiation aligned with new TSS interface
The change from NewTSSAthens3 to NewTSS reflects the TSS package revamp, providing a more generic and maintainable mock implementation.
204-206: Verify TSS signature parameters and enhance test coverage
While the removal of optionalPubkey aligns with Issue #2085, please verify:
- The purpose of the three zero parameters in
ts.tss.Sign(ts.ctx, hash[:], 0, 0, 0) - Whether these parameters should be meaningful test values instead of zeros
Consider adding assertions to verify the signature format and structure.
Let's check the TSS interface implementation:
✅ Verification successful
Let me gather more information about the TSS interface and its implementation to understand the parameters better.
Let me try a different pattern to find the TSS interface and implementation.
Parameters in TSS Sign method are correctly used
Based on the implementation found in zetaclient/tss/service.go and its mock in zetaclient/testutils/mocks/tss.go, the Sign method parameters are:
height: block height of the transactionnonce: transaction nonce for uniquenesschainID: target chain identifier
The test's use of zeros (0, 0, 0) is acceptable in this context as it's a unit test focusing on the signature generation rather than chain-specific validation. The signature format is properly handled, returning a 65-byte array containing R (32B), S (32B), and V (1B).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find TSS interface and Sign method implementations
ast-grep --pattern 'interface TSS {
$$$
Sign($$$) {
$$$
}
$$$
}'
# Find all Sign method calls to verify parameter usage
rg -A 2 '\.Sign\('
Length of output: 13303
Script:
#!/bin/bash
# Find TSS interface definition
rg -A 10 'interface TSS'
# Find actual TSS implementation to understand the parameters
ast-grep --pattern 'func ($VAR $_) Sign(ctx context.Context, msg []byte, height, nonce, chainID int64) ([]byte, error)'
Length of output: 148
Script:
#!/bin/bash
# Find TSS interface with a simpler pattern
rg -B 2 -A 5 'type TSS interface'
# Find Sign method implementations in tss package
rg -B 2 -A 5 'func.*Sign.*context\.Context' zetaclient/tss/
# Look for mock implementations for parameter usage
rg -B 2 -A 5 'func.*Sign.*context\.Context' zetaclient/testutils/
Length of output: 4645
zetaclient/tss/service_test.go (1)
1-30: LGTM! Well-structured package and imports.
The package structure follows Go conventions, and the imports are well-organized.
zetaclient/chains/evm/signer/sign_test.go (1)
36-36: LGTM! Improved address derivation approach.
The change from EVMAddress() to PubKey().AddressEVM() makes the address derivation path more explicit and aligns with best practices for deriving addresses from public keys. The pattern is consistently applied across all verification calls.
Also applies to: 55-55, 87-87, 119-119, 157-157, 161-161, 195-195, 231-231
zetaclient/chains/evm/signer/signer_test.go (1)
139-139: Test modifications look good
The updates to pass the testing context to helper functions are consistent and maintain the original test logic.
Also applies to: 151-151, 166-166, 195-195
zetaclient/chains/ton/signer/signer.go (1)
190-190: LGTM! Verify consistency across TSS implementations
The removal of the empty string parameter (previously optionalPubkey) aligns with the PR objectives. The change simplifies the interface while maintaining functionality.
Let's verify this change is consistent across all TSS implementations:
✅ Verification successful
TSS Sign interface change is consistent across the codebase
The verification confirms that the TSS Sign method signature has been consistently updated across the codebase:
- The TSS interface in
zetaclient/chains/interfaces/interfaces.godefines the new signature:
Sign(ctx context.Context, data []byte, height, nonce uint64, chainID int64) ([65]byte, error)- All implementations and usages follow this pattern, including:
- TON signer:
zetaclient/chains/ton/signer/signer.go - Solana signer:
zetaclient/chains/solana/signer/withdraw_spl.go - EVM signer:
zetaclient/chains/evm/signer/signer.go
No instances of the old signature with the additional pubkey parameter were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old TSS.Sign call pattern
# that might still include the optional pubkey parameter
# Search for any remaining Sign calls with 6 parameters
rg -p 'Sign\([^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*\)' --type go
# Search for specific pattern of empty string as last parameter
rg -p 'Sign\([^)]*,\s*""[^)]*\)' --type go
Length of output: 108
Script:
#!/bin/bash
# Let's try a different approach to find TSS Sign method implementations and usages
# Find TSS interface or struct definitions with Sign method
ast-grep --pattern 'type $_ interface {
$$$
Sign($$$)
$$$
}'
# Find all Sign method implementations
ast-grep --pattern 'func ($_ $_) Sign($$$) {
$$$
}'
# Find direct Sign method calls
rg -p "\.Sign\(" --type go -A 2
Length of output: 17870
zetaclient/chains/ton/observer/outbound.go (2)
178-178: Verify TSS address validation and add test coverage
The change from EVMAddress() to PubKey().AddressEVM() aligns with the TSS package revamp, but requires careful verification:
- This is a critical security check for withdrawal validation
- The line lacks test coverage according to static analysis
Would you like help creating test cases for this critical validation check? We should include:
- Test with valid TSS signer
- Test with invalid signer
- Test with edge cases (zero address, etc.)
✅ Verification successful
TSS address validation change is consistently implemented across the codebase
The migration from EVMAddress() to PubKey().AddressEVM() has been consistently implemented across all relevant components:
- EVM chain observers (inbound/outbound)
- TON chain observers (inbound/outbound)
- Solana chain observers
- EVM signers
The change in zetaclient/chains/ton/observer/outbound.go aligns with the broader codebase implementation and is covered by tests in inbound_test.go.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of PubKey().AddressEVM() across the codebase
rg "TSS\(\)\.EVMAddress\(\)" --type go
rg "TSS\(\)\.PubKey\(\)\.AddressEVM\(\)" --type go
Length of output: 1272
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 178-178: zetaclient/chains/ton/observer/outbound.go#L178
Added line #L178 was not covered by tests
195-195: Add test coverage for outbound tracker validation
The signer validation in addOutboundTracker is consistent with determineReceiveStatus, but also lacks test coverage. This is particularly important as it affects outbound transaction tracking.
Would you like assistance creating comprehensive test cases for the outbound tracker? Key scenarios to cover:
- Valid outbound tracking flow
- Invalid signer handling
- Integration with the broader TSS functionality
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-195: zetaclient/chains/ton/observer/outbound.go#L195
Added line #L195 was not covered by tests
zetaclient/chains/evm/signer/signer_admin_test.go (2)
19-20: LGTM: Consistent TSS mock instantiation with testing context
The uniform adoption of mocks.NewTSS(t) across all test functions improves test failure reporting by including the testing context.
Also applies to: 100-101, 148-149, 213-214, 289-290
Line range hint 1-328: LGTM: Comprehensive test coverage
The test suite demonstrates thorough coverage with:
- Positive test cases for all command types
- Error cases for invalid parameters
- Error cases for TSS failures
- Clear test case names and assertions
zetaclient/chains/interfaces/interfaces.go (3)
29-29: LGTM: Import aligns with TSS package revamp
The addition of the TSS package import supports the new type system and aligns with the package restructuring objectives.
114-119: LGTM: Clear and well-structured method signature
The PostVoteTSS method signature is well-defined with descriptive parameter names and appropriate types.
236-238: Verify implementation compatibility with interface changes
The interface changes improve type safety and simplify the API by:
- Using a strongly-typed
tss.PubKeyinstead of raw bytes - Removing the unused
optionalPubkeyparameter as per Issue TSSSigner: check ifoptionalPubkeyused or remove otherwise #2085
Let's verify the impact of these changes:
✅ Verification successful
Interface changes have been properly implemented across all chain signers
The verification confirms that the TSSSigner interface changes are correctly implemented:
- All chain signers (EVM, Bitcoin, Solana) use the TSS signer through the base.Signer embedding, which handles the interface compatibility
- The PubKey method is consistently used with tss.PubKey return type
- No remaining references to the removed optionalPubkey parameter were found
- Sign and SignBatch methods are properly implemented with the updated signature across all implementations
The changes improve type safety and API consistency without breaking any implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for implementations of TSSSigner interface and usage of removed optionalPubkey parameter
# Find all implementations of TSSSigner interface
echo "Checking TSSSigner implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) PubKey() tss.PubKey {
$$$
}'
# Find any remaining usage of optionalPubkey parameter
echo "Checking for remaining optionalPubkey usage..."
rg -l "optionalPubkey.*string"
Length of output: 312
Script:
#!/bin/bash
# Let's try a different approach to find implementations and usages
# Find all files that might implement TSSSigner
echo "=== Files containing TSSSigner ==="
rg -l "TSSSigner"
# Find implementations of Sign method which might have optionalPubkey
echo -e "\n=== Sign method implementations ==="
ast-grep --pattern 'func ($_ $_) Sign(ctx context.Context, data []byte, height, nonce uint64, chainID int64) ([65]byte, error) {
$$$
}'
# Find any PubKey method implementations
echo -e "\n=== PubKey method implementations ==="
rg -A 5 "func .* PubKey\(\)"
# Find any remaining references to optionalPubkey in the entire codebase
echo -e "\n=== optionalPubkey references ==="
rg "optionalPubkey"
Length of output: 2189
Script:
#!/bin/bash
# Let's check the actual implementations in the signer files we found
echo "=== Checking EVM signer implementation ==="
cat zetaclient/chains/evm/signer/signer.go
echo -e "\n=== Checking Bitcoin signer implementation ==="
cat zetaclient/chains/bitcoin/signer/signer.go
echo -e "\n=== Checking Solana signer implementation ==="
cat zetaclient/chains/solana/signer/signer.go
# Also check for any old commits or changes related to optionalPubkey
echo -e "\n=== Checking recent changes related to Sign method ==="
git log -n 5 --oneline --grep="Sign"
Length of output: 46148
zetaclient/chains/bitcoin/observer/observer_test.go (1)
129-129: Verify mock behavior alignment with new TSS implementation
The change from mocks.NewTSSMainnet() to mocks.NewTSS(t) across test cases aligns with the TSS package revamp. However, we should verify that the new mock implementation maintains the same behavior expected by these tests.
Also applies to: 137-137, 146-146
✅ Verification successful
Let me gather more information about the TSS mock implementation.
Let me gather one final piece of information about the mock implementation.
Mock TSS implementation change is consistent and well-adopted
The change from mocks.NewTSSMainnet() to mocks.NewTSS(t) is part of a broader standardization of TSS mocking across the codebase. The evidence shows:
- The new
mocks.NewTSS(t)is consistently used across multiple test files in different packages - No remaining instances of
NewTSSMainnet()were found - The change aligns with testing best practices by accepting a testing.T parameter for better error reporting
- The mock is used consistently in various test scenarios including Bitcoin, EVM, and Solana chains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the behavior consistency between old and new TSS mock implementations
# Search for any remaining uses of NewTSSMainnet
echo "Checking for any remaining NewTSSMainnet usage..."
rg "NewTSSMainnet\(\)"
# Search for the new mock implementation to verify its interface
echo "Checking new TSS mock implementation..."
ast-grep --pattern 'func NewTSS($t $_) $_'
Length of output: 267
Script:
#!/bin/bash
# Search for TSS mock implementation in mocks directory
echo "Searching for TSS mock implementation..."
fd "mock.*\.go" | xargs rg "type TSS|NewTSS"
# Search for TSS interface to understand expected behavior
echo -e "\nSearching for TSS interface definition..."
rg "type TSS interface"
# Look for test files using TSS mocks
echo -e "\nSearching for test files using TSS mocks..."
rg "mocks\.NewTSS" -A 2
Length of output: 10343
Script:
#!/bin/bash
# Search for the mock TSS implementation details
echo "Searching for mock TSS implementation details..."
fd "mock.*\.go" -t f | xargs cat
# Search for any test failures related to TSS mocks
echo -e "\nSearching for test failures or issues..."
rg "TestMain|func Test.*TSS" -A 5
Length of output: 16580
zetaclient/chains/solana/observer/outbound.go (1)
310-312: LGTM! Verify consistent TSS address retrieval across codebase.
The change to use PubKey().AddressEVM() aligns with the standardization of TSS address retrieval. This is a good improvement for consistency.
Let's verify the consistent usage of this pattern across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
TSS address retrieval standardization verified across codebase
The change to use PubKey().AddressEVM() is part of a consistent pattern across the codebase. The search results show this pattern is widely adopted in:
- EVM chain observers and signers
- Solana observers
- TON observers
- Base observers
- TSS setup and verification code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct EVMAddress calls and verify consistent PubKey().AddressEVM() usage
# Search for direct EVMAddress calls that might need updating
rg "\.EVMAddress\(\)" -A 3
# Search for the new standardized pattern to confirm adoption
rg "\.PubKey\(\)\.AddressEVM\(\)" -A 3
Length of output: 51636
zetaclient/chains/ton/observer/inbound_test.go (1)
268-268: LGTM! Verify consistent usage across codebase.
The change from EVMAddress() to PubKey().AddressEVM() aligns with the TSS package revamp objectives.
Let's verify the consistent usage of this pattern across the codebase:
✅ Verification successful
Consistent usage of PubKey().AddressEVM() pattern verified
The codebase shows consistent usage of the new pattern PubKey().AddressEVM() across all TSS-related functionality, particularly in:
- Chain observers (EVM, TON, Solana)
- TSS signers and setup
- Test assertions
The old EVMAddress() occurrences are in different contexts (e2e tests, config, runner) and are not related to TSS functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct EVMAddress() calls and verify consistent usage of PubKey().AddressEVM()
# Search for old pattern
echo "Checking for any remaining EVMAddress() calls:"
rg "EVMAddress\(\)"
# Search for new pattern
echo -e "\nVerifying consistent usage of new pattern:"
rg "PubKey\(\)\.AddressEVM\(\)"
Length of output: 20519
zetaclient/orchestrator/bootstap_test.go (2)
33-33: LGTM: Mock initialization change aligns with TSS package revamp
The change from NewTSSMainnet() to NewTSS(t) improves the test setup by:
- Making the mock more generic and reusable
- Adding test context for better cleanup handling
- Aligning with the broader TSS package improvements
Also applies to: 198-198
Line range hint 1-500: Verify test coverage for new TSS functionality
Since this is part of the TSS package revamp, let's verify the test coverage for the new TSS implementation.
✅ Verification successful
TSS implementation has comprehensive test coverage
Based on the analysis of the codebase:
- Core TSS implementation in
pkg/crypto/tss.gohas dedicated test coverage inpkg/crypto/tss_test.go - Mock implementations exist for testing across multiple packages
- Extensive test coverage for TSS operations including:
- Sign/SignBatch operations
- Keygen functionality
- Integration with observers and signers
- Protocol-specific TSS implementations (EVM, Solana)
The test file under review (bootstrap_test.go) appropriately uses the mock TSS implementation from zetaclient/testutils/mocks/tss.go for testing the orchestrator's TSS integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for TSS-related code
# Find all TSS-related test files
echo "Finding TSS-related test files..."
fd -e go -E "*mock*" -E "*fake*" tss
# Check for TSS interface usage
echo "Checking TSS interface usage..."
rg -A 3 "interface.*TSS.*{"
# Look for potential untested TSS methods
echo "Looking for untested TSS methods..."
ast-grep --pattern 'func ($x *TSS) $method($$$) {
$$$
}'
Length of output: 1828
Script:
#!/bin/bash
# Let's check for TSS interface and implementation test coverage more precisely
# Find TSS interface definition
echo "=== TSS Interface Definition ==="
rg -p "type\s+TSS\s+interface\s*{"
# Find TSS implementation test files
echo -e "\n=== TSS Implementation Tests ==="
fd -e go test tss
# Check test coverage in pkg/crypto where TSS implementation likely exists
echo -e "\n=== TSS Implementation Details ==="
rg -p "type\s+TSS\s+struct\s*{"
# Look for test functions related to TSS
echo -e "\n=== TSS Test Functions ==="
rg -p "func Test.*TSS.*\(t \*testing.T\)"
# Check mock implementations
echo -e "\n=== TSS Mocks ==="
fd mock.*tss -e go
Length of output: 2458
Script:
#!/bin/bash
# Let's check the TSS interface and implementation with fixed patterns
# Find TSS interface
echo "=== TSS Interface ==="
rg "type TSS interface" -A 5
# Find TSS struct implementation
echo -e "\n=== TSS Implementation ==="
rg "type TSS struct" -A 5
# Check TSS-related files in pkg/crypto
echo -e "\n=== TSS Core Implementation ==="
fd -t f "tss" pkg/crypto/
# Look for TSS mock implementations
echo -e "\n=== TSS Mocks ==="
rg "Mock.*TSS" -l
# Check test coverage for specific TSS operations
echo -e "\n=== TSS Operation Tests ==="
rg "func.*TSS.*(Sign|Keygen|KeyShare)" -A 2
Length of output: 5776
zetaclient/chains/bitcoin/signer/signer.go (2)
Line range hint 158-162: LGTM: Improved TSS address retrieval pattern
The new method TSS().PubKey().AddressBTC() provides a clearer and more logical path to derive the Bitcoin address from the TSS public key.
307-307: Add test coverage for public key serialization
While the new PubKey().Bytes(true) method provides more flexibility for key serialization, this critical path lacks test coverage. Consider adding test cases that verify:
- Compressed public key format
- Correct byte serialization
- Integration with transaction signing
✅ Verification successful
Let me analyze the test coverage patterns and file structure before making a conclusion about the verification status.
I notice there's a usage of PubKey().Bytes(true) in signer_keysign_test.go, but I need to verify the complete test coverage context before making a final determination.
Public key serialization is adequately tested
The change from PubKeyCompressedBytes() to PubKey().Bytes(true) is properly covered in the test suite:
signer_keysign_test.gocontains comprehensive tests for public key serialization in the Bitcoin signing flow:- Tests both TSS and wallet signatures for comparison
- Verifies the compressed public key format in witness script generation
- Validates the serialized transaction with the public key
The test coverage adequately verifies the correctness of the public key serialization change, including its integration with transaction signing and witness script generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage for public key serialization
rg -A 5 "PubKey\(\)\.Bytes\(true\)" "*_test.go"
Length of output: 99
Script:
#!/bin/bash
# Search for test files containing public key serialization tests
fd -e go -E "vendor/*" "test" | xargs rg -l "PubKey|TestSign"
# Search for existing test coverage patterns
fd -e go -E "vendor/*" "test" | xargs rg -A 5 "PubKeyCompressedBytes|Bytes\(true\)"
# Find the test file corresponding to the signer package
fd -e go "signer_test.go$"
Length of output: 2984
Script:
#!/bin/bash
# Check the content of bitcoin signer test files for public key serialization coverage
cat zetaclient/chains/bitcoin/signer/signer_test.go
cat zetaclient/chains/bitcoin/signer/signer_keysign_test.go
# Look for other potential public key serialization test patterns
rg -A 5 "TestSign.*Bitcoin" zetaclient/chains/bitcoin/signer/
Length of output: 17014
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 307-307: zetaclient/chains/bitcoin/signer/signer.go#L307
Added line #L307 was not covered by tests
zetaclient/chains/base/observer.go (3)
231-235: LGTM: Improved Bitcoin address retrieval pattern
The change to use PubKey().AddressBTC() follows a more structured approach for address generation, properly handling chain-specific requirements and error cases.
237-237: LGTM: Consistent EVM address retrieval pattern
The change to use PubKey().AddressEVM() maintains consistency with the new pattern of accessing addresses through the PubKey interface.
231-237: Verify consistent usage of new TSS address retrieval pattern
Let's ensure all TSS address retrievals have been updated to use the new pattern across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
TSS address retrieval pattern has been consistently updated
The codebase shows consistent usage of the new TSS address retrieval pattern:
PubKey().AddressBTC()for Bitcoin addressesPubKey().AddressEVM()for EVM addresses
The old methods BTCAddress() and EVMAddress() are not found in the codebase, indicating a complete migration to the new pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of old TSS address retrieval methods
# Search for old BTCAddress method usage
echo "Checking for old BTCAddress usage..."
rg "\.BTCAddress\("
# Search for old EVMAddress method usage
echo "Checking for old EVMAddress usage..."
rg "\.EVMAddress\("
# Search for new PubKey().Address pattern usage
echo "Checking for new PubKey().Address pattern usage..."
rg "PubKey\(\)\.Address(BTC|EVM)"
Length of output: 19312
zetaclient/chains/bitcoin/observer/observer.go (1)
356-356: Add test coverage for the TSS address retrieval.
The modification to use PubKey().AddressBTC() aligns with the TSS package revamp. However, this critical path for UTXO management lacks test coverage.
Would you like me to help create comprehensive test cases for the TSS address retrieval functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 356-356: zetaclient/chains/bitcoin/observer/observer.go#L356
Added line #L356 was not covered by tests
zetaclient/chains/solana/observer/outbound_test.go (2)
10-10: LGTM: Structured logging and test constants
The addition of zerolog for structured logging and the test public key constant are appropriate for the test context.
Also applies to: 39-39
58-60: LGTM: Enhanced test logging setup
The test helper now properly initializes structured logging with test writer, improving log capture during tests.
Also applies to: 64-64
zetaclient/chains/bitcoin/observer/outbound_test.go (2)
52-54: 🛠️ Refactor suggestion
Enhance test key security
The test private key is hardcoded. Consider moving sensitive test data to a dedicated test fixtures file or environment variables.
-skHex := "7b8507ba117e069f4a3f456f505276084f8c92aee86ac78ae37b4d1801d35fa8"
+skHex := testutils.GetTestPrivateKey()Likely invalid or redundant comment.
Line range hint 1-518: Verify test coverage completeness
The test file has comprehensive coverage but should verify integration with the broader codebase.
✅ Verification successful
Let me gather more information about the implementation details to verify the test coverage completeness.
Let me check for any additional test cases or edge cases that might be missing.
Test coverage is comprehensive and well-aligned with implementation
The test coverage is thorough and properly validates all critical aspects of the Bitcoin UTXO and TSS handling:
- Comprehensive UTXO selection test cases covering bootstrap, consolidation, and edge cases
- Extensive TSS vout validation for both normal and cancelled transactions
- Proper nonce-mark UTXO handling and verification
- Error cases for insufficient funds, invalid addresses, and incorrect amounts
- Integration with the broader codebase through proper mock implementations
The test file demonstrates production-grade quality with:
- Systematic test organization
- Clear test case descriptions
- Proper error handling validation
- Comprehensive edge case coverage
- Accurate mock implementations matching production behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage and related files
echo "Checking test coverage..."
rg -l "TestUTXO|TestTSSVout" --type go
echo "Checking related implementations..."
rg -l "SelectUTXOs|checkTSSVout" --type go
echo "Checking mock usage..."
ast-grep --pattern 'NewTSSFromPrivateKey($$$)'
Length of output: 525
Script:
#!/bin/bash
# Check implementation details of SelectUTXOs and checkTSSVout
echo "Checking SelectUTXOs implementation..."
rg "func.*SelectUTXOs" -A 5 zetaclient/chains/bitcoin/observer/outbound.go
echo "Checking checkTSSVout implementation..."
rg "func.*checkTSSVout" -A 5 zetaclient/chains/bitcoin/observer/outbound.go
echo "Checking signer usage..."
rg "func.*SelectUTXOs" -A 5 zetaclient/chains/bitcoin/signer/signer.go
Length of output: 1195
Script:
#!/bin/bash
# Check for edge cases and error handling in implementation
echo "Checking error handling in SelectUTXOs..."
rg "return.*err" -B 3 zetaclient/chains/bitcoin/observer/outbound.go
echo "Checking UTXO validation..."
rg "utxos.*:=" zetaclient/chains/bitcoin/observer/outbound.go
echo "Checking TSS validation..."
rg "TSS\(\).*Address" zetaclient/chains/bitcoin/observer/outbound.go
echo "Checking nonce handling..."
rg "nonce.*mark" -B 2 zetaclient/chains/bitcoin/observer/outbound.go
Length of output: 6758
zetaclient/chains/evm/signer/signer.go (1)
189-189: LGTM: Improved TSS signing interface
The changes simplify the TSS signing interface by removing the unused optionalPubkey parameter and improve logging by using a more direct method to access the EVM address.
Also applies to: 200-200
zetaclient/chains/base/observer_test.go (2)
42-42: LGTM: Consistent mock instantiation pattern
The change to use mocks.NewTSS(t) across all test cases improves consistency and provides better test assertion capabilities through the testing.T parameter.
Also applies to: 71-71, 189-189, 305-305, 384-384, 390-390
Line range hint 1-650: LGTM: Well-structured test suite
The test suite demonstrates good practices:
- Clear test case organization
- Consistent test patterns
- Comprehensive coverage including error cases
- Proper use of test utilities and mocks
zetaclient/chains/evm/observer/outbound.go (2)
504-517: LGTM: Improved error handling structure
The switch-case structure provides clear separation of error cases and improved readability. The comment about TSS upgrade adds valuable operational context.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 505-505: zetaclient/chains/evm/observer/outbound.go#L505
Added line #L505 was not covered by tests
[warning] 508-517: zetaclient/chains/evm/observer/outbound.go#L508-L517
Added lines #L508 - L517 were not covered by tests
452-452: Verify consistent usage of TSS address access pattern
The change from EVMAddress() to PubKey().AddressEVM() appears to be part of the TSS package revamp. Let's ensure this pattern is consistently applied across the codebase.
✅ Verification successful
Change from EVMAddress() to PubKey().AddressEVM() is correctly implemented
The search results confirm that the change from EVMAddress() to PubKey().AddressEVM() is consistently applied across all TSS-related code in the codebase. The remaining instances of EVMAddress() are in different contexts (e.g., e2e tests, runner configurations) and are unrelated to TSS functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of EVMAddress() to ensure consistent usage
rg "EVMAddress\(\)" --type go
# Search for the new pattern to confirm adoption
rg "PubKey\(\)\.AddressEVM\(\)" --type go
Length of output: 20335
zetaclient/chains/evm/observer/inbound_test.go (2)
Line range hint 1-467: LGTM! Comprehensive test coverage with well-structured test cases.
The test suite demonstrates thorough coverage of inbound transaction handling:
- Positive and negative test cases for each token type (Zeta, ERC20, Gas)
- Edge cases and error conditions are well-handled
- Clear documentation with real transaction examples
Also applies to: 469-600
468-468: Verify the consistency of TSS mock initialization across test files.
The change to use mocks.NewTSS(t) centralizes mock creation, which aligns with the TSS package revamp objectives.
Let's verify that this pattern is consistently used across other test files:
✅ Verification successful
TSS mock initialization pattern is consistently used across test files
The verification confirms that all test files consistently use mocks.NewTSS(t) for TSS mock initialization. No instances of direct struct initialization (mocks.TSS{} or &mocks.TSS{}) were found in test files, indicating proper adherence to the centralized mock creation pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent TSS mock initialization patterns
# Expected: All TSS mock initializations should use NewTSS(t)
# Search for any TSS mock initialization patterns
rg --type go "NewTSS|mock.*TSS|TSS.*mock" -A 2 -B 2
# Search for specific old patterns that should be replaced
rg --type go "mock\.TSS{|&mock\.TSS{" -A 2 -B 2
Length of output: 37215
Script:
#!/bin/bash
# Let's search for TSS mock initialization patterns more specifically
# Looking for both the new pattern and potential old patterns
# Search for NewTSS usage in test files
rg --type go "mocks\.NewTSS\(" -g "*_test.go"
# Search for direct TSS struct initialization in test files
rg --type go "mocks\.TSS\{|&mocks\.TSS\{|mock\.TSS\{|&mock\.TSS\{" -g "*_test.go"
# Search for any TSS-related mock setup in test files
ast-grep --pattern 'tss := $INIT' -l go
Length of output: 15408
zetaclient/chains/bitcoin/observer/outbound.go (1)
563-563: Add test coverage for TSS public key encoding.
The change in TSS public key encoding method requires test coverage to ensure compatibility with the new TSS package implementation.
Would you like me to help create test cases for this change? The tests should verify:
- The compressed public key format is correct
- The encoding is compatible with Bitcoin witness scripts
- The key can be properly used in transaction verification
✅ Verification successful
Let me analyze the test coverage and implementation details before making a final determination about the review comment's validity.
TSS public key encoding is adequately tested
The change in TSS public key encoding is already covered by existing tests:
- Direct TSS key usage tests in
signer_keysign_test.go - Integration tests in
outbound_test.gothat verify the encoded key works correctly with Bitcoin address generation - The tests validate both mainnet and testnet address derivation using the TSS public key
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are existing tests for TSS public key encoding
rg -g '**/*_test.go' -A 5 'PubKey\(\)\.Bytes\(true\)'
Length of output: 601
Script:
#!/bin/bash
# Check for test coverage of the outbound observer's TSS key usage
rg -g '**/*_test.go' -A 5 'checkTSSVin|TSS\(\)\.PubKey' zetaclient/chains/bitcoin/observer/
# Check implementation details of TSS key usage in the codebase
ast-grep --pattern 'func checkTSSVin($$$)'
Length of output: 1312
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 563-563: zetaclient/chains/bitcoin/observer/outbound.go#L563
Added line #L563 was not covered by tests
zetaclient/testutils/mocks/zetacore_client.go (1)
866-892: LGTM! The PostVoteTSS mock implementation follows best practices.
The implementation:
- Correctly handles both function and direct return values
- Includes proper panic handling for unspecified return values
- Maintains consistency with other mock methods in the file
zetaclient/chains/evm/observer/inbound.go (1)
617-617: Verify consistent usage of TSS address access pattern
The change from EVMAddress() to PubKey().AddressEVM() appears to be part of the TSS package revamp. While the change is consistent across both occurrences in this file, we should verify this pattern across the codebase.
Also applies to: 809-809
✅ Verification successful
TSS address access pattern is consistently updated across the codebase
The verification shows that the old pattern TSS().EVMAddress() is not present anywhere in the codebase, while the new pattern TSS().PubKey().AddressEVM() is consistently used across multiple components including EVM, TON, and Solana chains. The changes in the inbound observer align with this codebase-wide pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old TSS address access pattern
# and verify the consistency of the new pattern
echo "Checking for old pattern usage..."
rg "TSS\(\)\.EVMAddress\(\)"
echo "Verifying new pattern usage..."
rg "TSS\(\)\.PubKey\(\)\.AddressEVM\(\)"
Length of output: 1400
zetaclient/tss/setup.go (2)
241-245: Verify the necessity of calling tssServer.Start()
The comment suggests that tssServer.Start() only logs a message without additional effects. If the method does not perform essential initialization, consider removing it to simplify the code. However, ensure that there are no side effects or future dependencies on this call.
Please confirm whether tssServer.Start() is required for any underlying processes or future updates. If redundant, removing it can streamline the setup process.
8-8:
Ensure proper import of the slices package
The slices package is imported without a module path. Verify that it is part of the standard library for the Go version in use (Go 1.21 or later). If not, specify the full import path to avoid compilation errors.
If using a Go version earlier than 1.21, update the import as follows:
- import (
- "slices"
+ import (
+ "golang.org/x/exp/slices"Ensure that the module is included in go.mod:
✅ Verification successful
The slices package import is correctly used
The codebase is using Go 1.22.7 (as specified in go.mod), which includes the slices package in the standard library. The package is being used correctly for standard slice operations like Sort, SortFunc, Contains, and Reverse across multiple files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
go get golang.org/x/exp/slices
Length of output: 125
Script:
#!/bin/bash
# Check the Go version specified in go.mod file
fd "go.mod" --exec cat {}
# Check if slices package is used in the codebase
rg "slices\." -A 2 -B 2
Length of output: 20269
zetaclient/tss/keygen.go (1)
307-307:
Avoid wrapping a nil error in errors.Wrapf
In the TestKeySign function, when the key signing is unsuccessful but err is nil, wrapping err (which is nil) with errors.Wrapf results in an error with a nil cause. This can lead to confusion during debugging.
Proposed fix:
Use errors.Errorf instead of errors.Wrapf when err is nil.
-return errors.Wrapf(err, "key signing is not successful (status %d)", res.Status)
+return errors.Errorf("key signing is not successful (status %d)", res.Status)Likely invalid or redundant comment.
|
I suppose TSS migration test was broken due to this change |
swift1337
force-pushed
the
feat/zetaclient/tss-improvements
branch
from
6634a49 to
7d9c96b
Compare
|
@fbac filed an issue regarding failing tests |
* Streamline tss config * Revamp TSS keygen ceremony code * Refactor tss key sign test; streamline sig verification * Remove optional pub key (tss) * Implement PubKey entity * Implement new TSS service layer. Add unit tests * Add prometheus metrics * Restructure files * Implement TSS Setup Flow * Adapt some test cases; fix typo * Merge pubkey.go with crypto.go * Add tss key pass alongside with hotkey pass * Simplify start.go; drop old tss from zetaclient * Fix cyclic imports * Revamp TSS mock. Fix unit tests across zetaclient * Remove tss historical list from outbound evm cctx * TSS healthcheck * Fix flaky case when GetTSS() returns an empty string * Update changelog * Fix typos * Forward docker pprof in e2e * Forward pprof in e2e docker * Simplify VerifySignature * Fix typo * Fix batch signature verification * Add readme * Fix typo * Fix typos in healthcheck * Address PR comments [1] * Address PR comments [2] * Address PR comments [3] (improve errors clarity) * Address PR comments [4] * Remove redundant methods from app context * Address PR comments [5] * Address PR comments [6] (test coverage) * Fix TSS migration test * Fix btc issue
Context
This PR rewrites
zetaclient/tssalmost from scratch, drastically simplifying and streamlining logic, improving logging, and increasing code readability.PR changes
There are three main parts:
cmd/zetaclientd/start.go:start()command now less code because tss part was moved to its own package filezetaclient/tss/setup.go: contains the consecutive flow of TSS server, TSS service, keygen ceremony, and other partszetaclient/tss/*: TSS service and its utilitiesThe rest is mostly about adapting the new interface and fixing test cases.
Tasks
optionalPubkeytss/healthcheck.gotss/readme.mdOld Interface
New interface
Closes #3119
Closes #2085
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests
Documentation