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

routerrpc+lncli: enable subserver by default and switch over lncli sendpayment #4128

Merged
merged 10 commits into from
Mar 31, 2020

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Mar 30, 2020

This PR removes the conditional compilation logic from routerrpc. It will be enabled by default. This is not a big change, as it was already enabled in our pre-built binary releases.

With routerrpc being available always, lncli can be switched over to using the SendPayment call on the sub-server.

The old main rpc SendPayment and SendPaymentSync calls are marked as deprecated and expected to be removed in the next major release 0.11.

Note: SendToRoute will be switched over too, but after #3970 lands.

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.

quick pass

subrpcserver_config.go Show resolved Hide resolved
Usage: "a zpay32 encoded payment request to fulfill",
},
cli.Int64Flag{
Name: "fee_limit",
Copy link
Contributor

Choose a reason for hiding this comment

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

not blocking, but could be a good time to rename this to fee_limit_sat to match fee_limit_percent so they both have units

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am in doubt about it. It can be done at any time really too. This parameter already existed.

cmd/lncli/cmd_pay.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
@joostjager
Copy link
Contributor Author

Todo: switch over integration tests to routerrpc.SendPayment 😝 Investigating how much effort that is.

@joostjager joostjager force-pushed the default-routerrpc branch 3 times, most recently from 56ee174 to 325452c Compare March 31, 2020 12:07
@joostjager
Copy link
Contributor Author

Itests switched over. Revealed one bug where routerrpc wasn't setting the payment request, see routerrpc: fix empty payment request bug

lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the default-routerrpc branch 3 times, most recently from 4b61836 to fe5d73a Compare March 31, 2020 13:47
@joostjager joostjager added this to the 0.10.0 milestone Mar 31, 2020
@bhandras bhandras self-requested a review March 31, 2020 16:36
cmd/lncli/cmd_pay.go Show resolved Hide resolved
cmd/lncli/cmd_pay.go Show resolved Hide resolved
cmd/lncli/cmd_pay.go Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
cmd/lncli/cmd_pay.go Show resolved Hide resolved
cmd/lncli/cmd_pay.go Show resolved Hide resolved
cmd/lncli/cmd_pay.go Show resolved Hide resolved
lnrpc/rpc.proto Show resolved Hide resolved
cmd/lncli/cmd_pay.go Outdated Show resolved Hide resolved
@joostjager
Copy link
Contributor Author

Moved itest conversion into separate PR #4132

With multi-part payments, a payment-level route field does not make
sense anymore.
There is an alternative in routerrpc now. It doesn't support REST yet,
but the main rpc SendPayment with its bidirectional streaming isn't
usable via REST either.

SendPaymentSync still serves REST users and cannot be deprecated yet.
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.

LGTM

Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM

@joostjager joostjager merged commit afaabda into lightningnetwork:master Mar 31, 2020
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