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

make API docs request objects easier to understand and use #33

Open
ZZiigguurraatt opened this issue Nov 19, 2024 · 1 comment
Open

Comments

@ZZiigguurraatt
Copy link

ZZiigguurraatt commented Nov 19, 2024

Overview

In this issue, I propose a few changes to make the API docs a lot easier to use so that one can effectively learn how to build a request object needed for each RPC call. Right now, all the items mentioned below are embedded in the description text, making it easy to miss something (especially because you need to manually click the V to expand the description for each field) and become frustrated because your RPC call isn't working right. The request object of some API calls have a large number of fields (routerrpc.SendPaymentRequest is a good example), making interpreting the documentation fairly tedious.

I think they would be much simpler to understand things if available in tabular form.

Right now in the docs, we have 4 fields

Field | gRPC Type | REST Type | REST Placement

but it would be nice to

Mark which fields are required and which are optional

Some fields are required and some aren't. If we can add a column to the table to clearly differentiate, that would be nice. For example, expand the table to include

Field | Required? | gRPC Type | REST Type | REST Placement

where possible values are Always, Never, or if X is also defined.

In cases where we may have a choice between two required fields, I think we should just mark them all as required and the user will be required to look to see under what conditions it is required and what alternatives the field may have

In cases where we have nested requirements, such as tapchannelrpc.SendPaymentRequest, which require embedding a routerrpc.SendPaymentRequest inside of it, it becomes even more tedious trying to figure out what are the bare minimum number of fields that need to be populated in order for the request to be successful. Making the API docs easier to use is even more important in this case!

Define field alternatives

Some fields have one or more options that can be used. We should be able to clearly know what we can use instead by looking to the right instead of reading through every field's description. For example, expand the table to include

Field | Required? | Mutually exclusive with | gRPC Type | REST Type | REST Placement

An example where we have multiple possible fields is in routerrpc.SendPaymentRequest which has amt and amt_msat.

Define field defaults

Building further, it would be nice to state the default values in a column, rather than in the description text. For example, expand the table to include

Field  |  Required?  |  Default Value  | Mutually exclusive with |  gRPC Type  |  REST Type  |  REST Placement

Some fields have a default and some fields have no default. For example, routerrpc.SendPaymentRequest has a default fee_limit_sat of 0, but amt has no default value.

@guggero guggero transferred this issue from lightninglabs/docs.lightning.engineering Nov 19, 2024
@jamaljsr
Copy link
Member

While I agree that this would indeed be helpful information to expose in the API Docs site, it seems to me that this would only be possible with specific updates made to the *.proto files contained in each of the daemon repos (lnd, loop, etc.). These files don't currently have a consistent indication of which fields are optional/required or what default values will be used. If you aren't aware already, the API docs site is machine generated using the proto files from each repo as it's input.

The Proto3 spec does allow us to indicate which fields are not required using the optional keyword. Unfortunately, we do not make use of this keyword in our proto files. If you look at the router.proto file, you'll see that the optional keyword is never used. However, it does include the word "optional" in the comments for some of the message fields.

    /*
    An optional maximum total time lock for the route. This should not
    exceed lnd's `--max-cltv-expiry` setting. If zero, then the value of
    `--max-cltv-expiry` is enforced.
    */
    int32 cltv_limit = 9;

    /*
    Optional route hints to reach the destination through private channels.
    */
    repeated lnrpc.RouteHint route_hints = 10;

    /*
    An optional field that can be used to pass an arbitrary set of TLV records
    to a peer which understands the new records. This can be used to pass
    application specific data during the payment attempt. Record types are
    required to be in the custom range >= 65536. When using REST, the values
    must be encoded as base64.
    */
    map<uint64, bytes> dest_custom_records = 11;

Maybe we could use the presence of "optional" in the comment to detect fields that are not required. I'm not sure if this is totally reliable though, it may turn up some false positives since this is just a free-form text comment. Each RPC comment would need to be reviewed to be certain.

Regarding the field default values, there is no standard way in the Proto3 spec to specify custom defaults that will be used in cases where the value is omitted in the gRPC request. There are global defaults specified in the spec, but these are for all fields based on the data type. The defaults we use in our apps are all specified in the code. So in order to surface this information in the API Docs site, we'd probably need to come up with a structured way to document the defaults in the comments of the proto files. Then it could be parsed by the site generator here and displayed on the pages. We'd also want to have some process to ensure that the proto files remain in sync with the actual code implementation.

As you can see, this would likely be a pretty big undertaking to update all protos across all 6 repos to exposed the necessary information that then could be used by the site generator here to surface it on the deployed API Docs site. Making any changes here would be the final step of that process.

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

No branches or pull requests

2 participants