Skip to content

Expose RpcClient and RestClient interfaces as pub #825

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

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Mar 4, 2021

Useful for use outside of the BlockSource context, e.g., when implementing fee estimation or transaction broadcasting.

Useful for use outside of the BlockSource context, e.g., when
implementing fee estimation or transaction broadcasting.
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #825 (f48e273) into main (4894d52) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #825   +/-   ##
=======================================
  Coverage   90.97%   90.97%           
=======================================
  Files          48       48           
  Lines       26452    26452           
=======================================
  Hits        24064    24064           
  Misses       2388     2388           
Impacted Files Coverage Δ
lightning-block-sync/src/http.rs 95.39% <ø> (ø)
lightning-block-sync/src/rest.rs 67.24% <100.00%> (ø)
lightning-block-sync/src/rpc.rs 79.48% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 96.99% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4894d52...f48e273. Read the comment docs.

Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

LGTM

@TheBlueMatt TheBlueMatt merged commit 1efc0c8 into lightningdevkit:main Mar 5, 2021
@valentinewallace
Copy link
Contributor

Hm I'm sorry if I should've tested this earlier, but this is giving me this error: https://i.imgur.com/kmRusav.png

@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 9, 2021

Hm I'm sorry if I should've tested this earlier, but this is giving me this error: https://i.imgur.com/kmRusav.png

The API needs a concrete type T to transform the JsonResponse into. For that particular example, if you just need the number of blocks, I'd use call_method::<u64>("getblockcount", &[]) similar to this test:

https://github.com/rust-bitcoin/rust-lightning/blob/2cb655b3b188e521028817bf2506b5a99f9071e7/lightning-block-sync/src/rpc.rs#L192

Otherwise, if you need to work with a complex JSON response, then you'll have to define a TryInto to convert into a struct that you either re-use (e.g., Block) or define on your own if no such type exists. Alternatively, if you want to work directly with the JSON, you can define an identity TryInto giving back the JsonResponse. I tend to prefer the former since it encapsulates the conversion logic rather than doing it at the call site.

@valentinewallace
Copy link
Contributor

I do have to get complex structs sometimes. I think I'd prefer to do the more flexible approach (i.e. TryInto giving back the JsonResponse) to avoid waiting on upstream fixes in case I need to make new calls in the future. Should we reopen my old PR? Seems preferable to just return the serde_json::Value directly.

@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 9, 2021

I do have to get complex structs sometimes. I think I'd prefer to do the more flexible approach (i.e. TryInto giving back the JsonResponse) to avoid waiting on upstream fixes in case I need to make new calls in the future. Should we reopen my old PR? Seems preferable to just return the serde_json::Value directly.

Changes shouldn't have to be upstreamed to rust-bitcoin or rust-lightning. I expect they wouldn't be accepted in the former since rust-bitcoin doesn't support the RPC results more generally and we don't want use case-specific code in rust-lightning.

The question is really around data abstraction and encapsulation. The API was design such that anyone can write a conversion function to use with it. So you can simply define a small struct X with only the relevant fields from the JSON response and then impl TryInto<X> for JsonResponse. Doing it this way rather manipulating a JsonResponse directly at the call site is much easier for someone to read -- the conversion details are encapsulated elsewhere and the caller is given an abstraction with only the relevant data.

@valentinewallace
Copy link
Contributor

OK, I see. I was hoping to make the each sample node portion concise enough to include directly in the guide, but with adding this conversion boilerplate it isn't really nearly as concise. So, just wanted to point that out as a bit of a downside.

I agree it encapsulates the conversion logic but I don't think it increases the readability tooo much, kinda confuses it a bit since you have to parse the try_into functions and understand how they fit in (it seems a bit magic at first and warrants explanation for how the whole JsonResponse API is meant to be used).

@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 9, 2021

OK, I see. I was hoping to make the each sample node portion concise enough to include directly in the guide, but with adding this conversion boilerplate it isn't really nearly as concise. So, just wanted to point that out as a bit of a downside.

I agree it encapsulates the conversion logic but I don't think it increases the readability tooo much, kinda confuses it a bit since you have to parse the try_into functions and understand how they fit in (it seems a bit magic at first and warrants explanation for how the whole JsonResponse API is meant to be used).

The answer is always more abstraction. :) Rather than have BitcoindClient expose an RpcClient via get_new_rpc_client, define specific methods on BitcoindClient instead (e.g., get_blockchain_info, create_raw_transaction, etc.). Then users of BitcoindClient don't need to know anything about the JSON interchange format (whether as parameters or as return values). It's an implementation detail that is encapsulated inside BitcoindClient.

@valentinewallace
Copy link
Contributor

valentinewallace commented Mar 9, 2021

I think it's nice when everything the user needs is right there in a code sample in the guide and avoid abstraction layers (which don't add very much, imo) altogether. In my mind, the sample's being made to be married to the docs, so it's being made so self-contained snippets can be pulled out. I think a more production node would indeed have these helpers you mention.

Edit: or rather, the abstraction layers would usually add value, but in this case I think it detracts more.

@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 9, 2021

I think it's nice when everything the user needs is right there in a code sample in the guide and avoid abstraction layers (which don't add very much, imo) altogether. In my mind, the sample's being made to be married to the docs, so it's being made so self-contained snippets can be pulled out. I think a more production node would indeed have these helpers you mention.

Edit: or rather, the abstraction layers would usually add value, but in this case I think it detracts more.

Hmmm... I'd imagine we'd only want to repeat small snippets of the sample in the guide and link to the sample repo where necessary to see more details. Utilizing layers of abstraction allows us to do that while eliding details that could distract from the overall purpose of the guide: demonstrating how to set up a Lightning node using LDK. Those details are an exercise for the reader either by writing the code on their own or peeking at the solution (i.e., the sample node repo).

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.

4 participants