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

rpcserver+lnd_test: fixing unassigned UnsettledBalance field #2289

Merged
merged 1 commit into from Jan 11, 2019

Conversation

ccdle12
Copy link
Contributor

@ccdle12 ccdle12 commented Dec 7, 2018

fixes #2274

This PR adds a loop that sums the amount of PendingHtlcs to the UnsettledBalance field for Channels. Also adds a test in lnd_test to ensure that the UnsettledBalance field is actually updated.

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour gRPC channels needs review PR needs review by regular contributors labels Dec 7, 2018
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.

thanks for the PR @ccdle12! great to see a thorough test accompanying the change as well. left a few comments for an initial pass

rpcserver.go Outdated
@@ -2157,6 +2157,12 @@ func (r *rpcServer) ListChannels(ctx context.Context,
}
}

var unsettledBalance int64
for _, htlc := range channel.PendingHtlcs {
unsettledBalance += htlc.Amount
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to do the sumation in the for loop above?

lnd_test.go Show resolved Hide resolved
lnd_test.go Outdated
}

// Force close Alices channel with Carol.
_, _, err = net.CloseChannel(ctxb, net.Alice, chanPointAlice, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can use the closeChannelAndAssert method, which will fully teardown the channel. Here's an example from the test above:

ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout)
closeChannelAndAssert(ctxt, t, net, net.Alice, chanPoint, false)

lnd_test.go Outdated
const chanAmt = btcutil.Amount(1000000)
ctxb := context.Background()
ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
numInvoices := 6
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: define this as const down where payAmt is defined

@ccdle12
Copy link
Contributor Author

ccdle12 commented Dec 7, 2018

@cfromknecht Thanks for review! appreciate the insights into the tests, I'll update according to the comments

@ccdle12 ccdle12 closed this Dec 8, 2018
@ccdle12 ccdle12 reopened this Dec 8, 2018
@ccdle12
Copy link
Contributor Author

ccdle12 commented Dec 8, 2018

@cfromknecht I've updated the PR according to the suggestions but it's strange I'm receiving no test failures when I run make check locally but failing in travis?

@Roasbeef Roasbeef added needs testing PR hasn't yet been actively tested on testnet/mainnet first pass review done PR has had first pass of review, needs more tho and removed needs review PR needs review by regular contributors labels Dec 11, 2018
@halseth
Copy link
Contributor

halseth commented Dec 11, 2018

@ccdle12 The Travis failure is unrelated to your change, I'm working on a fix :)

@ccdle12
Copy link
Contributor Author

ccdle12 commented Dec 11, 2018

@halseth thanks! definitely puts my mind at ease a bit :)

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Changes LGTM, mostly just nits.

When addressing review feedback, "fixup" commits would be preferred, like the case for the last three commits. Otherwise, on first glance, they look like they introduce new behavior rather than addressing an existing comment.

lnd_test.go Show resolved Hide resolved
lnd_test.go Outdated
}

return true
}, time.Second*15)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can use defaultTimeout here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

lnd_test.go Outdated
expectedBalance := numInvoices * payAmt

// Closure helper to check Unsettled Balance.
assertUnsettledBalance := func(expected int,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this needs to be defined as a closure? Doesn't seem to be used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

lnd_test.go Show resolved Hide resolved
@ccdle12
Copy link
Contributor Author

ccdle12 commented Dec 12, 2018

@wpaulino thanks, appreciate the detailed review, I'll get started on the comments

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018
@ccdle12 ccdle12 force-pushed the unsettled-balance branch 2 times, most recently from 1e4507b to 90de921 Compare December 22, 2018 04:36
@ccdle12 ccdle12 closed this Jan 7, 2019
@ccdle12 ccdle12 deleted the unsettled-balance branch January 7, 2019 19:43
High Priority automation moved this from In progress to Done Jan 7, 2019
@ccdle12 ccdle12 restored the unsettled-balance branch January 7, 2019 19:46
@ccdle12 ccdle12 reopened this Jan 7, 2019
High Priority automation moved this from Done to In progress Jan 7, 2019
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.

@ccdle12 coming along nicely! a few last style/timeout changes

lnd_test.go Outdated
}

// Force and assert the channel closure.
ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

should use channelCloseTimeout instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

lnd_test.go Outdated
}

// Open a channel between Alice and Carol.
ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

can use channelOpenTimeout here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

lnd_test.go Outdated

// Create a paystream from Alice to Carol to enable Alice to make
// a series of payments.
alicePayStream, err := net.Alice.SendPayment(ctxb)
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably have a ctxt with defaultTimeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

lnd_test.go Outdated
ctxb := context.Background()

// Create carol in hodl mode.
carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"})
Copy link
Contributor

Choose a reason for hiding this comment

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

this line looks a little long to me. i'd suggest wrapping the args like:

carol, err := new.NewNode("Carol", []string{
        "--debughtlc",
        "--hodl.exit-settle",
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@ccdle12
Copy link
Contributor Author

ccdle12 commented Jan 7, 2019

@cfromknecht Thanks! I'll fixup according to the reviews and respond to the comments accordingly.

@cfromknecht
Copy link
Contributor

Awesome thanks @ccdle12! I'm happy with these changes, @wpaulino do you have anything else?

@wpaulino
Copy link
Contributor

wpaulino commented Jan 8, 2019

LGTM after a squash and rebase 🕺🏻

@cfromknecht
Copy link
Contributor

Indeed this should be ready to land after a rebase!

rpcserver: summing PendingHtlcs and assigning to UnsettledBalance

lnd_test: adding tests for UnsettledBalance
@ccdle12
Copy link
Contributor Author

ccdle12 commented Jan 11, 2019

@cfromknecht, @wpaulino

Squashed and rebased,

thanks guy, really appreciate all the help and the reviews!

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.

Thanks for tackling this @ccdle12, LGTM! 🦒

High Priority automation moved this from In progress to Final Testing -- Ready For Merge Jan 11, 2019
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.

Great work, @ccdle12! A one-line fix with a solid test addition 👏

LGTM 💯

@halseth halseth merged commit a324691 into lightningnetwork:master Jan 11, 2019
High Priority automation moved this from Final Testing -- Ready For Merge to Done Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channels enhancement Improvements to existing features / behaviour first pass review done PR has had first pass of review, needs more tho gRPC needs testing PR hasn't yet been actively tested on testnet/mainnet
Projects
No open projects
High Priority
  
Done
Development

Successfully merging this pull request may close these issues.

[listchannels] fix unsettled_balance
5 participants