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

queryroutes: set queryroutes numRoutes param default to 10 #2128

Conversation

mrfelton
Copy link
Contributor

Problem

When calling queryroutes from gRPC, the default value for the num_routes parameter is set to zero. Having a default value of zero is confusing as there would be little point of making the call if you wanted to fetch zero results. Additionally, the documentation states that the default should be 10.

This is inconsistent with the CLI, where the default of 10 is correctly applied.

Also, the documentation for the cli queryroutes command has bad output which this PR addresses as below:

Output from lncli queryroutes --help (before changes):

--num_max_routes value     the max number of routes to be returned (default: 10) (default: 10)

Solution

Set the default value as 10 when no value is provided by the caller.

Updated output from lncli queryroutes --help (after changes):

--num_max_routes value     the max number of routes to be returned (default: 10)

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.

Thanks for the PR! This is something that should be addressed indeed.

cmd/lncli/commands.go Show resolved Hide resolved
lnrpc/rpc.pb.go Outdated Show resolved Hide resolved
@mrfelton mrfelton force-pushed the fix/queryroutes-default-num-routes branch 2 times, most recently from 9f7f6c5 to 14f905e Compare October 30, 2018 22:14
@Roasbeef
Copy link
Member

IMO, it should just error out (at the RPC level) if a value isn't specified. In many places we keep cli level defaults distinct from actual RPC level defaults.

@mrfelton
Copy link
Contributor Author

@Roasbeef What would be a good reason to leave the default empty for RPC whilst setting it to 10 for CLI users? Seems like a better DX and UX to have both aligned, and to have a sensible default supplied (as is the case for the CLI right now).

I think diverging the interfaces in this way introduces unnecessary confusion.

@Roasbeef Roasbeef added rpc Related to the RPC interface routing lncli P4 low prio labels Oct 31, 2018
@wpaulino
Copy link
Contributor

wpaulino commented Nov 9, 2018

👍 for having a sensible default for the RPC itself, rather than returning an error.

@Roasbeef
Copy link
Member

Fails travis atm with:

rpcserver.go:1::warning: file is not gofmted with -s (gofmt)

Should be fixed by running make lint or make fmt locally.

Fix an inconsistency between the gRPC/CLI queryroutes implementations.
Ensure that the numRoutes param always defaults to 10.
@mrfelton mrfelton force-pushed the fix/queryroutes-default-num-routes branch from 14f905e to b3c78d7 Compare November 17, 2018 12:36
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 b4e4f40 into lightningnetwork:master Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lncli P4 low prio routing rpc Related to the RPC interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants