Skip to content

Conversation

nalinbhardwaj
Copy link
Contributor

Replaces current balance field by 3 fields, TotalBalance, ConfirmedBalance and UnconfirmedBalance to store the respective balance information.

Closes #415.

Copy link
Contributor

@mlerner mlerner left a comment

Choose a reason for hiding this comment

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

I'm doing a first pass review of this to help out with the review backlog. This PR looks mostly good to me, with a few nits.

rpcserver.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make TotalBal => totalBal because this is an intermediate variable.

rpcserver.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make ConfirmedBal => confirmedBal because this is an intermediate variable.

rpcserver.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this TODO needs to be added back - it is not implemented or in scope for this PR.

@mlerner
Copy link
Contributor

mlerner commented Nov 26, 2017

Two other things: you will need to rebase your branch with master to resolve, and also reformat commit messages, as mentioned in the style guide -
https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md#44-model-git-commit-messages

All commits should begin with the subsystem or package primarily affected by the change - in this case you could split up your commits into proto (for the *.proto changes) and lncli for code that uses those changes.

@nalinbhardwaj
Copy link
Contributor Author

@mlerner : I think I've addressed all changes you mentioned now. Thanks for letting me know. I'll do similar changes with #417 commit messages as well. :)

Note: Seems like the travis build is failing for unrelated reasons.

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.

Thanks for the PR!

Tested locally and works as advertised. I just have some minor nits w.r.t to the coding style prescribed within the repo. Once those nits are addressed, I'll get this merged!

rpcserver.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Nit: missing a period here, and the comments below.

All comments should be full sentences.

rpcserver.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Initial line should be modified to read something like:

WalletBalance returns total unspent outputs (confirmed and unconfirmed), all

@nalinbhardwaj
Copy link
Contributor Author

@Roasbeef : Addressed the nits, sorry about those.

@Roasbeef
Copy link
Member

Roasbeef commented Dec 3, 2017

Hey @nalinbhardwaj, can you rebase this? A recent PR went in that also modified the protos. You'll just need to rebuild them.

@nalinbhardwaj
Copy link
Contributor Author

@Roasbeef done. Not sure why there’s a commit you made “with me” though? I don’t see a way to remove that offline, let me know if that’s a problem.

@Roasbeef
Copy link
Member

Roasbeef commented Dec 3, 2017

@nalinbhardwaj you'll need to remove that last commit. When rebasing you should ensure that your master branch is up to date with the remote origin branch.

@nalinbhardwaj
Copy link
Contributor Author

Done now @Roasbeef sorry about the weird error, not sure why I got that into commit history in first attempt. It's clean now.

@Roasbeef
Copy link
Member

Roasbeef commented Dec 4, 2017

Is your PGP key available on any public key server?

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 e189324 into lightningnetwork:master Dec 4, 2017
@nalinbhardwaj
Copy link
Contributor Author

@Roasbeef I have it on keybase

@nalinbhardwaj nalinbhardwaj deleted the walletbalance branch December 4, 2017 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants