Merge tapd side-branch to master
#1155
Conversation
We can now use the LND_REPO, TAPROOT_ASSETS_REPO, TAPRPC_REPO, and LOOP_RPC build arguments to force a specific repo to be used so that commits referenced by LND_VERSION, TAPROOT_ASSETS_VERSION, TAPRPC_VERSION, and LOOP_VERSION don't have to exist in the default repository. If any of these build arguments are not defined, the build continues using the default repository for that module. NOTE: If these arguments ARE defined then the corresponding `_VERSION` argument MUST also be defined, otherwise the build continues using the default repository defined for that module.
This commit mainly changes the returned type of the payment result for asset payments. We create a struct TapPaymentResult which contains all the related information for the outcome of an asset payment.
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 integrates a significant number of updates from the Highlights
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 merges a long-running feature branch for tapd into the master branch. The changes are extensive, including significant dependency updates, new features reflected in protobuf definitions and generated clients, and substantial improvements to the test harness and integration tests. The new tests cover critical scenarios like multi-RFQ payments and channel version upgrades, which is great to see. My review focused on the new code and test utilities, and I've identified a couple of medium-severity issues in the test helpers that could be improved for better robustness and clarity.
| // Ignore the new RFQ array message from the stream, it is also | ||
| // not relevant. | ||
| quotes := msg.GetAcceptedSellOrders() | ||
| if quotes != nil { | ||
| for _, quote := range quotes.AcceptedSellOrders { | ||
| rpcRate := quote.BidAssetRate | ||
| rate, err := rpcutils.UnmarshalRfqFixedPoint( | ||
| rpcRate, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| rateVal = *rate | ||
|
|
||
| t.Logf("Got quote for %v asset units per BTC "+ | ||
| "from peer %v", rate, quote.Peer) | ||
| } | ||
|
|
||
| continue | ||
| } |
There was a problem hiding this comment.
The logic for handling multiple RFQ quotes in getAssetPaymentResult appears to be incorrect, and the preceding comment is misleading.
- The comment on line 493 states that the RFQ array message is ignored, but the code below it processes the message. This comment should be removed or updated to reflect the actual behavior.
- The loop starting on line 497 iterates through
quotes.AcceptedSellOrdersand repeatedly overwritesrateVal. If a payment is split across multiple peers with different rates, this will result inrateValholding only the rate of the last quote. This is incorrect for calculating the total asset amount or an effective rate.
While this may not cause test failures if all peers in current tests have the same rate, it makes the helper function brittle and incorrect for scenarios with varying rates. The logic should be revised to correctly handle multiple quotes, for example by calculating an effective rate or a total asset amount.
| binary string, args []LitArgOption) (string, []LitArgOption) { | ||
|
|
||
| if backwardCompat == nil { | ||
| noopArg := WithLitArg("taproot-assets.channel.noop-htlcs", "") |
There was a problem hiding this comment.
Referring back to @Roasbeef comment here: 9b7c172#r2422820720
iiuc he suggests to turn noop-htlcs on by default. Not specifically in the context of itests, but just for the tapd client in general. The flag would then be used to turn noop-htlcs off. (Rename the flag to no-noop-htlcs or something)
For the itest it would mean you wouldn't have to pass the argument here, but you would have to pass it for the cases you want to have it specifically turned off.
There was a problem hiding this comment.
I think we can make the switch to being by-default on, without changing the semantics of the config itself. We can just let it be "true" if not defined in our default config in tapd. This way the above line leads to no harm (won't need to update it too).
Since we bumped the version of loop and lnd in the previous commit we also caused the proto files to change. We need to regenerate the proto files with `make protos` and commit them.
Changes the itest asset used in the decode payreq itest to instead be a new variable declared within the scope of the itest. This prevents mutating the global asset that is re-used by other test cases and would lead to failures otherwise. Thanks @jtobin for spotting. Co-authored-by: Jared Tobin <jared@jtobin.io>
Since we're now always using no-op HTLCs over all taproot assets channels that support it, that means that certain liquidity related assumptions do not hold anymore. In some test cases where we assume that a satoshi balance was eventually accumulated we instead manually slosh that balance as sats don't really shift in the channel balance over the long term.
GeorgeTsagk
force-pushed
the
tapd-main-branch
branch
from
ec3473a to
d9b6f4c
Compare
| github.com/lightninglabs/loop/looprpc v1.0.8 | ||
| github.com/lightninglabs/loop/swapserverrpc v1.0.15 | ||
| github.com/lightninglabs/lndclient v0.20.0-1 | ||
| github.com/lightninglabs/loop v0.31.3-beta.0.20251014011913-03de0a8ed734 |
There was a problem hiding this comment.
This bump is required in order to be compatible with lnd & tapd right? Because with this bump, we'll need to wait for the next loop release (which should be imminent) before we can do a litd release.
There was a problem hiding this comment.
Yes, loop had to be bumped in order to reflect some of our latest changes in the tapd codebase (which it depends on).
| github.com/lightninglabs/taproot-assets v0.6.1 | ||
| github.com/lightninglabs/taproot-assets/taprpc v1.0.8-0.20250716163904-2ef55ba74036 | ||
| github.com/lightningnetwork/lnd v0.19.3-beta | ||
| github.com/lightninglabs/taproot-assets v0.7.0-rc1.0.20251014100421-92189161aa7a |
There was a problem hiding this comment.
Also, with this, we'll need to wait for tapd v0.7.0-rc2 before we can do a release as well.
There was a problem hiding this comment.
Yes, when all deps are properly tagged we'll have to make one final version bumping PR.
4 participants
Description
For the last couple of months we have been isolating tapd specific PRs on the
tapd-main-branchside branch. This has been continuously rebasing on top ofmasterand any conflicts have been gradually resolved.The PRs that contributed to
tapd-main-branchare:Some single-commits that are not contained in any PR were direct pushes to the
tapd-main-branch, which was not protected. These commits do routine/maintenance stuff like bumping versions or changing config values, so I hope they're easy to go through.Purpose of this PR
This is definitely not a call to go through all the diff and verify the content itself, but more like an ACK around version bumps or any changes in the test harness itself. The content of the itests has been thoroughly reviewed and context should be available in the PRs linked above.