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.

Sign up for GitHub

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

Jump to bottom

funding: move Manager to own package #4757

Merged
merged 12 commits into from
Dec 17, 2020
Merged

funding: move Manager to own package #4757

merged 12 commits into from
Dec 17, 2020

Conversation

Crypt-iQ
Copy link
Collaborator

@Crypt-iQ Crypt-iQ commented Nov 10, 2020 •
edited
Loading

Renames fundingManager to Manager. Moves the Manager to the funding package. Will help a lot with unit testing. The last commit should be viewed with a git client with --color-moved as GitHub is too picky to see the fundingmanager.go -> funding/manager.go as a code-move.

@Crypt-iQ Crypt-iQ added code health Related to code commenting, refactoring, and other non-behaviour improvements funding Related to the opening of new channels with funding transactions on the blockchain labels Nov 10, 2020
halseth
halseth approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM

fmgr/log.go Outdated Show resolved Hide resolved
fundingmanager_test.go Show resolved Hide resolved
fmgr/manager.go Outdated Show resolved Hide resolved
@bhandras bhandras self-requested a review November 12, 2020 14:39
bhandras
bhandras approved these changes Nov 12, 2020
View reviewed changes
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 💯

q: would it be more idiomatic Go to have the package name as funding and then rename FundingManager as Manager?

fundingmanager.go Outdated Show resolved Hide resolved
@Crypt-iQ
Copy link
Collaborator Author

q: would it be more idiomatic Go to have the package name as funding and then rename FundingManager as Manager?

I think funding is a better name for the package and I agree with the rename to Manager.

@Crypt-iQ
Copy link
Collaborator Author

If the package is named funding and what was previously the fundingManager is now the Manager, then what will the interface that Manager satisfies be called?

@Crypt-iQ Crypt-iQ changed the title fmgr: move FundingManager to own package funding: move Manager to own package Nov 24, 2020
@Crypt-iQ
Copy link
Collaborator Author

The interface is named Controller - also removed the PendingChannels() from the Manager as it was totally unused. Perhaps this visibility can be added later once the code is cleaned up a bit.

halseth
halseth suggested changes Nov 25, 2020
View reviewed changes
funding/interfaces.go Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
funding/log.go Outdated Show resolved Hide resolved
log.go Show resolved Hide resolved
bhandras
bhandras approved these changes Nov 27, 2020
View reviewed changes
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, nice refactors! @Crypt-iQ

@Roasbeef Roasbeef added this to the 0.13.0 milestone Dec 11, 2020
@Crypt-iQ Crypt-iQ force-pushed the fmgr_1109 branch 2 times, most recently from ae793c6 to 6b1a42d Compare December 15, 2020 18:09
halseth
halseth approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Nice, I like this latest iteration a lot! Easy to follow commits, LGTM 👍

bhandras
bhandras approved these changes Dec 17, 2020
View reviewed changes
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 💯

@Crypt-iQ
funding: rename from fmgr, change references
61e0f67
@Crypt-iQ
lnd+funding: use funding.WriteOutpoint instead of lnd version
f6524aa 
This commit duplicates the utxonursery's writeOutpoint function
in the funding package so that when the rest of the fundingmanager
code is moved, it can use the WriteOutpoint function for its
channel opening state data.
@Crypt-iQ
multi: move and export funding-related vars to funding package
caa0f5d 
Also moves the lnd global MaxFundingAmount to server.go
@Crypt-iQ Crypt-iQ merged commit e043996 into lightningnetwork:master Dec 17, 2020
@Crypt-iQ Crypt-iQ deleted the fmgr_1109 branch December 17, 2020 16:43
@Roasbeef Roasbeef added this to Done in v0.13.0-beta Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhandras bhandras bhandras approved these changes

@halseth halseth halseth approved these changes

Assignees
No one assigned
Labels
code health Related to code commenting, refactoring, and other non-behaviour improvements funding Related to the opening of new channels with funding transactions on the blockchain
Projects
No open projects
v0.13.0-beta
Done
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Crypt-iQ @bhandras @halseth @Roasbeef