Skip to content

looprpc: populate static address swap timing and cost fields#1150

Merged
hieblmi merged 4 commits into
lightninglabs:masterfrom
hieblmi:show-static-details
Jun 3, 2026
Merged

looprpc: populate static address swap timing and cost fields#1150
hieblmi merged 4 commits into
lightninglabs:masterfrom
hieblmi:show-static-details

Conversation

@hieblmi
Copy link
Copy Markdown
Collaborator

@hieblmi hieblmi commented May 29, 2026

  • Added initiation_time, last_update_time, cost_server, cost_onchain, and cost_offchain to StaticAddressLoopInSwap.
  • Populated static loop-in list responses with stored initiation/update timestamps.
  • Reported the persisted quoted swap fee as cost_server once the static loop-in invoice has been paid.
  • Left on-chain/off-chain costs at zero because static timeout-path miner fees are not currently persisted.
  • Added focused test coverage for static loop-in cost reporting behavior.

@hieblmi hieblmi requested a review from starius May 29, 2026 12:04
@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, 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 enhances the static address loop-in functionality by exposing timing and cost information through the RPC interface. By updating the protobuf definitions and implementing logic to calculate and report these metrics, users can now gain better visibility into the lifecycle and financial performance of their static address swaps.

Highlights

  • API Schema Updates: Extended the StaticAddressLoopInSwap RPC message to include initiation_time, last_update_time, cost_server, cost_onchain, and cost_offchain fields.
  • Cost Reporting Logic: Implemented staticAddressLoopInSwapCost to populate swap costs, ensuring server costs are reported once the invoice is paid, while keeping on-chain and off-chain costs at zero as they are not currently persisted.
  • Testing: Added comprehensive test coverage in TestStaticAddressLoopInSwapCost to verify the correct reporting of swap costs across different swap states.
New Features

🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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.

Footnotes

  1. 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.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds initiation time, last update time, and cost fields (server, on-chain, off-chain) to the StaticAddressLoopInSwap protobuf message and populates them in the server implementation. It also introduces a helper function to calculate these costs and adds corresponding unit tests. The review feedback correctly identifies a potential issue where calling UnixNano() on uninitialized time.Time fields can lead to unexpected negative timestamps, and suggests adding safety checks for zero-time values and nil pointers.

Comment thread loopd/swapclient_server.go Outdated
@hieblmi hieblmi force-pushed the show-static-details branch 3 times, most recently from 7aae14b to 1e5911c Compare May 29, 2026 14:52
@hieblmi hieblmi self-assigned this Jun 2, 2026
@hieblmi
Copy link
Copy Markdown
Collaborator Author

hieblmi commented Jun 3, 2026

@claude review this

@claude
Copy link
Copy Markdown

claude Bot commented Jun 3, 2026

Code Review: looprpc: populate static address swap timing and cost fields

Overview

This PR adds five new fields to StaticAddressLoopInSwap — initiation/update timestamps and three cost fields — bringing the static loop-in RPC response closer to parity with standard loop swaps. The implementation is clean and the scope is appropriately narrow.


Findings

Minor issues

1. Nil guard may silently hide a bug (loopd/swapclient_server.go)

if swp == nil {
    continue
}

GetAllSwaps returns []*StaticAddressLoopIn from the DB store, which should never produce nil entries. Silently skipping a nil pointer means a broken store or upstream bug would go unnoticed in production. If this guard is truly necessary, a warning log would make the silent skip observable:

if swp == nil {
    log.Warnf("ListStaticAddressSwaps: nil swap in results, skipping")
    continue
}

If nil entries are impossible by contract (likely), the guard can be removed entirely.


2. Proto docs for timestamps don't mention units

client.proto adds:

int64 initiation_time = 7;
int64 last_update_time = 8;

These are Unix nanoseconds (via UnixNano()), but that's not stated. The existing SwapStatus message omits units too, so this is consistent with codebase style, but it remains a footgun for API consumers. Worth at minimum a comment like "Unix timestamp in nanoseconds." — especially since the new fields live in a different message type and readers won't naturally cross-reference SwapStatus.


3. cost_onchain / cost_offchain are always zero — proto doc should note this explicitly

The function comment in staticAddressLoopInSwapServerCost explains the situation clearly, and the PR description acknowledges it. However, the proto field comments only say:

// On-chain transaction cost.
int64 cost_onchain = 10;

// Off-chain routing fees.
int64 cost_offchain = 11;

A reader calling ListStaticAddressSwaps via gRPC will see these fields silently return 0 with no indication that this is a known limitation. Consider a short note such as "Currently always zero; timeout-path miner fees are not yet persisted." This is particularly useful for external tooling built against the REST/Swagger endpoint.


What looks good

  • staticAddressLoopInTimestamp correctly guards against zero-value time.Time before calling UnixNano(), preventing a large negative timestamp from reaching clients. The zero-returns-zero contract is clean and well-tested.

  • staticAddressLoopInSwapServerCost state coverage is correct: PaymentReceived, Succeeded, and SucceededTransitioningFailed are exactly the states where the server has settled the invoice and the fee is real, not an estimate.

  • Test coverage for both new helpers is focused and covers the meaningful boundary cases (pending, each success variant, timeout, failed). The table-driven approach in TestStaticAddressLoopInSwapServerCost makes it easy to add states later.

  • Proto field numbers (7–11) are correctly reserved for future use, and the generated client.pb.go is consistent with the proto source.

  • The QuotedSwapFeeCostServer mapping is semantically correct: for static loop-ins the invoice amount equals depositAmount − QuotedSwapFee, so the quoted fee IS the actual server cost once the payment settles.


Summary

The implementation is correct and safe. The three points above are all minor: the nil guard is cosmetic, and the two proto documentation gaps are quality-of-life issues for external consumers. Nothing here blocks merging, but items 2 and 3 would improve the API's self-documenting quality.

@hieblmi
Copy link
Copy Markdown
Collaborator Author

hieblmi commented Jun 3, 2026

Addressed Claude's minor feedback in 6e733f98:

  • Changed the nil static loop-in swap handling from a silent skip to an RPC error, so an unexpected nil result from the store is observable.
  • Documented initiation_time and last_update_time as Unix timestamps in nanoseconds.
  • Documented that static loop-in cost_onchain and cost_offchain currently remain zero because those fees are not persisted.

@hieblmi hieblmi force-pushed the show-static-details branch 4 times, most recently from ecf01e2 to 8fe21dc Compare June 3, 2026 16:07
@hieblmi hieblmi force-pushed the show-static-details branch from 8fe21dc to c6b4362 Compare June 3, 2026 16:41
@starius
Copy link
Copy Markdown
Collaborator

starius commented Jun 3, 2026

loop static listswaps -h says:

Shows a list of finalized static address swaps.

But it also includes pending swaps. I fixed it in one commit.

Also I added a test for RPC in another commit.

Copy link
Copy Markdown
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 💾
I found two issues and fixed them in separate commits.

@hieblmi hieblmi merged commit f879c52 into lightninglabs:master Jun 3, 2026
10 checks passed
@hieblmi hieblmi deleted the show-static-details branch June 3, 2026 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants