Add test coverage for ComputeView #1919
Conversation
Summary of ChangesHello @GeorgeTsagk, I'm Gemini Code Assist1! 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 Highlights
🧠 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 AssistThe 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
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 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| 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 | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
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 }, ) }
Pull Request Test Coverage Report for Build 20347304176Details
💛 - Coveralls |
GeorgeTsagk force-pushed the channel-commitment-test branch from b0a9047 to c5718c3 Compare 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 force-pushed the channel-commitment-test branch from c5718c3 to 163a77b Compare 
Description
Adds test coverage for
ComputeViewfunction of tapchannel package.Depends on lightningnetwork/lnd#10454Depends on lightningnetwork/lnd#10434