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: implement network error handling for verify payment #154

Merged
merged 3 commits into from
Oct 20, 2024

Conversation

Toheeb-Ojuolape
Copy link
Contributor

@Toheeb-Ojuolape Toheeb-Ojuolape commented Oct 7, 2024

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ New Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 👷 Example Application
  • 🧑‍💻 Code Snippet
  • 🎨 Design
  • 📖 Content
  • 🧪 Tests
  • 🔖 Release
  • 🚩 Other

Description

This PR implements a try-and-catch block for the verifyPayment method in invoice.ts and returns false if a network error occurs

Related Tickets & Documents

Resolves #125

Closes #155

@Toheeb-Ojuolape
Copy link
Contributor Author

Hi @rolznz this is the new PR for the issue where I made a commit using the --no-verify flag which did not result in a change in the yarn.lock. I also implemented your error code corrections and I hope you find this acceptable. Thank you.

@rolznz
Copy link
Contributor

rolznz commented Oct 7, 2024

Thanks @Toheeb-Ojuolape it looks great!

Any chance you would be able to write a test too? if not, that's fine.

@Toheeb-Ojuolape
Copy link
Contributor Author

Thanks @Toheeb-Ojuolape it looks great!

Any chance you would be able to write a test too? if not, that's fine.

Hi @rolznz , sorry I won't be able to write a test for it as that would require me to mock both verifyPayment and the network errors which is out of the scope of this current issue. I can add test suites later on if there is an issue created related to unit tests for verifyPayment, validatePreimage and all the other async functions in invoice.ts.

I hope you find this acceptable 🙂

@Toheeb-Ojuolape
Copy link
Contributor Author

Hi @rolznz just following up on this. Once this PR has been merged, I'll create a new issue for writing tests for async functions like verifyPayment, isPaid and validatePreimage in the invoice.ts and assign the task to myself.

I look forward to your feedback.

@Toheeb-Ojuolape
Copy link
Contributor Author

Hi @rolznz, my PR is still here for your approval 🥺. Any chance this Friday is a good time to approve?

I have already created a new issue for tests in invoice.ts which I plan to address once I have closed out on this: #155

@Toheeb-Ojuolape Toheeb-Ojuolape changed the title fix: implemented network error handling for verify payment fix: implement network error handling for verify payment Oct 13, 2024
@rolznz
Copy link
Contributor

rolznz commented Oct 20, 2024

@Toheeb-Ojuolape I added some tests. Thanks!

@rolznz rolznz merged commit 92f285b into getAlby:master Oct 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants