Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: derive Bitcoin TSS address by chain ID; added more Signet static info in 'chains' package #2907

Merged

Conversation

ws4charlie
Copy link
Contributor

@ws4charlie ws4charlie commented Sep 23, 2024

Description

  • To support multiple Bitcoin chains, we need to derive TSS address differently by Bitcoin chain id.
  • Added a few missed Signet info to chains package.
  • Consolidate BTCAddressWitnessPubkeyHash and BTCAddress into one single method BTCAddress(chainID int64) for simplicity.

Closes: #1397

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Summary by CodeRabbit

  • New Features

    • Added functionality to derive Bitcoin TSS addresses based on chain ID.
    • Enhanced support for the Bitcoin Signet testnet.
    • Introduced a new constant for TSS public key for Athens3.
  • Bug Fixes

    • Improved error handling during TSS address validation.
  • Tests

    • Added new test cases for Bitcoin addresses and validation scenarios related to TSS public keys.
  • Documentation

    • Updated changelog to reflect new functionalities and improvements.

Copy link
Contributor

coderabbitai bot commented Sep 23, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Walkthrough

The changes introduced in this pull request enhance the ZetaClient's support for multiple Bitcoin chains, specifically by adding functionality for deriving Bitcoin TSS addresses based on chain IDs. The updates include modifications across various files to streamline address retrieval, validation, and testing processes, ensuring better handling of Bitcoin Signet and other chains. Additionally, the configuration management has been adjusted to accommodate multiple Bitcoin chains, aligning with ongoing efforts to future-proof the system.

Changes

Files Change Summary
changelog.md Updated to include new functionality for deriving Bitcoin TSS addresses and added static info for Signet.
cmd/zetaclientd/start.go Modified start function to filter supported Bitcoin chain IDs and update TSS address validation.
pkg/chains/bitcoin.go Added BitcoinSignetParams and updated functions to support Bitcoin Signet network.
pkg/chains/bitcoin_test.go Introduced new test cases for Bitcoin Signet network in existing test functions.
zetaclient/chains/base/observer.go Updated OutboundID method to retrieve Bitcoin address using chain ID as a parameter.
zetaclient/chains/base/observer_test.go Adjusted expected outbound ID construction in tests to include chain ID.
zetaclient/chains/bitcoin/observer/inbound.go Modified TSS address retrieval in ObserveInbound method to include chain ID.
zetaclient/chains/bitcoin/observer/observer.go Changed address retrieval logic in FetchUTXOs method to simplify and enhance error handling.
zetaclient/chains/bitcoin/observer/outbound.go Updated methods to include chain ID in TSS address retrieval for outbound processes.
zetaclient/chains/bitcoin/signer/signer.go Altered address retrieval in AddWithdrawTxOutputs to require chain ID.
zetaclient/tss/tss_signer.go Removed BitcoinChainID from TSS struct and updated methods to handle multiple chains.
zetaclient/tss/tss_signer_test.go Added new tests for validating Bitcoin addresses and TSS functionality.

Sequence Diagram(s)

Loading
sequenceDiagram
    participant User
    participant ZetaClient
    participant Observer
    participant TSS

    User->>ZetaClient: Request Bitcoin TSS Address
    ZetaClient->>Observer: Retrieve Outbound ID
    Observer->>TSS: Get BTC Address with Chain ID
    TSS-->>Observer: Return BTC Address
    Observer-->>ZetaClient: Return Outbound ID
    ZetaClient-->>User: Provide Bitcoin TSS Address

Assessment against linked issues

Objective Addressed Explanation
Update ZetaClient config to support multiple Bitcoin chains (Issue #1397)
Ensure proper handling of multiple Bitcoin chains in configuration

Possibly related issues

Possibly related PRs

Suggested labels

chain:bitcoin, SOLANA_TESTS

Suggested reviewers

  • fbac
  • kingpinXD
  • swift1337
  • lumtis
  • brewmaster012
  • skosito

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 79.16667% with 10 lines in your changes missing coverage. Please review.

Project coverage is 67.49%. Comparing base (3eba701) to head (3a111e0).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
zetaclient/chains/bitcoin/observer/rpc_status.go 0.00% 4 Missing ⚠️
zetaclient/chains/bitcoin/observer/observer.go 0.00% 3 Missing ⚠️
zetaclient/chains/bitcoin/signer/signer.go 33.33% 1 Missing and 1 partial ⚠️
zetaclient/chains/bitcoin/observer/inbound.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2907      +/-   ##
===========================================
+ Coverage    67.27%   67.49%   +0.21%     
===========================================
  Files          380      380              
  Lines        21175    21169       -6     
===========================================
+ Hits         14245    14287      +42     
+ Misses        6264     6213      -51     
- Partials       666      669       +3     
Files with missing lines Coverage Δ
pkg/chains/bitcoin.go 100.00% <100.00%> (ø)
x/observer/keeper/grpc_query_tss.go 86.44% <100.00%> (ø)
zetaclient/chains/base/observer.go 87.61% <100.00%> (+0.42%) ⬆️
zetaclient/chains/bitcoin/observer/outbound.go 33.08% <100.00%> (ø)
zetaclient/tss/tss_signer.go 21.98% <100.00%> (+12.42%) ⬆️
zetaclient/chains/bitcoin/observer/inbound.go 18.11% <0.00%> (ø)
zetaclient/chains/bitcoin/signer/signer.go 17.64% <33.33%> (-0.14%) ⬇️
zetaclient/chains/bitcoin/observer/observer.go 32.77% <0.00%> (+0.13%) ⬆️
zetaclient/chains/bitcoin/observer/rpc_status.go 0.00% <0.00%> (ø)

@ws4charlie ws4charlie marked this pull request as ready for review September 23, 2024 15:29
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

Outside diff range and nitpick comments (14)
zetaclient/chains/bitcoin/observer/rpc_status.go (1)

32-32: Approve change with recommendation for test coverage.

The modification to derive the Bitcoin TSS address using the chain ID aligns with the PR objectives and enhances the flexibility of the system. This change allows for support of multiple Bitcoin-based chains, as outlined in issue #1397.

However, it is imperative to ensure that this critical change is adequately covered by unit tests to maintain the robustness and reliability of the system.

To address the lack of test coverage flagged by the static analysis tool, I can assist in crafting a unit test for the checkRPCStatus function. Would you like me to provide a sample test case?

Tools
GitHub Check: codecov/patch

[warning] 32-32: zetaclient/chains/bitcoin/observer/rpc_status.go#L32
Added line #L32 was not covered by tests

pkg/chains/bitcoin.go (1)

Line range hint 1-58: Overall, the changes effectively add Signet support while maintaining code consistency.

The additions and modifications in this file successfully implement support for the Bitcoin Signet network, aligning with the PR objectives. The code maintains consistency with existing patterns and naming conventions.

To further enhance maintainability and potentially improve performance, consider a comprehensive refactoring of the Bitcoin network parameter handling. This could involve:

  1. Creating a single map that associates chain IDs with their respective network parameters and names.
  2. Refactoring both BitcoinNetParamsFromChainID and BitcoinChainIDFromNetworkName to use this map.
  3. Implementing helper functions for adding new networks, ensuring consistency across all related data structures.

This approach would centralize the network information, reduce duplication, and simplify future additions or modifications to supported Bitcoin networks.

pkg/chains/bitcoin_test.go (1)

Line range hint 1-77: Suggestion: Enhance test coverage for comprehensive network validation.

The test file demonstrates good coverage for various Bitcoin networks. To further improve the robustness of the test suite, consider adding the following:

  1. Implement and test IsBitcoinTestnet function.
  2. Implement and test IsBitcoinSignet function.

These additions would ensure consistent validation across all supported Bitcoin networks, aligning with the PR's objective of enhancing multi-chain support.

Example implementation:

func TestIsBitcoinTestnet(t *testing.T) {
    require.True(t, IsBitcoinTestnet(BitcoinTestnet.ChainId))
    require.False(t, IsBitcoinTestnet(BitcoinMainnet.ChainId))
    require.False(t, IsBitcoinTestnet(BitcoinRegtest.ChainId))
    require.False(t, IsBitcoinTestnet(BitcoinSignetTestnet.ChainId))
}

func TestIsBitcoinSignet(t *testing.T) {
    require.True(t, IsBitcoinSignet(BitcoinSignetTestnet.ChainId))
    require.False(t, IsBitcoinSignet(BitcoinMainnet.ChainId))
    require.False(t, IsBitcoinSignet(BitcoinRegtest.ChainId))
    require.False(t, IsBitcoinSignet(BitcoinTestnet.ChainId))
}

Ensure to implement the corresponding IsBitcoinTestnet and IsBitcoinSignet functions in the main package if they don't already exist.

zetaclient/testutils/constant.go (1)

20-21: Approve addition of TSSPubkeyAthens3 constant with suggestions for improvement

The addition of the TSSPubkeyAthens3 constant is appropriate and consistent with the existing code structure. However, to enhance code maintainability and security, please consider the following recommendations:

  1. Add a comment explaining the purpose and usage of this constant, similar to the comments provided for other constants in this file.

  2. Regarding the static analysis tool's flag for a potential API key exposure:
    While this appears to be a false positive as the value represents a public key rather than a sensitive API key, it's crucial to ensure that this is indeed a public key and not inadvertently exposing any sensitive information.

To address these points, consider modifying the code as follows:

// TSSPubkeyAthens3 is the TSS public key for Athens3 network used in testing
// This key is used for [brief explanation of its purpose]
TSSPubkeyAthens3 = "zetapub1addwnpepq28c57cvcs0a2htsem5zxr6qnlvq9mzhmm76z3jncsnzz32rclangr2g35p"
Tools
Gitleaks

21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

zetaclient/testutils/mocks/tss_signer.go (2)

Line range hint 128-163: Address inconsistencies in BTCAddressWitnessPubkeyHash implementation.

The method signature has been updated with an unused int64 parameter, while the implementation uses s.chain.ChainId. This inconsistency may lead to confusion and potential bugs.

Consider the following improvements:

  1. Utilize the parameter in the implementation:

    func (s *TSS) BTCAddressWitnessPubkeyHash(chainID int64) *btcutil.AddressWitnessPubKeyHash {
        // ... (existing implementation)
        
        net, err := chains.GetBTCChainParams(chainID)
        if err != nil {
            fmt.Printf("error getting btc chain params: %v", err)
            return nil
        }
        
        tssAddress := s.BTCAddress(chainID)
        // ... (rest of the implementation)
    }
  2. If the parameter is not needed, remove it and use s.chain.ChainId consistently:

    func (s *TSS) BTCAddressWitnessPubkeyHash() *btcutil.AddressWitnessPubKeyHash {
        // ... (keep the current implementation)
    }

Ensure that all callers of this method are updated accordingly, and that the usage of chainID is consistent across related methods.


Line range hint 1-200: Comprehensive review of changes and suggestions for improvement.

The modifications to BTCAddress and BTCAddressWitnessPubkeyHash methods introduce inconsistencies and don't fully address the objective of supporting multiple Bitcoin chains based on chain ID.

To align with the PR objectives and improve the implementation:

  1. Implement a consistent approach for handling chain IDs across all relevant methods.

  2. Consider introducing a chainConfig map or similar structure to store chain-specific information:

    type ChainConfig struct {
        Network *chaincfg.Params
        // Add other chain-specific fields as needed
    }
    
    var chainConfigs = map[int64]ChainConfig{
        chains.BitcoinMainnet.ChainId: {Network: &chaincfg.MainNetParams},
        chains.BitcoinTestnet3.ChainId: {Network: &chaincfg.TestNet3Params},
        // Add other supported chains
    }
  3. Update the TSS struct to support multiple chains:

    type TSS struct {
        // ... (existing fields)
        chainConfigs map[int64]ChainConfig
    }
  4. Refactor methods to utilize the chainConfig:

    func (s *TSS) BTCAddress(chainID int64) string {
        config, ok := s.chainConfigs[chainID]
        if !ok {
            return "" // or handle error
        }
        // Use config.Network for chain-specific logic
        // ...
    }
    
    func (s *TSS) BTCAddressWitnessPubkeyHash(chainID int64) *btcutil.AddressWitnessPubKeyHash {
        config, ok := s.chainConfigs[chainID]
        if !ok {
            return nil // or handle error
        }
        // Use config.Network instead of s.chain.ChainId
        // ...
    }

These changes will provide a more robust and flexible implementation for supporting multiple Bitcoin chains, aligning with the PR objectives and improving overall code quality.

zetaclient/chains/interfaces/interfaces.go (1)

260-261: Approve changes with documentation suggestion

The modifications to the BTCAddress and BTCAddressWitnessPubkeyHash method signatures in the TSSSigner interface are appropriate and align with the objective of supporting multiple Bitcoin chains. The addition of the chainID parameter enables the derivation of chain-specific Bitcoin addresses, which is crucial for multi-chain functionality.

To enhance code clarity and maintainability, I recommend adding documentation comments to these methods, explaining the purpose of the chainID parameter and any constraints or expectations regarding its values.

Consider adding documentation comments as follows:

// BTCAddress returns the Bitcoin address for the specified chain ID.
// chainID: The ID of the Bitcoin chain (e.g., mainnet, testnet, signet).
BTCAddress(chainID int64) string

// BTCAddressWitnessPubkeyHash returns the Bitcoin witness public key hash address for the specified chain ID.
// chainID: The ID of the Bitcoin chain (e.g., mainnet, testnet, signet).
BTCAddressWitnessPubkeyHash(chainID int64) *btcutil.AddressWitnessPubKeyHash
zetaclient/chains/bitcoin/signer/signer.go (1)

Line range hint 1-524: Suggestions for code improvements

While reviewing the changes, I noticed some areas in the existing code that could benefit from refactoring or improvement:

  1. The TODO comments (e.g., lines 78-79, 293) should be addressed or converted into GitHub issues for better tracking.

  2. The SignWithdrawTx function (starting at line 293) is quite long and complex. Consider refactoring it into smaller, more focused functions to improve readability and maintainability.

  3. Error handling in the TryProcessOutbound function (starting at line 421) could be improved by using a more structured approach, such as returning errors instead of logging them directly.

  4. The retry logic in the TryProcessOutbound function (lines 508-530) could be extracted into a separate helper function for better reusability.

  5. Consider using constants for magic numbers throughout the code (e.g., 1e8, 1e-8) to improve clarity and maintainability.

These suggestions are not directly related to the current changes but could be considered for future improvements to enhance the overall code quality.

zetaclient/chains/bitcoin/observer/observer.go (1)

372-374: Enhance test coverage for TSS address retrieval and usage

The static analysis hints indicate that the newly added lines for TSS address retrieval and its usage in ListUnspentMinMaxAddresses are not covered by tests.

To improve the robustness of the codebase, it is crucial to add unit tests covering these new changes. Would you like me to generate a unit test skeleton for the FetchUTXOs method, focusing on the TSS address retrieval and its usage? This can be implemented as a new GitHub issue to track the task.

Also applies to: 376-376

Tools
GitHub Check: codecov/patch

[warning] 372-374: zetaclient/chains/bitcoin/observer/observer.go#L372-L374
Added lines #L372 - L374 were not covered by tests

zetaclient/chains/bitcoin/observer/outbound_test.go (1)

64-65: Clarify test environment and chain selection

The use of chains.BitcoinTestnet.ChainId in a function named createObserverWithUTXOs may lead to confusion. Consider renaming the function to explicitly indicate that it's creating a testnet observer, or add a comment explaining the choice of testnet for this particular test setup.

-func createObserverWithUTXOs(t *testing.T) *Observer {
+func createTestnetObserverWithUTXOs(t *testing.T) *Observer {
   // Create Bitcoin observer
   ob := createObserverWithPrivateKey(t)
   tssAddress := ob.TSS().BTCAddressWitnessPubkeyHash(chains.BitcoinTestnet.ChainId).EncodeAddress()
+  // Using testnet for this test setup
changelog.md (2)

Line range hint 3-15: Consider enhancing changelog entries with more details

While the changelog provides a good overview of recent changes, some entries could benefit from more detailed descriptions. For instance:

  1. [2907] - "derive Bitcoin tss address by chain id and added more Signet static info" - It would be helpful to briefly explain the impact of this change on the system's functionality or performance.

  2. [2870] - "support for multiple Bitcoin chains in the zetaclient" - Consider elaborating on which specific Bitcoin chains are now supported and any limitations or considerations users should be aware of.

Adding these details would make the changelog more informative for users and developers alike.


Line range hint 17-24: Ensure backwards compatibility and update documentation for refactors

The refactoring efforts appear substantial and may have significant implications:

  1. [2890] - The refactoring of MsgUpdateChainInfo and addition of MsgRemoveChainInfo likely changes the API. Ensure that these changes are well-documented and that any breaking changes are clearly communicated to users.

  2. [2899] - The removal of btc deposit fee v1 might affect existing integrations. Consider providing migration guidelines if necessary.

  3. [2826] - The removal of unused code from the emissions module and addition of a new parameter for fixed block reward amount is a significant change. Ensure that this is thoroughly tested and that any dependent systems are updated accordingly.

It's crucial to update all relevant documentation, including API references and integration guides, to reflect these changes.

zetaclient/tss/tss_signer.go (2)

Line range hint 463-470: Refactor BTCAddress to return an error for improved robustness

Currently, the BTCAddress function returns an empty string when an error occurs, which can make error handling less explicit for callers. Consider modifying the function signature to return (string, error) to allow callers to handle errors more effectively and enhance code robustness.


Line range hint 473-480: Refactor BTCAddressWitnessPubkeyHash to return an error for better error propagation

The function presently returns nil upon encountering an error, which might obscure the underlying issue for the caller. By adjusting the function to return (*btcutil.AddressWitnessPubKeyHash, error), callers can handle errors explicitly, leading to clearer and more maintainable code.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3eba701 and 2c65b28.

Files selected for processing (19)
  • changelog.md (1 hunks)
  • cmd/zetaclientd/start.go (1 hunks)
  • pkg/chains/bitcoin.go (3 hunks)
  • pkg/chains/bitcoin_test.go (2 hunks)
  • zetaclient/chains/base/observer.go (1 hunks)
  • zetaclient/chains/base/observer_test.go (1 hunks)
  • zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
  • zetaclient/chains/bitcoin/observer/observer.go (1 hunks)
  • zetaclient/chains/bitcoin/observer/outbound.go (3 hunks)
  • zetaclient/chains/bitcoin/observer/outbound_test.go (2 hunks)
  • zetaclient/chains/bitcoin/observer/rpc_status.go (1 hunks)
  • zetaclient/chains/bitcoin/signer/signer.go (1 hunks)
  • zetaclient/chains/bitcoin/signer/signer_keysign_test.go (2 hunks)
  • zetaclient/chains/bitcoin/signer/signer_test.go (1 hunks)
  • zetaclient/chains/interfaces/interfaces.go (1 hunks)
  • zetaclient/testutils/constant.go (1 hunks)
  • zetaclient/testutils/mocks/tss_signer.go (3 hunks)
  • zetaclient/tss/tss_signer.go (3 hunks)
  • zetaclient/tss/tss_signer_test.go (2 hunks)
Additional context used
Path-based instructions (18)
cmd/zetaclientd/start.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

pkg/chains/bitcoin.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

pkg/chains/bitcoin_test.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/bitcoin/observer/inbound.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/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/interfaces/interfaces.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_signer.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/tss/tss_signer.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/tss/tss_signer_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

Learnings (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-09-19T18:25:57.534Z
Learning: ObserveInbound coverage will be improved in future refactor.
GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound.go

[warning] 149-149: zetaclient/chains/bitcoin/observer/inbound.go#L149
Added line #L149 was not covered by tests

zetaclient/chains/bitcoin/observer/observer.go

[warning] 372-374: zetaclient/chains/bitcoin/observer/observer.go#L372-L374
Added lines #L372 - L374 were not covered by tests


[warning] 376-376: zetaclient/chains/bitcoin/observer/observer.go#L376
Added line #L376 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

Gitleaks
zetaclient/testutils/constant.go

21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

zetaclient/tss/tss_signer_test.go

125-125: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

Additional comments not posted (14)
pkg/chains/bitcoin.go (1)

13-13: Addition of BitcoinSignetParams is appropriate and consistent.

The introduction of BitcoinSignetParams aligns with the existing pattern for other Bitcoin network parameters. This addition effectively supports the Bitcoin Signet network, which is in line with the PR objectives.

pkg/chains/bitcoin_test.go (1)

20-20: Approval: Signet test case added successfully.

The inclusion of the Signet network test case enhances the coverage of the BitcoinNetParamsFromChainID function. This addition aligns with the PR objectives of supporting multiple Bitcoin chains and is consistent with the existing test structure.

zetaclient/tss/tss_signer_test.go (1)

Line range hint 1-190: Commendable test suite with opportunities for enhancement.

The zetaclient/tss/tss_signer_test.go file presents a well-structured and comprehensive test suite for EVM and BTC address generation and validation. The use of table-driven tests facilitates easy extension and maintenance of test cases. To further elevate the quality and robustness of this test suite, consider implementing the following general improvements:

  1. Expand test coverage by adding more edge cases and error scenarios across all test functions.
  2. Implement consistent use of constants for chain IDs, error messages, and other static values to improve maintainability.
  3. Ensure that all test data, including public keys, are non-sensitive and specifically designated for testing purposes.
  4. Consider implementing helper functions for common setup tasks to reduce code duplication across test functions.
  5. Add comments explaining the purpose and expected behavior of each test case, especially for complex scenarios.

These enhancements will contribute to a more robust, maintainable, and comprehensive test suite, further ensuring the reliability of the TSS signer implementation.

Tools
Gitleaks

125-125: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

zetaclient/chains/bitcoin/signer/signer.go (1)

157-157: Approve change with verification

The modification to include the chain ID when deriving the Bitcoin TSS address is appropriate and aligns with the PR objectives. This change enhances the flexibility of the system to support multiple Bitcoin-based chains.

To ensure consistency across the codebase, please run the following verification:

Please review the results to ensure that all calls to BTCAddressWitnessPubkeyHash include the ChainId parameter. If any inconsistencies are found, they should be addressed to maintain uniformity across the codebase.

zetaclient/chains/bitcoin/observer/inbound.go (1)

149-149: Approval: Chain-specific TSS address retrieval implemented.

The modification to include ob.Chain().ChainId as a parameter when retrieving the TSS Bitcoin address is a judicious enhancement. This change aligns seamlessly with the pull request's primary objective of supporting multiple Bitcoin chains, allowing for chain-specific TSS addresses.

To ensure the robustness of this modification, it is imperative to augment the test coverage. Please execute the following command to verify the current test coverage for this file:

Should the results indicate insufficient coverage, kindly consider expanding the test suite to encompass this critical change.

Tools
GitHub Check: codecov/patch

[warning] 149-149: zetaclient/chains/bitcoin/observer/inbound.go#L149
Added line #L149 was not covered by tests

zetaclient/chains/bitcoin/observer/outbound.go (4)

417-417: Improvement in TSS address derivation

The modification to include ob.Chain().ChainId in the BTCAddressWitnessPubkeyHash method call is a commendable enhancement. This change aligns with the PR objectives by enabling the derivation of Bitcoin TSS addresses based on the chain ID, thus facilitating support for multiple Bitcoin chains.


602-602: Consistent improvement in TSS address usage

The addition of ob.Chain().ChainId to the BTCAddress method call is a logical extension of the previous modification. This change maintains consistency in deriving chain-specific TSS addresses throughout the codebase, further supporting the PR's objective of accommodating multiple Bitcoin chains.


661-661: Comprehensive implementation of chain-specific TSS addresses

The inclusion of ob.Chain().ChainId in the BTCAddress method call completes the set of modifications required to support chain-specific TSS addresses. This change, along with the previous two, ensures a consistent and comprehensive implementation across all relevant functions, fully realizing the PR's objective of supporting multiple Bitcoin chains.


Line range hint 417-661: Successful implementation of chain-specific TSS addresses

The modifications in this file effectively implement the PR objective of deriving Bitcoin TSS addresses based on chain ID. The changes are consistently applied across all relevant functions (findNonceMarkUTXO, checkTSSVout, and checkTSSVoutCancelled), ensuring a comprehensive and coherent implementation. This enhancement significantly improves the codebase's ability to support multiple Bitcoin chains, as intended.

The minimal and focused nature of these changes reduces the risk of introducing new issues while achieving the desired functionality. The implementation demonstrates a thorough understanding of the codebase and the requirements of the task at hand.

changelog.md (3)

Line range hint 26-33: Commendable test suite improvements

The additions to the test suite are comprehensive and cover various aspects of the system:

  1. The inclusion of e2e tests for TON blockchain and Bitcoin deposit and call scenarios enhances the robustness of the test suite.
  2. The extension of staking precompile tests and support for multiple runs for precompile tests improves the reliability of the test results.
  3. The addition of chain header tests in E2E tests is crucial for ensuring the integrity of chain-related functionalities.

To further enhance the test suite, consider:

  1. Adding performance benchmarks for critical operations, especially those affected by recent refactors.
  2. Implementing fuzz testing for complex input scenarios, particularly for cross-chain operations.
  3. Ensuring that all new features mentioned in the "Features" section have corresponding test coverage.

Line range hint 56-62: CI improvements and chores enhance development workflow

The CI improvements and chores contribute to a more robust development and release process:

  1. The adjustment to the release pipeline, including manual execution and approval steps, adds an extra layer of control and verification to the release process.
  2. The addition of docker-compose and make commands for launching full nodes simplifies the deployment process for developers and operators.
  3. The update to build and push docker images for both Ubuntu and macOS increases platform coverage.

To further enhance the development workflow, consider:

  1. Implementing automated performance regression tests in the CI pipeline to catch any performance degradations early.
  2. Adding static code analysis tools to the CI process to maintain code quality and catch potential issues before they reach the review stage.
  3. Implementing automated dependency vulnerability scanning to ensure the security of third-party dependencies.

Line range hint 35-54: Verify critical fixes and consider additional security measures

The fixes address a wide range of issues, some of which are critical:

  1. [2674] - The fix for operator voting on discarded keygen ballots is crucial. Ensure that this change doesn't introduce any new vulnerabilities in the voting process.

  2. [2735] - The fix for the outbound tracker blocking confirmation is significant. Verify that this change doesn't negatively impact the system's overall performance or introduce race conditions.

  3. [2853] - The fix for calling precompile through sc with sc state update is important. Ensure that this change is thoroughly tested, especially for edge cases and potential security implications.

  4. [2844] - The addition of tsspubkey to the index for tss keygen voting is a critical change. Verify that this doesn't introduce any potential for key compromise or unauthorized access.

Consider implementing additional logging and monitoring for these critical areas to quickly identify any potential issues in production. Also, it may be beneficial to conduct a security audit focusing on these changes to ensure no new vulnerabilities have been introduced.

cmd/zetaclientd/start.go (2)

271-277: Approved: Filtering and Collecting Bitcoin Chain IDs

The code effectively filters the supported Bitcoin chains and collects their IDs into the btcChainIDs slice. This approach is efficient and aligns with clean code principles.


279-284: Approved: Validating TSS Addresses with Proper Error Handling

The assignment of tss.CurrentPubkey from currentTss.TssPubkey is appropriate. The subsequent validation of TSS addresses against the Bitcoin chain IDs is correctly implemented with proper error handling.

pkg/chains/bitcoin.go Outdated Show resolved Hide resolved
pkg/chains/bitcoin.go Outdated Show resolved Hide resolved
pkg/chains/bitcoin_test.go Outdated Show resolved Hide resolved
zetaclient/chains/bitcoin/signer/signer_keysign_test.go Outdated Show resolved Hide resolved
zetaclient/tss/tss_signer_test.go Outdated Show resolved Hide resolved
zetaclient/chains/base/observer.go Outdated Show resolved Hide resolved
zetaclient/chains/bitcoin/observer/observer.go Outdated Show resolved Hide resolved
zetaclient/chains/base/observer_test.go Outdated Show resolved Hide resolved
zetaclient/chains/bitcoin/observer/outbound_test.go Outdated Show resolved Hide resolved
zetaclient/tss/tss_signer.go Outdated Show resolved Hide resolved
zetaclient/tss/tss_signer.go Outdated Show resolved Hide resolved
zetaclient/tss/tss_signer.go Show resolved Hide resolved
zetaclient/tss/tss_signer.go Outdated Show resolved Hide resolved
zetaclient/chains/base/observer.go Show resolved Hide resolved
zetaclient/chains/base/observer.go Outdated Show resolved Hide resolved
pkg/chains/bitcoin.go Show resolved Hide resolved
zetaclient/chains/interfaces/interfaces.go Outdated Show resolved Hide resolved
@ws4charlie ws4charlie added this pull request to the merge queue Sep 24, 2024
Merged via the queue into develop with commit f6ab039 Sep 24, 2024
31 checks passed
@ws4charlie ws4charlie deleted the feat-multiple-btc-tss-address-and-more-signet-static-info branch September 24, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make UpdateConfigFromCore and ZetaClient config more future-proof
4 participants