Skip to content

chainntnfs: export NotifierOptions and internal field for interface use#10028

Merged
Roasbeef merged 1 commit intolightningnetwork:masterfrom
ffranr:export-notifier-options
Jul 3, 2025
Merged

chainntnfs: export NotifierOptions and internal field for interface use#10028
Roasbeef merged 1 commit intolightningnetwork:masterfrom
ffranr:export-notifier-options

Conversation

@ffranr
Copy link
Collaborator

@ffranr ffranr commented Jul 2, 2025 •
edited
Loading

Export NotifierOptions and its internal field to enable external satisfaction of the protofsm.DaemonAdapters interface.

@ffranr ffranr self-assigned this Jul 2, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 2, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment •
edited by ffranr
Loading

Choose a reason for hiding this comment

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

ffranr: I've removed this comment, it was pre-rebase and stale.

@ffranr
chainntnfs: export NotifierOptions and internal field for interface use
5cea874 
Export NotifierOptions and its internal field to enable external
satisfaction of the protofsm.DaemonAdapters interface.
@ffranr ffranr force-pushed the export-notifier-options branch from 3d23e32 to 5cea874 Compare July 2, 2025 15:33
@ffranr
Copy link
Collaborator Author

ffranr commented Jul 2, 2025

/gemini summary

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 2, 2025
View reviewed changes
Copy link

@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 introduces several enhancements, including context propagation, improved peer connection management, and flexible database migrations. The changes aim to improve the codebase's robustness and maintainability. I've identified a critical bug in htlcswitch/link.go and a misleading comment in htlcswitch/hop/iterator.go. Once these issues are addressed, this pull request will be a valuable addition to the codebase.

@gemini-code-assist
Copy link

gemini-code-assist bot commented Jul 2, 2025

Summary of Changes

This pull request focuses on increasing the accessibility of the chainntnfs package by exporting key types and functions related to notification options. The primary goal is to enable external packages to satisfy the protofsm.DaemonAdapters interface by providing the necessary public constructs for configuring chain event notifications.

Highlights

  • API Export: I've exported the notifierOptions struct as NotifierOptions and its internal includeBlock field as IncludeBlock. This makes these types and fields accessible from outside the chainntnfs package.
  • Function Export: The defaultNotifierOptions function has been exported as DefaultNotifierOptions, allowing external packages to create default instances of NotifierOptions.
  • Internal Usage Updates: All internal references to the previously unexported notifierOptions struct, its includeBlock field, and the defaultNotifierOptions function have been updated to use their new exported names (NotifierOptions, IncludeBlock, and DefaultNotifierOptions respectively).
Changelog
  • chainntnfs/interface.go
    • Renamed the notifierOptions struct to NotifierOptions (exported).
    • Renamed the includeBlock field within NotifierOptions to IncludeBlock (exported).
    • Renamed the defaultNotifierOptions function to DefaultNotifierOptions (exported).
    • Updated the NotifierOption type alias to reference the newly exported NotifierOptions struct.
    • Updated the WithIncludeBlock function to set the IncludeBlock field.
  • chainntnfs/txnotifier.go
    • Updated the newConfNtfn function signature to accept *NotifierOptions instead of *notifierOptions.
    • Modified the newConfNtfn function to access the exported IncludeBlock field (opts.IncludeBlock).
    • Updated the RegisterConf function to call the exported DefaultNotifierOptions() function.
Activity
  • The author, @ffranr, requested a summary of the pull request.

@ffranr
Copy link
Collaborator Author

ffranr commented Jul 2, 2025

@gemini-code-assist you should remove this comment that you made: #10028 (review)

The comment is stale, pre-rebase.

@gemini-code-assist
Copy link

gemini-code-assist bot commented Jul 2, 2025

Thanks for letting me know about the stale comment, @ffranr. However, as an AI, I am unable to directly remove comments or perform any actions on the pull request. You would need to remove the comment yourself if it's no longer relevant after the rebase.

@guggero
Copy link
Collaborator

guggero commented Jul 2, 2025

Is it possible you're trying to achieve the same thing as this? https://github.com/lightninglabs/lndclient/pull/231/files
Perhaps we can use the lndclient wrapped options?

@ffranr
Copy link
Collaborator Author

ffranr commented Jul 2, 2025

Is it possible you're trying to achieve the same thing as this? https://github.com/lightninglabs/lndclient/pull/231/files Perhaps we can use the lndclient wrapped options?

@guggero I don't think those are the same notifier options that we need for the protofsm.DaemonAdapters interface.

@ffranr ffranr mentioned this pull request Jul 2, 2025
Merged
@ffranr ffranr requested review from Roasbeef and guggero July 2, 2025 16:24
guggero
guggero approved these changes Jul 3, 2025
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Roasbeef
Roasbeef approved these changes Jul 3, 2025
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐴

@Roasbeef Roasbeef merged commit b3eb9a3 into lightningnetwork:master Jul 3, 2025
72 of 75 checks passed
@guggero guggero added this to the v0.19.2 milestone Jul 4, 2025
@guggero guggero mentioned this pull request Jul 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@guggero guggero guggero approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ffranr ffranr

Labels

None yet

Projects

None yet

Milestone

v0.19.2

Development

Successfully merging this pull request may close these issues.

3 participants

@ffranr @guggero @Roasbeef