Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

channeldb: inject clock into database #3797

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Dec 5, 2019

This PR fixes a leftover of the mpp invoice registry pr #3415. There we took a shortcut to expose Now as a public field on the database in order to unblock the stack of dependent prs. This is now converted to a proper clock.Clock dependency.

@joostjager joostjager changed the title channeldb: inject now function channeldb: inject now function [wip, no review] Dec 5, 2019
channeldb/db.go Outdated
@@ -142,7 +142,9 @@ type DB struct {

// Open opens an existing channeldb. Any necessary schemas migrations due to
// updates will take place as necessary.
func Open(dbPath string, modifiers ...OptionModifier) (*DB, error) {
func Open(dbPath string, now func() time.Time, modifiers ...OptionModifier) (
Copy link
Contributor

Choose a reason for hiding this comment

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

why not set time.Now as the default in DefaultOptions(), then add a functional modifier? much smaller diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion, done

@wpaulino wpaulino removed their request for review December 5, 2019 18:34
@bhandras
Copy link
Collaborator

Just saw this. Now that we have clock.Clock you might as well use that instead of the now closure.

@Roasbeef Roasbeef added code health Related to code commenting, refactoring, and other non-behaviour improvements database Related to the database/storage of LND incomplete WIP PR, not fully finalized, but light review possible labels Jan 7, 2020
@joostjager joostjager force-pushed the explicit-now-dependency-channeldb branch from d484505 to 9b9721a Compare January 20, 2020 09:58
@joostjager joostjager changed the title channeldb: inject now function [wip, no review] channeldb: inject now function Jan 20, 2020
@joostjager joostjager removed the incomplete WIP PR, not fully finalized, but light review possible label Jan 20, 2020
@joostjager joostjager changed the title channeldb: inject now function channeldb: inject clock into database Jan 20, 2020
Use our standard clock mock for database time queries.
@joostjager joostjager force-pushed the explicit-now-dependency-channeldb branch from 9b9721a to b600ecd Compare January 20, 2020 10:08
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 🕥

Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

much cleaner, nice fix LGTM 🎸

@joostjager joostjager merged commit b573a5e into lightningnetwork:master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Related to code commenting, refactoring, and other non-behaviour improvements database Related to the database/storage of LND
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants