Add test coverage for `ComputeView` by GeorgeTsagk · Pull Request #1919 · lightninglabs/taproot-assets

GeorgeTsagk · 2025-12-18T16:35:09Z

Description

Adds test coverage for ComputeView function of tapchannel package.

~~Depends on lightningnetwork/lnd#10454~~
~~Depends on lightningnetwork/lnd#10434~~

gemini-code-assist · 2025-12-18T16:35:27Z

Summary of Changes

Hello @GeorgeTsagk, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enhancing the robustness of the tapchannel package by introducing extensive unit tests for the ComputeView function. These tests cover a wide range of HTLC (Hashed Timelock Contract) processing scenarios, ensuring accurate balance calculations and update filtering. Concurrently, minor adjustments were made to the HTLC processing logic to correctly account for specific height conditions, and project dependencies were updated to reflect ongoing development in related lnd components.

Highlights

New Test Coverage: A comprehensive suite of unit tests has been added for the ComputeView function in the tapchannel package, significantly improving its reliability and ensuring correct behavior across various scenarios.
HTLC Processing Logic Refinement: The logic within processRemoveEntry and processAddEntry in tapchannel/commitment.go has been updated to correctly handle HTLC entries where the removeHeight or addHeight is zero, ensuring these are processed as intended.
Dependency Updates: The go.mod and go.sum files have been updated to include a newer version of github.com/btcsuite/btcd/btcec/v2 and temporary replace directives pointing to a specific lnd fork by the author, aligning with external dependencies.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request adds comprehensive test coverage for the ComputeView function in the tapchannel package, which is a great addition for ensuring the correctness of the commitment logic. The new tests are well-structured and cover a wide range of scenarios.

The PR also includes a small but important fix in processRemoveEntry and processAddEntry to correctly handle HTLC entries that are being added to the current commitment state.

I have one suggestion for the new test file to improve maintainability by replacing magic numbers with named constants and to prevent a potential issue with duplicate HTLC indexes in test data.

gemini-code-assist · 2025-12-18T16:48:56Z

tapchannel/commitment_test.go

+		desc := lnwallet.NewTestAuxHtlcDescriptor(
+			lnwire.ChannelID{},
+			lnwallet.PaymentHash{byte(htlc.htlcIndex)},
+			100,
+			lnwire.MilliSatoshi(10000000),
+			htlc.htlcIndex,
+			htlc.parentIndex,
+			htlc.entryType,
+			makeAssetRecords(htlc.amount),
+			htlc.addHeightLocal,
+			htlc.addHeightRemote,
+			htlc.removeHeightLocal,
+			htlc.removeHeightRemote,
+		)
+		localDescriptors = append(localDescriptors, desc)
+	}
+
+	// Create remote HTLCs
+	for _, htlc := range cfg.remoteHtlcs {
+		desc := lnwallet.NewTestAuxHtlcDescriptor(
+			lnwire.ChannelID{},
+			lnwallet.PaymentHash{byte(htlc.htlcIndex)},
+			100,
+			lnwire.MilliSatoshi(10000000),
+			htlc.htlcIndex,
+			htlc.parentIndex,
+			htlc.entryType,
+			makeAssetRecords(htlc.amount),
+			htlc.addHeightLocal,
+			htlc.addHeightRemote,
+			htlc.removeHeightLocal,
+			htlc.removeHeightRemote,
+		)
+		remoteDescriptors = append(remoteDescriptors, desc)
+	}
+
+	// Add non-asset HTLC if requested
+	if cfg.localNonAsset {
+		localDescriptors = append(
+			localDescriptors,
+			lnwallet.AuxHtlcDescriptor{
+				ChanID:        lnwire.ChannelID{},
+				RHash:         lnwallet.PaymentHash{},
+				Timeout:       100,
+				Amount:        lnwire.MilliSatoshi(10000000),
+				HtlcIndex:     999,
+				ParentIndex:   0,
+				CustomRecords: nil, // No assets
+			},
+		)
+	}
+	if cfg.remoteNonAsset {
+		remoteDescriptors = append(
+			remoteDescriptors,
+			lnwallet.AuxHtlcDescriptor{
+				ChanID:        lnwire.ChannelID{},
+				RHash:         lnwallet.PaymentHash{},
+				Timeout:       100,
+				Amount:        lnwire.MilliSatoshi(10000000),
+				HtlcIndex:     999,
+				ParentIndex:   0,
+				CustomRecords: nil, // No assets
+			},
+		)
+	}


The createView function uses several magic numbers for creating test HTLC descriptors (e.g., 100 for timeout, 10000000 for amount, 999 for HTLC index). It's a good practice to define these as constants to improve readability and maintainability.

Additionally, both local and remote non-asset HTLCs are created with the same HtlcIndex of 999. This could cause conflicts in tests where both are used. Using distinct constants for these indexes will prevent this potential issue.

I suggest defining the following constants at the file level:

const ( testBtcTimeout = 100 testBtcAmount = lnwire.MilliSatoshi(10000000) localNonAssetHtlcIndex = 999 remoteNonAssetHtlcIndex = 998 )

And then using them within createView for both asset and non-asset HTLCs.

desc := lnwallet.NewTestAuxHtlcDescriptor( lnwire.ChannelID{}, lnwallet.PaymentHash{byte(htlc.htlcIndex)}, testBtcTimeout, testBtcAmount, htlc.htlcIndex, htlc.parentIndex, htlc.entryType, makeAssetRecords(htlc.amount), htlc.addHeightLocal, htlc.addHeightRemote, htlc.removeHeightLocal, htlc.removeHeightRemote, ) localDescriptors = append(localDescriptors, desc) } // Create remote HTLCs for _, htlc := range cfg.remoteHtlcs { desc := lnwallet.NewTestAuxHtlcDescriptor( lnwire.ChannelID{}, lnwallet.PaymentHash{byte(htlc.htlcIndex)}, testBtcTimeout, testBtcAmount, htlc.htlcIndex, htlc.parentIndex, htlc.entryType, makeAssetRecords(htlc.amount), htlc.addHeightLocal, htlc.addHeightRemote, htlc.removeHeightLocal, htlc.removeHeightRemote, ) remoteDescriptors = append(remoteDescriptors, desc) } // Add non-asset HTLC if requested if cfg.localNonAsset { localDescriptors = append( localDescriptors, lnwallet.AuxHtlcDescriptor{ ChanID: lnwire.ChannelID{}, RHash: lnwallet.PaymentHash{}, Timeout: testBtcTimeout, Amount: testBtcAmount, HtlcIndex: localNonAssetHtlcIndex, ParentIndex: 0, CustomRecords: nil, // No assets }, ) } if cfg.remoteNonAsset { remoteDescriptors = append( remoteDescriptors, lnwallet.AuxHtlcDescriptor{ ChanID: lnwire.ChannelID{}, RHash: lnwallet.PaymentHash{}, Timeout: testBtcTimeout, Amount: testBtcAmount, HtlcIndex: remoteNonAssetHtlcIndex, ParentIndex: 0, CustomRecords: nil, // No assets }, ) }

coveralls · 2025-12-18T17:01:25Z

Pull Request Test Coverage Report for Build 23287921871

Details

0 of 0 changed or added relevant lines in 0 files are covered.
40 unchanged lines in 11 files lost coverage.
Overall coverage increased (+0.2%) to 33.664%

Files with Coverage Reduction	New Missed Lines	%
address/address.go	2	67.54%
asset/asset.go	2	46.74%
tappsbt/create.go	2	22.71%
asset/mock.go	3	65.89%
tapchannel/aux_leaf_signer.go	3	43.18%
tapgarden/planter.go	3	63.45%
commitment/tap.go	4	72.21%
rfqmsg/records.go	4	65.69%
tapdb/universe.go	4	76.99%
tapdb/multiverse.go	6	54.38%

Totals
Change from base Build 23242626536:	0.2%
Covered Lines:	33326
Relevant Lines:	98996

💛 - Coveralls

lightninglabs-deploy · 2026-03-05T23:04:37Z

@GeorgeTsagk, remember to re-request review from reviewers when ready

GeorgeTsagk · 2026-03-11T12:52:51Z

All dependency PRs have been merged. This is now ready for review.

jtobin

Nothing but nits, LGTM. 👍 👍

An agent flagged that whoseCommit=Local coverage is limited, in that most complex multi-HTLC/settle/fail scenarios use whoseCommit=Remote. Sounds reasonable, but again nittish, as IMO the changes are a strict improvement.

jtobin · 2026-03-12T12:47:31Z

tapchannel/commitment_test.go

+	}
+
+	for _, tc := range testCases {
+		tc := tc


Nit: apparently this is unnecessary in Go 1.22+.

jtobin · 2026-03-12T12:48:36Z

tapchannel/commitment_test.go

+			expectedTheirUpdates: 0,
+		},
+		{
+			name:         "1 add, 1 settle",


Nit: duplicate name, maybe worth differentiating explicitly.

jtobin · 2026-03-12T12:48:44Z

tapchannel/commitment_test.go

+			expectedTheirUpdates: 1,
+		},
+		{
+			name:         "1 add, 1 settle",


Other side of nit.

We add a simple framework to test the function ComputeView which produces the asset balance state of a channel by processing the htlc view at the current height. Now that "NewTestAuxHtlcDescriptor" is available on LND we can craft various test cases where the HTLC entries have different add/remove heights.

GeorgeTsagk self-assigned this Dec 18, 2025

github-project-automation bot added this to Taproot-Assets Project Board Dec 18, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board Dec 18, 2025

gemini-code-assist bot reviewed Dec 18, 2025

View reviewed changes

GeorgeTsagk force-pushed the channel-commitment-test branch 2 times, most recently from c5718c3 to 163a77b Compare December 19, 2025 10:15

GeorgeTsagk added the no-changelog label Jan 8, 2026

GeorgeTsagk force-pushed the channel-commitment-test branch from 163a77b to 7855926 Compare March 11, 2026 12:47

GeorgeTsagk requested review from darioAnongba and jtobin March 11, 2026 12:52

darioAnongba approved these changes Mar 11, 2026

View reviewed changes

jtobin approved these changes Mar 12, 2026

View reviewed changes

github-project-automation bot moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board Mar 12, 2026

GeorgeTsagk force-pushed the channel-commitment-test branch from 7855926 to 710bff7 Compare March 16, 2026 10:49

GeorgeTsagk enabled auto-merge March 16, 2026 11:51

GeorgeTsagk disabled auto-merge March 16, 2026 14:30

GeorgeTsagk enabled auto-merge March 16, 2026 14:30

GeorgeTsagk disabled auto-merge March 19, 2026 09:11

GeorgeTsagk force-pushed the channel-commitment-test branch from 710bff7 to 9b3bedc Compare March 19, 2026 09:16

GeorgeTsagk enabled auto-merge March 19, 2026 10:30

GeorgeTsagk added this pull request to the merge queue Mar 19, 2026

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 19, 2026

darioAnongba added this pull request to the merge queue Mar 19, 2026

Merged via the queue into main with commit ece0740 Mar 19, 2026
79 of 80 checks passed

github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Mar 19, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test coverage for `ComputeView`#1919

Add test coverage for `ComputeView`#1919
darioAnongba merged 1 commit intomainfrom
channel-commitment-test

GeorgeTsagk commented Dec 18, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot commented Dec 18, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Dec 18, 2025

Uh oh!

coveralls commented Dec 18, 2025 •

edited

Loading

Uh oh!

lightninglabs-deploy commented Mar 5, 2026

Uh oh!

GeorgeTsagk commented Mar 11, 2026

Uh oh!

jtobin left a comment

Uh oh!

jtobin Mar 12, 2026

Uh oh!

jtobin Mar 12, 2026

Uh oh!

jtobin Mar 12, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

GeorgeTsagk commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

gemini-code-assist bot commented Dec 18, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 23287921871

Details

💛 - Coveralls

Uh oh!

lightninglabs-deploy commented Mar 5, 2026

Uh oh!

GeorgeTsagk commented Mar 11, 2026

Uh oh!

jtobin left a comment

Choose a reason for hiding this comment

Uh oh!

jtobin Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jtobin Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jtobin Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GeorgeTsagk commented Dec 18, 2025 •

edited

Loading

coveralls commented Dec 18, 2025 •

edited

Loading