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

lnrpc+routerrpc+lncli: add msat fields #3706

Merged
merged 2 commits into from Nov 14, 2019

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Nov 11, 2019

Lightning supports payments at the msat granularity, but lnd does not expose this on the rpc interface. This PR adds msat fields for payment amounts and fee limits.

Fixes #3689

NOTE: This renames some of the existing rpc fields which is a breaking change for users building against our proto files. The change doesn't change anything on the wire level.

@joostjager joostjager added this to the 0.9.0 milestone Nov 11, 2019
@joostjager joostjager self-assigned this Nov 11, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 11, 2019
@joostjager joostjager force-pushed the joostjager:pay-msat branch 2 times, most recently from 734cf41 to 79d46bb Nov 11, 2019
@joostjager joostjager requested review from wpaulino and guggero Nov 11, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 11, 2019
lnrpc/rpc.proto Outdated Show resolved Hide resolved
Copy link
Collaborator

guggero left a comment

Nice to see more consistency in the API!
If we introduce a braking change in the API anyway, wouldn't this be the moment to also just get rid of the sat amounts and only deal with msat? Or at least deprecate them and remove in the next release?

lnrpc/marshall_utils.go Show resolved Hide resolved
lnrpc/marshall_utils.go Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:pay-msat branch from 79d46bb to 8df0aaa Nov 12, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 12, 2019

If we introduce a braking change in the API anyway, wouldn't this be the moment to also just get rid of the sat amounts and only deal with msat?

Get rid of it is also a breaking change on the wire level. So that needs to be preceded with deprecation first.

Outstanding question on that above

@joostjager joostjager force-pushed the joostjager:pay-msat branch 2 times, most recently from 23be804 to c980d8a Nov 13, 2019
@joostjager joostjager force-pushed the joostjager:pay-msat branch from c980d8a to 09b3101 Nov 13, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 13, 2019

Left existing field unchanged, to prevent cascading changes in projects that depend on it.

@joostjager joostjager requested review from guggero and wpaulino Nov 13, 2019
Copy link
Collaborator

guggero left a comment

LGTM 🎉

v0.9.0-beta automation moved this from Needs Review to Approved Nov 13, 2019
@joostjager joostjager merged commit e8b306b into lightningnetwork:master Nov 14, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 62.437%
Details
v0.9.0-beta automation moved this from Approved to Done Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.