[Feature] Implements a ping endpoint (http)

[anmode]

fixes: #180

1. Adds ping endpoint to HTTP API
2. Implement Ping logic in Watchtower plugin to interact with CLN.
3. Modified and created get_request , post_request and request This explain better Visit
4. Created test for each function such as get_request , post_request and request

[sr-gi]

@anmode as a general comment, do not create fix X commits to just amend something created in the previous commit.

The rule of thumb is: If a fix can be squashed, it has to be squashed.

We do create separate commits for functionality that can be logically separated, or for things that build on top of each other things and that may make sense to have split, but not for modification suggestions on PR comments. In your case, all commits need to be squashed.

Also, while we are at it, commits need (short) descriptive titles and a message body describing what's being changed. Same goes for PRs. e.g:

Add endpoint ❌ --> Adds ping endpoint to the HTTP API ✅ Pingpong ❌ --> Implements ping/pong logic for the tower and CLN plugin ✅

[anmode]

Thank you for the feedback. I apologize for not following the best practices of committing and PRs. I will make sure to follow them in the future. I understand that all my commits should be squashed and that commits should have descriptive titles and messages.

[anmode]

Thanks! I addressed all the suggestions i hope. and checked the test locally. Result- Sucessful

[anmode]

Should i squash d459af4 and 4e5170c ?

[sr-gi]

Should i squash d459af4 and 4e5170c ?

Yes, as mentioned in the review, all commits need to be squashed

[anmode]

Should i squash d459af4 and 4e5170c ?

Yes, as mentioned in the review, all commits need to be squashed

Ohh sure!! Thanks

[sr-gi]

Should i squash d459af4 and 4e5170c ?

Yes, as mentioned in the review, all commits need to be squashed

Wait a min! 😅 I confused. All commits should be squashed into a single commit?

Or i was thinking as two have meaningful commits only. If a small fix commit that has to be squashed to other commit!!!

None of the commits but the first are meaningful on their own, they are fixes upon fixes or adding things that were missing, hence why they need to be all squashed into a single commit.

[anmode]

Should i squash d459af4 and 4e5170c ?

Yes, as mentioned in the review, all commits need to be squashed

Wait a min! 😅 I confused. All commits should be squashed into a single commit? Or i was thinking as two have meaningful commits only. If a small fix commit that has to be squashed to other commit!!!

None of the commits but the first are meaningful on their own, they are fixes upon fixes or adding things that were missing, hence why they need to be all squashed into a single commit.

okayy!! Thanks got it! Done

[anmode]

23d75df-

1. request function was added and the get_request and post_request functions were refactored to use it- Tested successfully
2. Move the ping function to the main.rs but face some problem as rpcMethod allow only two fixed arguments. I m facing an issue with tower_net_address(how can I define it) and I define proxy and then pass it.
3. Created ping rpcMethod

error-

Error[E0277]: the trait bound `watchtower_plugin::net::http::RequestError: std::error::Error` is not satisfied
   --> watchtower-plugin/src/main.rs:305:78
    |
305 |     let response = get_request(&tower_net_addr, Endpoint::Ping, &proxy).await?;
    |                                                                              ^ the trait `std::error::Error` is not implemented for `watchtower_plugin::net::http::RequestError`

[anmode]

Testing is left of request , get_request and post_request. I'll do it in the next commit. Thanks!

[anmode]

There may be errors in test-making...Needs review, please. But all passed in cargo test.

[sr-gi]

Do not merge master into your branch. Rebase the branch so it forks from master's HEAD.

This should be a single commit.

[sr-gi]

There may be errors in test-making... [...] But all passed in cargo test.

wdym?

[anmode]

Do not merge master into your branch. Rebase the branch so it forks from master's HEAD.

This should be a single commit.

Actually...this is my fault i did create pR for docker image in master branch of mine. I didn't noticed. Now I'm stuck. What to do ?

But ok I'll try to do

[anmode]

There may be errors in test-making... [...] But all passed in cargo test.

wdym?

Oops sorry! I was trying to say. If code for test look good or not? Functionality wise it is ok..as i tested it!

[sr-gi]

Do not merge master into your branch. Rebase the branch so it forks from master's HEAD.

This should be a single commit.

Actually...this is my fault i did create pR for docker image in master branch of mine. I didn't noticed. Now I'm stuck. What to do ?

But ok I'll try to do

Rebase master so you get the latest updates from upstream, then rebase this to the latest commit on upstream master instead of on your local master.

From now on try not to work on your master branch.

[anmode]

I think I did! is it ok?? Thanks. for docker pR should I continue on master only?

[sr-gi]

Thanks. for docker pR should I continue on master only?

Sorry but I don't understand what you mean

[anmode]

Thanks. for docker pR should I continue on master only?

Sorry but I don't understand what you mean

actually for this pR! ok i did rebased and squashed the commits and here branch is pingpong but i was just asking about my other PR which is https://github.com/talaia-labs/rust-teos/pull/197 so here by mistake i continue development over master branch so should i continue on master in docker pR?

[sr-gi]

Thanks. for docker pR should I continue on master only?

Sorry but I don't understand what you mean

actually for this pR! ok i did rebased and squashed the commits and here branch is pingpong but i was just asking about my other PR which is #197 so here by mistake i continue development over master branch so should i continue on master in docker pR?

Yeah, the PR is already open, so I would say yes. Do create separate branches for PRs from now on though, so it's less messy.

[sr-gi]

There may be errors in test-making... [...] But all passed in cargo test.

wdym?

Oops sorry! I was trying to say. If code for test look good or not? Functionality wise it is ok..as i tested it!

Tests are failing, and so is formatting. Check the issues and fix them so we can do a final review. Do make sure all tests are passing and all commits have been squashed before requesting the review.

[anmode]

Ohh okay!! Thanks, there is error in one of the test - async fn test_wrong_endpoint and I fixed it. should i continue that change in this pR ?and i solved the formatting error (Lint one).

[anmode]

I'm bit afraid of this!. but I'll try to solve:

     =========================== short test summary info ============================

FAILED test.py::test_unreachable_watchtower - Failed: Timeout >60.0s
FAILED test.py::test_auto_retry_watchtower - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
FAILED test.py::test_manually_retry_watchtower - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
FAILED test.py::test_misbehaving_watchtower - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
FAILED test.py::test_get_appointment - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
ERROR test.py::test_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_unreachable_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_auto_retry_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_manually_retry_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_misbehaving_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_get_appointment - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:

[sr-gi]

I'm bit afraid of this!. but I'll try to solve:

     =========================== short test summary info ============================

FAILED test.py::test_unreachable_watchtower - Failed: Timeout >60.0s
FAILED test.py::test_auto_retry_watchtower - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
FAILED test.py::test_manually_retry_watchtower - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
FAILED test.py::test_misbehaving_watchtower - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
FAILED test.py::test_get_appointment - TimeoutError: Unable to find "[re.compile('Server started with public key')]" in logs.
ERROR test.py::test_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_unreachable_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_auto_retry_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_manually_retry_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_misbehaving_watchtower - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:
ERROR test.py::test_get_appointment - ValueError: 
Node errors:
 - lightningd-2: Node exited with return code -11
Global errors:

Interesting. This is happening because you are actually trying to define an rpc command called ping, when CLN already has a ping command, that's why you saw logs like:

lightningd: FATAL SIGNAL 11 (version v0.12.1)
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable
lightningd: libbacktrace: no debug info in Mach-O executable

I guess we'll have to rename the command to pingtower

[anmode]

failures:

---- net::http::tests::test_get_request stdout ----
thread 'net::http::tests::test_get_request' panicked at 'Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.', watchtower-plugin/src/net/http.rs:577:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- net::http::tests::test_request stdout ----
thread 'net::http::tests::test_request' panicked at 'Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.', watchtower-plugin/src/net/http.rs:545:18

failures:
    net::http::tests::test_get_request
    net::http::tests::test_request

test result: FAILED. 89 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 7.18s.

I tried the backtracing! But unable to get where i am creating the new runtime in the existing runtime. So it blocks. but I wonder why post_request is not failing if there is issue in request function. I tried to modify the test too as i think request function is quite ok!. Sorry I need a little help. I am learning debugging in Rust. So I can perform more better Thanks but yaa I am trying....https://ryhl.io/blog/async-what-is-blocking/ as far i understood.

        let api_mock = server
            .mock("POST", Endpoint::Register.path().as_str())
            .with_status(200)
            .with_header("content-type", "application/json")
            .create_async()
            .await; 

api_mock.assert();

This logic is creating a problem

[anmode]

Ohh done!! I fix it and updated the pR description too!

[anmode]

Should I add the response too in docs? as I just add the command

[sr-gi]

Should I add the response too in docs? as I just add the command

The command description is enough.

[anmode]

Done!

[sr-gi]

ACK de9cd71

[anmode]

Thanks! got to know some new things @mariocynicys @sr-gi !

[sr-gi]

@mariocynicys this is also missing a review

[mariocynicys]

Thanks @sr-gi @mariocynicys. My first contribution towards TEOS. Going to be merged soon. This pR help me a lot.

Well done! You need to sign your commit though before merging it.

[anmode]

Thanks @sr-gi @mariocynicys. My first contribution towards TEOS. Going to be merged soon. This pR help me a lot.

Well done! You need to sign your commit though before merging it.

Ohh sure!

[anmode]

Done!! 1bb5bde

[sr-gi]

Needs rebasing

[anmode]

Needs rebasing

done! 222925170295e0e64b424ca9bd049c563a7a6e0d