Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

fix(channels): neutrino clients manual channels flagged private #699

Merged
merged 1 commit into from
Aug 22, 2018

Conversation

JimmyMow
Copy link
Member

@JimmyMow JimmyMow commented Aug 21, 2018

This PR we make sure that if a user of ours is using neutrino that we flag their channels as private when they manually open them. Reason for this is because light consumer clients are not expected to have high uptime.

However we cannot hardcode channels as private every time because we do support more power users driving their remote nodes or BTCPay Server with Zap. So we only flag channels as private if the activeConnection is local.

Addresses #681

@coveralls
Copy link

coveralls commented Aug 21, 2018

Pull Request Test Coverage Report for Build 3819

  • 0 of 4 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.006%) to 13.358%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/lib/lnd/methods/channelController.js 0 1 0.0%
app/reducers/channels.js 0 3 0.0%
Totals Coverage Status
Change from base Build 3817: -0.006%
Covered Lines: 512
Relevant Lines: 3347

💛 - Coveralls

@tanx
Copy link

tanx commented Aug 21, 2018

👍

@JimmyMow JimmyMow added this to the v0.2.2-beta milestone Aug 21, 2018
Copy link
Member

@mrfelton mrfelton left a comment

Choose a reason for hiding this comment

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

A few questions and comment on the code @JimmyMow

.flowconfig Outdated
@@ -9,6 +9,7 @@
<PROJECT_ROOT>/dll/.*
<PROJECT_ROOT>/release/.*
<PROJECT_ROOT>/git/.*
<PROJECT_ROOT>/.git/.*
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

@@ -19,13 +19,14 @@ function ensurePeerConnected(lnd, pubkey, host) {
* @return {[type]} [description]
*/
export function connectAndOpen(lnd, event, payload) {
const { pubkey, host, localamt } = payload
const { pubkey, host, localamt, private: privateChannel } = payload
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to change the variable name to privateChannel? Can't it just remain named private? especially as that is what we want it to be named when we pass it into lnd.openChannel below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because private is a reserved word in strict mode

// like remote node and BTCPay Server we will announce to the network as
// these users are using Zap to drive nodes that are online 24/7
const store = new Store({ name: 'settings' })
const { type } = store.get('activeConnection', null)
Copy link
Member

Choose a reason for hiding this comment

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

If activeConnection is not set in the store, store.get should return an empty object, instead of null

const { type } = store.get('activeConnection',{})

Otherwise, we would be trying to destructure null which would cause an error.

@@ -1,3 +1,3 @@
declare module 'module' {
declare module 'grpc' {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably create a new file for this rather than modifying the existing flowtype for module

Copy link
Member Author

Choose a reason for hiding this comment

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

going to remove this as this was fixed in a diff PR and merged

@@ -8,6 +10,16 @@ import channelsReducer, {
OPENING_FAILURE
} from 'reducers/channels'

jest.mock('electron', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed any longer as we've added a manual mock for electron here: https://github.com/LN-Zap/zap-desktop/blob/master/test/unit/__mocks__/electron.js

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -191,7 +192,18 @@ export const openChannel = ({ pubkey, host, local_amt }) => (dispatch, getState)
dispatch(openingChannel())
dispatch(addLoadingPubkey(pubkey))

ipcRenderer.send('lnd', { msg: 'connectAndOpen', data: { pubkey, host, localamt } })
// Grab the activeConnection type from our local store.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Line length should be 120.

@mrfelton
Copy link
Member

@JimmyMow I've created a separate PR to address the issues with flow: #702. That can be removed from this PR.

@JimmyMow JimmyMow force-pushed the fix/private-channels-when-using-default branch 5 times, most recently from c956da1 to ad062c2 Compare August 22, 2018 15:12
Make sure that if a user of ours is using neutrino that we flag their
channels as private when they manually open them. Reason for this is
because light consumer clients are not expected to have high uptime.

However we cannot hardcode channels as private every time because we
do support more power users driving their remote nodes or BTCPay
Server with Zap. So we only flag channels as private if the
activeConnection is local.
@JimmyMow JimmyMow force-pushed the fix/private-channels-when-using-default branch from ad062c2 to 9b837ba Compare August 22, 2018 15:48
@mrfelton
Copy link
Member

Tested ACK 9b837ba

@mrfelton mrfelton merged commit fb9c4b5 into master Aug 22, 2018
@mrfelton mrfelton deleted the fix/private-channels-when-using-default branch August 22, 2018 16:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants