zetaclient/chains/bitcoin/signer/sign_test.go (3)

24-46: Consider extracting setup into helper functions.

The test setup could be more maintainable by extracting the TSS and receiver address setup into helper functions.

+func setupTestSigner(t *testing.T) *signer.Signer {
+    return signer.NewSigner(
+        chains.BitcoinMainnet,
+        mocks.NewBTCRPCClient(t),
+        mocks.NewTSS(t).FakePubKey(testutils.TSSPubKeyMainnet),
+        base.DefaultLogger(),
+    )
+}
+
+func setupTestAddresses(t *testing.T, s *signer.Signer) (btcutil.Address, []byte, btcutil.Address, []byte) {
+    tssAddr, err := s.TSS().PubKey().AddressBTC(chains.BitcoinMainnet.ChainId)
+    require.NoError(t, err)
+    tssScript, err := txscript.PayToAddrScript(tssAddr)
+    require.NoError(t, err)
+
+    receiver := "bc1qaxf82vyzy8y80v000e7t64gpten7gawewzu42y"
+    to, err := chains.DecodeBtcAddress(receiver, chains.BitcoinMainnet.ChainId)
+    require.NoError(t, err)
+    toScript, err := txscript.PayToAddrScript(to)
+    require.NoError(t, err)
+
+    return tssAddr, tssScript, to, toScript
+}

---

47-171: Enhance test table structure for better maintainability.

Consider these improvements to the test table:

1. Use constants for repeated values.
2. Group related test cases using subtests.
3. Add test case documentation.

+const (
+    defaultAmount    = 0.2
+    defaultNonceMark = 10000
+    defaultFees     = 2000
+)
+
+// testCase represents a test case for AddWithdrawTxOutputs
+type testCase struct {
+    name      string
+    tx        *wire.MsgTx
+    to        btcutil.Address
+    total     float64
+    amount    float64
+    nonceMark int64
+    fees      int64
+    cancelTx  bool
+    fail      bool
+    message   string
+    txout     []*wire.TxOut
+}
+
+// successCases returns test cases for successful scenarios
+func successCases(to btcutil.Address, tssScript, toScript []byte) []testCase {
+    return []testCase{
+        {
+            name: "should add outputs successfully",
+            // ... existing test case
+        },
+        // ... other success cases
+    }
+}

---

173-188: Enable parallel test execution for better performance.

Consider running the test cases in parallel since they are independent.

 for _, tt := range tests {
     t.Run(tt.name, func(t *testing.T) {
+        t.Parallel()
         err := signer.AddWithdrawTxOutputs(tt.tx, tt.to, tt.total, tt.amount, tt.nonceMark, tt.fees, tt.cancelTx)

pkg/math/integer.go (2)

5-7: Enhance function documentation for better clarity.

While the examples are helpful, the documentation could be more comprehensive. Consider adding:

• Parameter descriptions including valid ranges
• Return value description
• Edge case behavior documentation
• Named variables in examples for better readability

-// IncreaseIntByPercent is a function that increases integer by a percentage.
-// Example1: IncreaseIntByPercent(10, 15, true) = 10 * 1.15 = 12
-// Example2: IncreaseIntByPercent(10, 15, false) = 10 + 10 * 0.15 = 11
+// IncreaseIntByPercent increases an integer value by a specified percentage.
+// Parameters:
+//   - value: The base integer value to increase
+//   - percent: The percentage to increase by (0-MaxUint32)
+//   - round: If true, rounds the increase to nearest integer; if false, truncates
+// Returns:
+//   The increased value after applying the percentage
+// Examples:
+//   baseValue := int64(10)
+//   withRounding := IncreaseIntByPercent(baseValue, 15, true)    // Returns 12 (10 * 1.15 rounded)
+//   withoutRounding := IncreaseIntByPercent(baseValue, 15, false) // Returns 11 (10 + 10 * 0.15 truncated)

---

12-15: Well-implemented optimization for multiples of 100.

Good use of integer arithmetic for better performance when handling multiples of 100. Consider enhancing the comment to explain the performance benefit.

-		// optimization: a simple multiplication
+		// Optimization: Using integer multiplication for multiples of 100
+		// avoids floating-point operations for better performance

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

158-158: Correct the error message for clarity

The error message lacks the word "to," making it grammatically incorrect. Updating it enhances readability and professionalism.

Apply this diff to correct the error message:

-logger.Error().Err(err).Msgf("failed get bitcoin network info")
+logger.Error().Err(err).Msgf("failed to get bitcoin network info")

---

181-181: Use past tense in success log messages

The log message should use "succeeded" instead of "succeed" to accurately reflect the completion of the action.

Apply this diff to correct the log message:

-logger.Info().Str(logs.FieldTx, signedTx.TxID()).Msg("SignRBFTx succeed")
+logger.Info().Str(logs.FieldTx, signedTx.TxID()).Msg("SignRBFTx succeeded")

---

196-196: Use past tense in success log messages

Similarly, update the log message to use "succeeded" for grammatical correctness.

Apply this diff to correct the log message:

-logger.Info().Str(logs.FieldTx, signedTx.TxID()).Msg("SignWithdrawTx succeed")
+logger.Info().Str(logs.FieldTx, signedTx.TxID()).Msg("SignWithdrawTx succeeded")

---

223-223: Fix typo in documentation comment

There is a typo in the comment. Correcting it improves code documentation quality.

Apply this diff to fix the typo:

-// try broacasting tx with increasing backoff (1s, 2s, 4s, 8s, 16s) in case of RPC error
+// try broadcasting tx with increasing backoff (1s, 2s, 4s, 8s, 16s) in case of RPC error

---

143-146: Handle potential errors when retrieving the signer address

Currently, if an error occurs while getting the signer address, it's silently ignored. To prevent unforeseen issues, handle the error explicitly.

Apply this diff to handle the error:

 signerAddress, err := zetacoreClient.GetKeys().GetAddress()
-if err == nil {
+if err != nil {
+    logger.Error().Err(err).Msg("Failed to get signer address")
+} else {
     lf["signer"] = signerAddress.String()
 }

---

225-235: Attempt initial broadcast without delay

To reduce latency, consider attempting to broadcast the transaction immediately before entering the retry loop with backoff.

Apply this diff to modify the broadcasting logic:

 backOff := broadcastBackoff
+// Attempt initial broadcast without delay
+err := signer.Broadcast(tx)
+if err == nil {
+    logger.Info().Fields(lf).Msgf("broadcasted Bitcoin outbound successfully")
+    // Proceed with successful flow
+    // Save transaction, add tracker, etc.
+    // ...
+    return
+}
+logger.Warn().Err(err).Fields(lf).Msg("broadcasting Bitcoin outbound, retrying with backoff")
 // Retry broadcasting with backoff
 for i := 0; i < broadcastRetries; i++ {
     time.Sleep(backOff)

---

225-235: Adjust retry counter for accurate logging

The retry counter i starts at zero, which may be misleading in logs. Start counting retries from one for better clarity.

Apply this diff to adjust the retry counter:

 for i := 0; i < broadcastRetries; i++ {
     time.Sleep(backOff)

     // broadcast tx
     err := signer.Broadcast(tx)
     if err != nil {
-        logger.Warn().Err(err).Fields(lf).Msgf("broadcasting Bitcoin outbound, retry %d", i)
+        logger.Warn().Err(err).Fields(lf).Msgf("broadcasting Bitcoin outbound, retry %d", i+1)
         backOff *= 2
         continue
     }

---

250-253: Log when outbound inclusion fails

Currently, if the outbound transaction isn't included, there's no log indicating the failure. Adding a log enhances observability.

Apply this diff to log the failure:

 _, included := ob.TryIncludeOutbound(ctx, cctx, txHash)
 if included {
     logger.Info().Fields(lf).Msgf("included newly broadcasted Bitcoin outbound")
+} else {
+    logger.Warn().Fields(lf).Msgf("failed to include newly broadcasted Bitcoin outbound")
 }

e2e/e2etests/test_bitcoin_withdraw_rbf.go (1)

20-21: Correct the typo and clarify the comment for better understanding.

The comment contains a typo and is unclear. "Chainging" should be "Changing", and the sentence structure needs adjustment for clarity.

Please apply the following change to improve the comment:

-// Chainging the 'minTxConfirmations' to 1 to not include Bitcoin a pending tx.
+// Changing the 'minTxConfirmations' to 1 to include pending Bitcoin transactions.

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

58-58: Ensure consistent error handling by using errors.Wrapf.

For consistency and better error stack traces, consider using errors.Wrapf instead of fmt.Errorf, as other parts of the code utilize the errors package for wrapping errors.

Update the error handling as follows:

-return nil, fmt.Errorf("invalid fee rate %s", params.GasPriorityFee)
+return nil, errors.Wrapf(err, "invalid fee rate %s", params.GasPriorityFee)

zetaclient/chains/bitcoin/observer/gas_price.go (3)

16-47: Add unit tests for the WatchGasPrice function to improve reliability.

The WatchGasPrice function introduces critical functionality that is currently not covered by tests. Adding unit tests will ensure its correctness and prevent future regressions.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 16-21: zetaclient/chains/bitcoin/observer/gas_price.go#L16-L21
Added lines #L16 - L21 were not covered by tests

---

[warning] 24-36: zetaclient/chains/bitcoin/observer/gas_price.go#L24-L36
Added lines #L24 - L36 were not covered by tests

---

[warning] 38-45: zetaclient/chains/bitcoin/observer/gas_price.go#L38-L45
Added lines #L38 - L45 were not covered by tests

---

51-89: Increase test coverage for the PostGasPrice function.

The PostGasPrice function is essential for accurate gas price reporting but lacks test coverage. Implementing tests will enhance confidence in its operation and handle edge cases effectively.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 51-70: zetaclient/chains/bitcoin/observer/gas_price.go#L51-L70
Added lines #L51 - L70 were not covered by tests

---

[warning] 74-77: zetaclient/chains/bitcoin/observer/gas_price.go#L74-L77
Added lines #L74 - L77 were not covered by tests

---

[warning] 80-87: zetaclient/chains/bitcoin/observer/gas_price.go#L80-L87
Added lines #L80 - L87 were not covered by tests

---

[warning] 89-89: zetaclient/chains/bitcoin/observer/gas_price.go#L89
Added line #L89 was not covered by tests

---

93-106: Handle unsupported network types more gracefully.

In the specialHandleFeeRate function, consider handling additional network types or providing a clearer error message without the leading space. This will improve the function's robustness and clarity.

Apply this diff to adjust the error message:

 return 0, fmt.Errorf(" unsupported bitcoin network type %d", ob.Chain().NetworkType)
+return 0, fmt.Errorf("unsupported bitcoin network type %d", ob.Chain().NetworkType)

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 93-104: zetaclient/chains/bitcoin/observer/gas_price.go#L93-L104
Added lines #L93 - L104 were not covered by tests

x/crosschain/keeper/abci.go (1)

63-64: Improve test coverage for new code paths

The newly added lines at 63-64, where updateFunc is assigned when found, are not covered by unit tests. Enhancing test coverage for this conditional logic is important to ensure that the correct gas price updater functions are selected and executed for different chain types. This will increase code reliability and help prevent future regressions.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 63-64: x/crosschain/keeper/abci.go#L63-L64
Added lines #L63 - L64 were not covered by tests

zetaclient/chains/bitcoin/observer/mempool.go (2)

55-70: Add unit tests for WatchMempoolTxs functionality

The WatchMempoolTxs method introduces critical functionality for monitoring pending outbound transactions in the Bitcoin mempool. It is essential to have unit tests covering this method to ensure its reliability and correctness.

Consider adding unit tests that simulate various scenarios, such as transactions becoming stuck, to verify that the method behaves as expected.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 55-61: zetaclient/chains/bitcoin/observer/mempool.go#L55-L61
Added lines #L55 - L61 were not covered by tests

---

[warning] 64-70: zetaclient/chains/bitcoin/observer/mempool.go#L64-L70
Added lines #L64 - L70 were not covered by tests

---

207-221: Consolidate configuration for PendingTxFeeBumpWaitBlocks

The functions GetStuckTxChecker and GetFeeBumpWaitBlocks handle chain-specific logic based on the chainID. Consider refactoring these configurations into a centralized location or configuration file for easier maintenance and scalability.

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

172-174: Add tests for the WatchMempoolTxs background worker

The addition of the WatchMempoolTxs background worker is crucial for monitoring mempool transactions. It is important to include tests that validate its integration and interaction with other components.

Implement unit tests or integration tests to ensure the new worker operates correctly under various conditions.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 172-174: zetaclient/chains/bitcoin/observer/observer.go#L172-L174
Added lines #L172 - L174 were not covered by tests

---

254-273: Ensure thread-safe updates in SetLastStuckOutbound

In SetLastStuckOutbound, the method updates lastStuckTx while holding a lock. However, the logging statements inside the lock could potentially introduce latency.

Minimize the locked section to only the assignment, and perform logging outside the lock to improve concurrency.

-	ob.Mu().Lock()
-	defer ob.Mu().Unlock()
-	// logging inside lock
-	ob.lastStuckTx = stuckTx
+	ob.Mu().Lock()
+	ob.lastStuckTx = stuckTx
+	ob.Mu().Unlock()
+	// logging outside lock

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

175-177: Refine the upper bound check for fee rate validation

The current check compares *feeResult.FeeRate to maxBTCSupply, which represents the total supply of Bitcoin (21000000.0). Fee rates are typically much smaller values and unlikely to approach this limit. Using maxBTCSupply as an upper bound may not be appropriate and could mask unrealistic high fee rates. Consider defining a more realistic upper limit based on expected maximum fee rates to ensure accurate validation without false negatives.

zetaclient/chains/bitcoin/rpc/rpc_rbf_live_test.go (1)

99-102: Remove commented-out code to improve code clarity

The code between lines 99 and 102 is commented out, which may lead to confusion and reduce code readability. Remove the unused code to maintain a clean codebase.

Apply this diff to remove the commented code:

  // STEP 2
- // build and send tx2 (child of tx1)
- // nonceMark += 1
- // txHash2 := buildAndSendRBFTx(t, client, privKey, txHash1, sender, to, amount, nonceMark, feeRate, bumpFeeReserved)
- // fmt.Printf("sent tx2: %s\n", txHash2)

x/observer/types/crosschain_flags.go (1)

8-10: Consider documenting the rationale for the 10-minute interval.

While the RetryInterval of 10 minutes seems reasonable, it would be beneficial to document why this specific duration was chosen, especially in the context of Bitcoin transaction fee dynamics.

e2e/utils/bitcoin.go (2)

13-32: Enhance error messages for better debugging.

Consider adding more descriptive error messages to the assertions to help identify which specific condition failed:

-    require.Negative(t, txResult.Confirmations)
+    require.Negative(t, txResult.Confirmations, "transaction should have negative confirmations")

-    require.Error(t, err)
-    require.Nil(t, entry)
+    require.Error(t, err, "transaction should not be in mempool")
+    require.Nil(t, entry, "mempool entry should be nil")

-    require.Error(t, err)
-    require.Nil(t, rawTx)
+    require.Error(t, err, "transaction should not exist in blockchain")
+    require.Nil(t, rawTx, "raw transaction should be nil")

---

34-48: Add return value documentation and enhance error messages.

The function would benefit from:

1. Documentation of the return value
2. More descriptive error messages in assertions

-// MustHaveMinedTx ensures the given tx has been mined
+// MustHaveMinedTx ensures the given tx has been mined and returns the detailed transaction information.
+// It panics if the transaction is not mined or cannot be retrieved.
 func MustHaveMinedTx(ctx context.Context, client *rpcclient.Client, txHash *chainhash.Hash) *btcjson.TxRawResult {
     t := TestingFromContext(ctx)
 
     // positive confirmations
     txResult, err := client.GetTransaction(txHash)
-    require.NoError(t, err)
-    require.Positive(t, txResult.Confirmations)
+    require.NoError(t, err, "failed to get transaction")
+    require.Positive(t, txResult.Confirmations, "transaction should have positive confirmations")
 
     // tx exists in blockchain
     rawResult, err := client.GetRawTransactionVerbose(txHash)
-    require.NoError(t, err)
+    require.NoError(t, err, "failed to get raw transaction")

zetaclient/chains/bitcoin/rpc/rpc_test.go (2)

14-59: Extract magic numbers as named constants.

Consider defining constants for commonly used values to improve maintainability:

+const (
+    normalFeeRate     = 0.0001
+    negativeFeeRate   = -0.0001
+    maxSupplyFeeRate  = 21000000
+    expectedNormalRate = 10
+    expectedRegnetRate = 1
+)

 func Test_GetEstimatedFeeRate(t *testing.T) {
     tests := []struct {

---

61-91: Simplify switch statement using if-else.

The switch statement with a single condition could be simplified:

-           switch {
-           case tt.rpcError:
+           if tt.rpcError {
                client.On("EstimateSmartFee", mock.Anything, mock.Anything).Return(nil, errors.New("error"))
-           case tt.resultError:
+           } else if tt.resultError {
                client.On("EstimateSmartFee", mock.Anything, mock.Anything).Return(&btcjson.EstimateSmartFeeResult{
                    Errors: []string{"error"},
                }, nil)
-           default:
+           } else {
                client.On("EstimateSmartFee", mock.Anything, mock.Anything).
                    Maybe().
                    Return(&btcjson.EstimateSmartFeeResult{
                        Errors:  nil,
                        FeeRate: &tt.rate,
                    }, nil)
-           }
+           }

e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (1)

16-22: Improve documentation for the WaitGroup usage.

The documentation should explain why synchronization is needed:

-// wgDeposit is a wait group for deposit runner to finish
+// wgDepositRunner coordinates the completion of Bitcoin deposit runners across E2E tests.
+// It ensures that all deposit-related operations are completed before the test suite ends,
+// preventing potential race conditions or resource leaks.
 var wgDepositRunner sync.WaitGroup

 func init() {
-    // there is one single deposit runner for Bitcoin E2E tests
+    // Initialize with count 1 as there is exactly one deposit runner for Bitcoin E2E tests
     wgDepositRunner.Add(1)
 }

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

42-43: Improve security comment documentation.

The "#nosec G115" comment should include a brief explanation of why this case is safe to ignore.

-  // #nosec G115 always positive
+  // #nosec G115 blockNumber from Bitcoin RPC is guaranteed to be positive

zetaclient/chains/bitcoin/observer/db_test.go (2)

15-35: Add edge cases to SaveBroadcastedTx tests.

Consider adding test cases for:

• Empty transaction hash
• Zero nonce
• Database connection failures

---

90-116: Add negative test case for LoadBroadcastedTxMap.

Consider adding a test case for database read failures to ensure proper error handling.

zetaclient/chains/bitcoin/signer/sign_rbf_test.go (2)

21-28: Add documentation for the test transaction reference.

Consider adding a comment explaining why this specific transaction was chosen as a test case and what aspects it validates.

---

173-228: Consider extracting test setup logic.

The test setup logic, particularly the mock configuration, could be moved to a separate helper function to improve readability.

+func setupTestMocks(s *testSuite, tt *testCase) {
+    // mock cctx rate
+    tt.cctx.GetCurrentOutboundParam().GasPriorityFee = tt.cctxRate
+
+    // mock RPC live fee rate setup
+    setupFeeMocks(s, tt)
+
+    // mock RPC transactions setup
+    setupTransactionMocks(s, tt)
+}
+
 func Test_SignRBFTx(t *testing.T) {
     // ... existing setup code ...
     for _, tt := range tests {
         t.Run(tt.name, func(t *testing.T) {
             s := newTestSuite(t, tt.chain)
-            // mock setup code
+            setupTestMocks(s, &tt)

zetaclient/chains/bitcoin/signer/outbound_data_test.go (2)

95-141: Consider adding more specific error messages for better debugging.

While the error cases are well-covered, the error messages could be more specific to help with debugging.

Apply this diff to enhance error messages:

-			errMsg:       "cctx is nil",
+			errMsg:       "invalid input: cctx is nil",

-			errMsg:   "can only send gas token to a Bitcoin network",
+			errMsg:   "invalid coin type: can only send gas token to a Bitcoin network",

-			errMsg:   "cannot convert gas price",
+			errMsg:   "invalid gas price format: cannot convert to numeric value",

-			errMsg:   "cannot decode receiver address",
+			errMsg:   "invalid receiver address format: cannot decode Bitcoin address",

-			errMsg:   "unsupported receiver address",
+			errMsg:   "unsupported receiver address type: expected P2PKH or P2WPKH",

---

142-194: Consider adding test cases for boundary conditions.

The test cases for restricted CCTX and dust amount are good, but consider adding tests for boundary conditions.

Add test cases for:

1. Amount exactly equal to dust threshold
2. Maximum allowed amount
3. Edge cases for gas prices (0, maximum allowed)

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

167-167: Consider adding documentation for the new method.

The new GetMempoolEntry method is well-placed, but would benefit from documentation explaining its purpose in the context of RBF functionality.

Add documentation above the method:

+	// GetMempoolEntry retrieves detailed information about a transaction in the mempool.
+	// This is used for monitoring transaction status and implementing RBF (Replace-By-Fee) functionality.
	GetMempoolEntry(txHash string) (*btcjson.GetMempoolEntryResult, error)

zetaclient/chains/bitcoin/fee.go (2)

22-32: Consider using iota for sequential constants.

The transaction size-related constants could benefit from using iota for better maintainability and to prevent manual value assignments.

const (
-       bytesPerInput        = 41          // each input is 41 bytes
-       bytesPerOutputP2TR   = 43          // each P2TR output is 43 bytes
-       bytesPerOutputP2WSH  = 43          // each P2WSH output is 43 bytes
-       bytesPerOutputP2WPKH = 31          // each P2WPKH output is 31 bytes
-       bytesPerOutputP2SH   = 32          // each P2SH output is 32 bytes
-       bytesPerOutputP2PKH  = 34          // each P2PKH output is 34 bytes
+       bytesPerInput        = 41
+       bytesPerOutputP2TR   = iota + 43   // 43 bytes
+       bytesPerOutputP2WSH               // 43 bytes
+       bytesPerOutputP2WPKH = iota + 31  // 31 bytes
+       bytesPerOutputP2SH                // 32 bytes
+       bytesPerOutputP2PKH               // 34 bytes

---

49-49: Improve comment clarity.

The comment could be more descriptive about the purpose of this variable in the context of gas limits.

-       // This will be the suggested gas limit used for zetacore
+       // Suggested gas limit (177vB) used by zetacore for withdrawer transactions

e2e/utils/zetacore.go (1)

32-48: Add retry mechanism for resilience.

The function makes a single attempt to query the CCTX data. Consider adding a retry mechanism with backoff for better reliability in e2e tests.

+func withRetry(ctx context.Context, maxAttempts int, fn func() error) error {
+    var err error
+    for i := 0; i < maxAttempts; i++ {
+        if err = fn(); err == nil {
+            return nil
+        }
+        time.Sleep(time.Second * time.Duration(i+1))
+    }
+    return err
+}

 func GetCctxByInboundHash(
     ctx context.Context,
     inboundHash string,
     client crosschaintypes.QueryClient,
 ) *crosschaintypes.CrossChainTx {
     t := TestingFromContext(ctx)
-    // query cctx by inbound hash
-    in := &crosschaintypes.QueryInboundHashToCctxDataRequest{InboundHash: inboundHash}
-    res, err := client.InTxHashToCctxData(ctx, in)
+    var res *crosschaintypes.QueryInboundHashToCctxDataResponse
+    err := withRetry(ctx, 3, func() error {
+        var queryErr error
+        in := &crosschaintypes.QueryInboundHashToCctxDataRequest{InboundHash: inboundHash}
+        res, queryErr = client.InTxHashToCctxData(ctx, in)
+        return queryErr
+    })

zetaclient/chains/bitcoin/signer/fee_bumper_test.go (2)

21-105: Add test cases for edge cases.

The test function could benefit from additional test cases covering edge cases such as zero values and maximum values for fees and rates.

+        {
+            chain:    chains.BitcoinMainnet,
+            name:     "should handle maximum values",
+            client:   mocks.NewBTCRPCClient(t),
+            tx:       btcutil.NewTx(wire.NewMsgTx(wire.TxVersion)),
+            cctxRate: math.MaxInt64,
+            liveRate: float64(math.MaxInt64) / btcutil.SatoshiPerBitcoin,
+            errMsg:   "rate exceeds maximum allowed value",
+        },

---

249-326: Consider adding parallel test execution.

The test cases are independent and could benefit from parallel execution using t.Parallel().

 func Test_FetchFeeBumpInfo(t *testing.T) {
+    t.Parallel()
     liveRate := 0.00012
     mockClient := mocks.NewBTCRPCClient(t)

zetaclient/chains/bitcoin/observer/mempool_test.go (3)

423-443: Add detailed documentation for helper functions.

The helper functions would benefit from comprehensive documentation explaining:

• The purpose and usage of each parameter
• The expected behavior of the returned functions
• Example usage in test cases

Add documentation blocks like this:

// makePendingTxFinder creates a mock implementation of PendingTxFinder for testing.
// Parameters:
//   - tx: The transaction to return, or nil to simulate no pending transaction
//   - nonce: The nonce to return with the transaction
//   - errMsg: If non-empty, the function will return an error with this message
// Returns a function that implements the PendingTxFinder interface.
func makePendingTxFinder(tx *btcutil.Tx, nonce uint64, errMsg string) observer.PendingTxFinder {

---

34-34: Fix typo in function name.

The function name contains a typo: Test_FefreshLastStuckOutbound should be Test_RefreshLastStuckOutbound.

-func Test_FefreshLastStuckOutbound(t *testing.T) {
+func Test_RefreshLastStuckOutbound(t *testing.T) {

---

108-349: Enhance test coverage with additional assertions.

The Test_GetLastPendingOutbound function could benefit from additional assertions to validate:

• The transaction weight and size
• The correctness of input and output scripts
• Error message content for failure cases

Add assertions like this to the test cases:

// For successful cases
require.NotZero(t, lastTx.MsgTx().SerializeSize())
require.Greater(t, len(lastTx.MsgTx().TxIn), 0)
require.Greater(t, len(lastTx.MsgTx().TxOut), 0)

// For error cases
require.ErrorContains(t, err, "expected error message")

zetaclient/chains/bitcoin/rpc/rpc_live_test.go (1)

57-65: Well-structured testnet4 configuration.

The addition of testnet4 support is well-implemented with clear configuration parameters. Consider adding a comment explaining why testnet4 uses testnet3 network name.

Add a comment explaining the network name:

// testnet4 uses testnet3 network name because it shares the same network parameters
// and address format with testnet3
Params: "testnet3",

cmd/zetae2e/local/local.go (1)

320-320: Document reason for commented test.

The RBF test case is commented out without explanation. Add a comment explaining why this test is disabled and any conditions for re-enabling it.

// TODO(RBF-123): Temporarily disabled due to <reason>
// Re-enable when <condition> is met
//e2etests.TestBitcoinWithdrawRBFName,

changelog.md (2)

9-13: Consider removing empty sections.

The sections "Tests", "Refactor", and "Fixes" are currently empty. Consider removing them until there are actual entries to add, as empty sections don't provide value and can make the changelog harder to scan.

-### Tests
-
-## Refactor
-
-### Fixes

---

7-7: Enhance PR reference formatting.

Consider adding a brief description of the RBF feature for better context. The current format only mentions the PR number.

-* [3306](https://github.com/zeta-chain/node/pull/3306) - add support for Bitcoin RBF (Replace-By-Fee)
+* [3306](https://github.com/zeta-chain/node/pull/3306) - Add support for Bitcoin RBF (Replace-By-Fee) to enable stuck transaction replacement using fee bumping

x/crosschain/keeper/abci_test.go (3)

5-5: Consider removing duplicate chain imports.

The file imports both chains and zetachains from the same package. Consider using a single import to maintain clarity.

-import (
    "github.com/zeta-chain/node/pkg/chains"
    zetachains "github.com/zeta-chain/node/pkg/chains"
+import (
    "github.com/zeta-chain/node/pkg/chains"

Also applies to: 14-14

---

461-622: Consider adding edge cases to Bitcoin gas rate tests.

While the test coverage is good, consider adding these edge cases:

1. Maximum gas rate boundary conditions
2. Zero gas rate handling
3. Negative gas rate scenarios
4. Race conditions in gas rate updates

---

624-676: Consider documenting chain support status.

The test clearly defines which chains are supported for gas price updates. Consider adding comments to explain why certain chains are not supported and any future plans for support.

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

170-170: Consider adding address validation checks.

While converting the address to string is correct, consider adding validation to ensure the address format is valid before conversion.

-FromAddress: sample.BtcAddressP2WPKH(t, &chaincfg.MainNetParams).String(),
+addr := sample.BtcAddressP2WPKH(t, &chaincfg.MainNetParams)
+require.NotNil(t, addr)
+FromAddress: addr.String(),

e2e/e2etests/e2etests.go (1)

721-729: Consider enhancing test documentation.

While the test registration is correct, consider adding more detailed documentation about:

1. Expected RBF behavior
2. Fee calculation strategy
3. Success/failure conditions
4. Required network conditions

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: ws4charlie
PR: zeta-chain/node#2987
File: pkg/memo/fields_v0_test.go:270-320
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `FieldsV0` struct of the `memo` package, `RevertAddress` may be left empty when `CallOnRevert` is set to true.