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

Fix amount field #8097

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Conversation

markettes
Copy link
Contributor

@markettes markettes commented Oct 16, 2023

This PR attempts to solve #8038

Change Description

Description of change / link to associated issue.

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@markettes markettes marked this pull request as draft October 18, 2023 14:21
@markettes markettes marked this pull request as ready for review October 18, 2023 14:22
@markettes markettes closed this Nov 22, 2023
@markettes markettes reopened this Nov 22, 2023
@hieblmi hieblmi self-requested a review November 24, 2023 14:00
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

🎉Thank you very much for picking this up @marekttes. The PR is neat and clean.
I only have a question about the MinConfs check in the server.

lnrpc/lightning.proto Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@markettes markettes requested a review from hieblmi November 24, 2023 16:07
@guggero guggero self-requested a review November 27, 2023 22:57
Copy link
Collaborator

@guggero guggero 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. I think the CLI needs a couple of changes, otherwise the code looks okay.
Can you please make sure you check the pull request check list? The commits should be squashed into two, one for the RPC changes and one for the CLI. And a change log entry for the 0.18 release should be added to docs/release-notes/release-notes-v0.18.md.

cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
@ellemouton
Copy link
Collaborator

@markettes - see the guidelines for how to model commit titles. For your commits, it would probably be along the lines of: lnrpc: add MinConfs to WalletBalanceRequest, lncli: show balance for sweepall and finally docs: release notes update for 0.18.0.

Please also remove the merge commit 🙏 instead, rebase on master before pushing

@markettes markettes force-pushed the fix-amount-field branch 3 times, most recently from 9fa8111 to 92dba38 Compare December 1, 2023 17:19
@markettes
Copy link
Contributor Author

All done @ellemouton 😄

@guggero
Copy link
Collaborator

guggero commented Dec 1, 2023

The linter is failing because of a line that is longer than 80 characters (you need to count tabs as 8 spaces, best to set up your IDE that way to make it easier).

@markettes markettes force-pushed the fix-amount-field branch 2 times, most recently from a14899e to 0cd39c2 Compare December 2, 2023 01:10
@markettes
Copy link
Contributor Author

Ready now @guggero, sorry, my bad!

Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this! I think we can remove an else statement to improve readability.

cmd/lncli/commands.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Two more nits, otherwise looks, thanks!

cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
@guggero guggero linked an issue Dec 4, 2023 that may be closed by this pull request
@guggero guggero merged commit ad88396 into lightningnetwork:master Dec 4, 2023
23 of 25 checks passed
@guggero guggero added this to the v0.18.0 milestone Dec 4, 2023
@markettes markettes deleted the fix-amount-field branch December 4, 2023 11:24
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.

Send coins amount field doesn't work in some/all cases
4 participants