internal/test/test.go (2)

19-19: Consider using a more descriptive import alias.

The liquid_wallet alias is functional but could be more concise. Consider using liquidwallet or lwallet for better readability.

-	liquid_wallet "github.com/BoltzExchange/boltz-client/v2/internal/onchain/liquid-wallet"
+	liquidwallet "github.com/BoltzExchange/boltz-client/v2/internal/onchain/liquid-wallet"

---

77-108: Improve error handling and timing in wallet initialization.

The wallet initialization logic is solid, but there are a few areas for improvement:

1. The fixed 3-second sleep after mining may not be reliable across different environments
2. Consider adding cleanup for the wallet if funding fails

-		time.Sleep(3 * time.Second)
+		// Wait for wallet to sync with better timeout handling
+		for retries := 0; retries < 10; retries++ {
+			time.Sleep(500 * time.Millisecond)
+			balance, err := wallet.GetBalance()
+			if err == nil && balance.Confirmed > 0 {
+				break
+			}
+		}

Makefile (1)

64-70: Improve the regtest setup condition.

The change from checking regtest/start.sh to regtest/docker-compose.override.yml is reasonable, but the condition could be more robust.

Consider checking for multiple critical files to ensure complete regtest setup:

-ifeq ("$(wildcard regtest/docker-compose.override.yml)","")
+ifeq ("$(wildcard regtest/docker-compose.override.yml regtest/start.sh)","")

internal/onchain/liquid-wallet/wallet_test.go (2)

73-77: Address the TODO comment and improve transaction verification.

The commented-out output verification should be addressed for complete test coverage.

The TODO indicates an issue with output verification. This is likely related to the transaction output structure. Would you like me to help implement the correct output verification logic?

-				// TODO: fix this
-				//require.True(t, slices.ContainsFunc(tx.Outputs, func(o onchain.TransactionOutput) bool {
-				//return o.Address == address
-				//}))
+				// Verify transaction contains the expected output
+				found := false
+				for _, output := range tx.Outputs {
+					if output.Address == address && output.Amount == uint64(amount) {
+						found = true
+						break
+					}
+				}
+				require.True(t, found, "Expected output not found in transaction")

---

101-136: Well-designed auto-consolidation test with minor timing concerns.

The test correctly verifies the auto-consolidation feature by creating multiple UTXOs and checking for the consolidation transaction. However, the timing assumptions could be more robust.

Consider making the timing more flexible:

-	}, 30*syncInterval, syncInterval)
+	}, 60*syncInterval, syncInterval/2)

This provides more time for async operations while checking more frequently.

internal/onchain/liquid-wallet/wallet.go (5)

1-1: Consider renaming the package to follow Go conventions.

Go package names should be lowercase without underscores. Consider renaming to liquidwallet.

-package liquid_wallet
+package liquidwallet

---

146-148: Consider making initial sync failure non-fatal.

If the initial FullScan fails, the entire wallet creation fails. Consider allowing wallet creation to succeed and retrying the sync in the background loop.

-	if err := result.FullScan(); err != nil {
-		return nil, err
-	}
+	if err := result.FullScan(); err != nil {
+		logger.Errorf("Initial full scan failed: %v. Will retry in background.", err)
+	}

---

157-170: Consider adding exponential backoff for repeated sync failures.

While the jitter helps distribute load, repeated failures should use exponential backoff to avoid overwhelming the Esplora server.

Would you like me to implement an exponential backoff mechanism for the sync loop?

---

242-243: Track and implement the unconfirmed balance TODO.

The comment indicates unconfirmed balance should be tracked separately from confirmed balance.

Would you like me to open an issue to track the implementation of unconfirmed balance handling?

---

264-265: Track the pagination TODO for transaction listing.

The comment indicates pagination needs to be implemented in the lwk library.

Would you like me to check if the lwk library has pagination support or open an issue to track this feature?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

internal/onchain/liquid-wallet/wallet_test.go (5)

18-22: Consider adding documentation for test constants.

The constants lack documentation explaining their purpose and why these specific values were chosen.

+// Global backend instance shared across tests
 var backend *liquid_wallet.BlockchainBackend

+// syncInterval defines how frequently the wallet syncs with the blockchain during tests
 const syncInterval = 1 * time.Second
+// consolidationThreshold is the number of UTXOs that triggers auto-consolidation
 const consolidationThreshold = 3

---

35-92: Good test structure with room for improvement.

The test function properly creates isolated wallet instances and uses subtests effectively. However, there are some issues to address.

1. Address the TODO comment (lines 66-69): The commented output validation should be implemented or removed.
2. Consider extracting magic numbers: The amount 10000 and fee rate 1 could be constants.
3. Improve assertion clarity (line 71): The fee calculation logic could be more explicit.

+const (
+	testSendAmount = int64(10000)
+	testFeeRate = uint64(1)
+)

 func TestWallet_Funded(t *testing.T) {
 	fundedWallet, err := test.InitTestWalletLiquid(backend)
 	require.NoError(t, err)
 	
 	// ... existing GetBalance test ...
 	
 	t.Run("SendToAddress", func(t *testing.T) {
 		address := test.GetNewAddress(test.LiquidCli)
-		amount := int64(10000)
+		amount := testSendAmount
 		txId, err := fundedWallet.SendToAddress(onchain.WalletSendArgs{
 			Address:     address,
 			Amount:      uint64(amount),
-			SatPerVbyte: 1,
+			SatPerVbyte: testFeeRate,
 			SendAll:     false,
 		})

---

66-72: Address the TODO comment or remove it.

The commented validation code should either be implemented or removed if it's no longer needed.

Can you clarify whether the output validation is still needed? If so, I can help implement the fix for the transaction output validation.

---

119-145: Improve auto-consolidation test reliability.

The test logic is sound but could be more robust and clearer.

 func TestWallet_AutoConsolidate(t *testing.T) {
 	wallet := newWallet(t)
 	numTxns := consolidationThreshold
 	amount := uint64(500)
+	
+	// Send multiple small transactions to trigger consolidation
 	for i := 0; i < numTxns; i++ {
 		address, err := wallet.NewAddress()
 		require.NoError(t, err)
 		require.NotEmpty(t, address)
 		test.SendToAddress(test.LiquidCli, address, amount)
-		time.Sleep(100 * time.Millisecond)
+		time.Sleep(100 * time.Millisecond) // Prevent transaction conflicts
 	}
 	test.MineBlock()
 
 	require.Eventually(t, func() bool {
 		txes, err := wallet.GetTransactions(0, 0)
 		require.NoError(t, err)
+		
+		// Expect numTxns incoming transactions + 1 consolidation transaction
 		if len(txes) == numTxns+1 {
 			balance, err := wallet.GetBalance()
 			require.NoError(t, err)
 			total := uint64(numTxns) * amount
-			change := uint64(txes[0].BalanceChange)
-			require.Equal(t, balance.Confirmed, total+change)
+			
+			// The consolidation transaction should have a negative balance change (fee)
+			consolidationTx := txes[0] // Assuming newest transaction is first
+			require.Negative(t, consolidationTx.BalanceChange, "Consolidation should have negative balance change due to fees")
+			
+			// Total balance should be original amount minus consolidation fees
+			expectedBalance := total + uint64(consolidationTx.BalanceChange)
+			require.Equal(t, expectedBalance, balance.Confirmed)
 			return true
 		}
 		return false
 	}, 30*syncInterval, syncInterval)
 }

---

128-128: Document the purpose of the sleep duration.

The 100ms sleep could benefit from a comment explaining why this specific duration is needed.

-		time.Sleep(100 * time.Millisecond)
+		time.Sleep(100 * time.Millisecond) // Small delay to prevent mempool conflicts

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro