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

fix post http request #10

Merged
merged 2 commits into from
Apr 4, 2023
Merged

fix post http request #10

merged 2 commits into from
Apr 4, 2023

Conversation

Bashy
Copy link
Collaborator

@Bashy Bashy commented Apr 4, 2023

This dirty fix needs to be adapted, i just made the post http request to work, so the payment_request is retrieved from lnbits response.

Adaptations needed.

@Bashy Bashy self-assigned this Apr 4, 2023
@Bashy Bashy marked this pull request as draft April 4, 2023 17:25
dirty fix for post http request to work
Copy link
Contributor

Choose a reason for hiding this comment

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

We still might want to refactor this class and use the symfony HttpClient instead of file_get_contents()? But we can delay that decision in a follow up iteration.

Copy link
Collaborator Author

@Bashy Bashy Apr 4, 2023

Choose a reason for hiding this comment

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

To be honest I took time to find a way to use Symphony HttpClient but I didn't find a way to achieve it, I agree that Symphony HttpClient might be more adapted to do the http request

@Chemaclass Chemaclass marked this pull request as ready for review April 4, 2023 19:59
public/index.php Show resolved Hide resolved
@@ -41,21 +34,19 @@ public function requestInvoice(float $satsAmount, string $metadata): array
// 'description_hash' => hash('sha256', $metadata),
];

$response = $this->httpFacade->post($endpoint, [
'headers' => [
$response = $this->httpApi->postRequestInvoice(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in a future iteration, we can inject options directly to the HttpClient class, and we don't need to pass the $endpoint as a parameter, what do you think?

Something like:

$response = $this->httpApi->postRequestInvoice(
    body: json_encode($content, JSON_THROW_ON_ERROR),
    headers: [
        'Content-Type' => 'application/json',
        'X-Api-Key' => $this->options['api_key'],
    ],
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, nice observation. We can try that out in a follow up iteration. What are your thoughts on this, @Bashy ?

Copy link
Collaborator Author

@Bashy Bashy Apr 4, 2023

Choose a reason for hiding this comment

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

It can be done as long as we don't have any other backend type.
Other kind of LN backend will have different $endpoint, headers and body values though, which kind of structure would be the best to keep interoperability in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, that's right. Ok, let's keep it like it is for now. Thanks for bringing this idea up 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are not going to have so many endpoints, my suggestion is to try to have specific methods in the HttpApi for each endpoint, where each of them can "auto-solve its own dependencies" (I mean, the parameters, endpoint, etc).

It's about concreteness over abstractness in this case, but to be honest, I am still not sure about the scalability of it, it would be nice to think over it in the future 😄

@JesusValera JesusValera merged commit 087f6d1 into main Apr 4, 2023
@JesusValera JesusValera deleted the drity-post-request-fix branch April 4, 2023 20:13
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