Inject clocks into swap lifecycle and update swap tests

[hieblmi]

This PR threads an injected clock.Clock through swap setup and resume paths so swap timestamps and duration accounting no longer depend directly on time.Now()/time.Since().

It also updates the affected tests to provide test clocks explicitly and adds coverage to ensure loop-out initiation timestamps use the injected clock.

[gemini-code-assist]

Summary of Changes

Hello, 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 refactors the time-related dependencies within the swap lifecycle to use an injectable clock. This significantly improves the testability and reliability of the swap logic by decoupling it from the system's real-time clock, allowing for more controlled and predictable execution in test environments and consistent behavior in production.

Highlights

• Dependency Injection for Time: Introduced an injectable clock.Clock interface throughout the swap setup and resume paths. This change replaces direct calls to time.Now() and time.Since() with calls to the injected clock, making time-dependent logic more deterministic and testable.
• Enhanced Testability: Updated existing tests to explicitly provide clock.NewTestClock() instances to swap configurations. This allows for precise control over time in tests, ensuring consistent and reliable testing of time-sensitive swap operations, such as initiation timestamps and duration accounting.
• Consistent Time Handling: Ensured that all critical timestamping and duration calculations within loop-in and loop-out swaps, including initiation times, state updates, and payment durations, now utilize the injected clock, promoting a unified approach to time management across the codebase.

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

Changelog

• client.go
  • Imported the github.com/lightningnetwork/lnd/clock package.
  • Modified newSwapConfig calls in resumeSwaps, LoopOut, and LoopIn to inject clock.NewDefaultClock().
• cost_migration_test.go
  • Imported the github.com/lightningnetwork/lnd/clock package.
  • Injected clock.NewTestClock(time.Now()) into swapConfig instances within TestCalculateLoopOutCost and TestCostMigration.
• loopd/daemon.go
  • Modified the loop.Resume call to inject clock.NewDefaultClock().
• loopin.go
  • Removed the time package import.
  • Replaced time.Now() with cfg.clock.Now() for initiationTime.
  • Replaced time.Now() with s.clock.Now() for lastUpdateTime in publishOnChainHtlc and setState.
• loopin_test.go
  • Imported the time and github.com/lightningnetwork/lnd/clock packages.
  • Injected clock.NewTestClock(time.Now()) into newSwapConfig calls across various loop-in tests.
• loopout.go
  • Imported the github.com/lightningnetwork/lnd/clock package.
  • Replaced time.Now() with cfg.clock.Now() for initiationTime.
  • Replaced time.Now() with s.clock.Now() for updateTime in persistState.
  • Replaced time.Now() with s.clock.Now() and time.Since(start) with s.clock.Now().Sub(start) in payInvoiceAsync.
  • Added a clock.Clock field to the resumeManager struct.
  • Modified the Resume function to accept and store a clock.Clock instance.
  • Replaced time.Now() with m.clock.Now() in resumeLoopOutPayment.
• loopout_test.go
  • Imported the github.com/lightningnetwork/lnd/clock package.
  • Injected clock.NewTestClock(startTime) into swapConfig instances within various loop-out tests.
  • Added an assertion to verify swap.InitiationTime matches the startTime from the test clock.
• swap.go
  • Imported the github.com/lightningnetwork/lnd/clock package.
  • Added a clock.Clock field to the swapConfig struct.
  • Updated the newSwapConfig function to accept and assign a clock.Clock instance.

Activity

• No human activity has been recorded on this pull request yet.

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.

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

[gemini-code-assist]

Code Review

This pull request effectively refactors the swap lifecycle to use an injected clock.Clock, removing direct dependencies on time.Now() and time.Since(). This is a great improvement for testability and makes the code more robust. The changes in production code are clean and correct.

My main feedback is on the test files. While some tests were updated to use a fixed time, many still use time.Now() when creating a TestClock. This can lead to flaky or non-deterministic tests. For consistency and to ensure test reliability, I've suggested using a fixed timestamp across all test files.

_{Note: Security Review is unavailable for this PR.}

[hieblmi]

@claude review this

[claude]

Code Review for PR 1091 - Inject clocks into swap lifecycle

Overall this is a clean and well-scoped refactor that improves testability by injecting a clock.Clock into the swap lifecycle.

POSITIVE ASPECTS

• The approach is idiomatic: using clock.Clock from lnd is the established pattern in this codebase.
• The test assertion added in testLoopOutPaymentParameters (require.Equal(t, startTime, swap.InitiationTime)) provides real coverage that the injected clock is actually used, not just wired up.
• Converting time.Since(start) to s.clock.Now().Sub(start) in payInvoiceAsync is the correct way to make duration measurement deterministic in tests.

ISSUES

1. Parameter name shadows imported package

In swap.go:newSwapConfig and loopout.go:Resume, the parameter is named 'clock', which shadows the 'clock' package import within the function body. While harmless here, it is a Go anti-pattern that can cause confusing compile errors if the function is ever extended. Convention is typically to use 'clk' or 'swapClock'. Same applies to the Resume function signature in loopout.go.

2. No nil guard for the clock field

newSwapConfig accepts a clock.Clock with no nil check. If a caller passes nil, the first s.clock.Now() call will panic. Consider defaulting to clock.NewDefaultClock() when nil is passed, or documenting that the parameter must not be nil.

3. Missing initiation-time assertion for loop-in

testLoopOutPaymentParameters now asserts swap.InitiationTime == startTime. There is no equivalent assertion for the loop-in path (e.g., in testLoopInSuccess). Adding a parallel check would complete the coverage symmetry and prevent the loop-in clock path from silently regressing.

4. Magic timestamp time.Unix(123, 0) used without explanation

Every test file uses clock.NewTestClock(time.Unix(123, 0)) without any comment. A brief comment or a shared package-level constant would improve readability. The named variable startTime pattern used in testLoopOutPaymentParameters should be followed at the other test sites too.

5. cost_migration.go:124 still uses time.Now()

startTs := time.Now() measures migration wall-clock duration for a log line, which is likely intentional and out of scope here. A brief comment clarifying intent would prevent a future reader from flagging it as an oversight.

MINOR NIT

The resumeManager.clock field is only used in resumeLoopOutPayment. A comment noting that it is not propagated into the swap config (clock injection for the resumeSwaps path happens separately in client.resumeSwaps) would reduce future confusion about why two separate clock injection points exist.

SUMMARY

The core change is correct and the test coverage improvement is welcome. The main items worth addressing before merge are the parameter-name shadowing (item 1) and the missing nil guard (item 2). The rest are suggestions to improve clarity and test parity.

[starius]

LGTM! 🚀
Please fix compilation of each commit. Maybe just squash them into a single commit.

[starius]

The first commit does not compile:

./loopin_test.go:50:68: not enough arguments in call to newSwapConfig

Please make sure each commit compiles and passes tests.

[starius]

Shouldn't we add a similar check as in loopout_test.go? That the time comes from the clock:

require.Equal(t, startTime, swap.InitiationTime)