rpcserver: return consistent grpc NotFound error for GetChaninfo & LookupInvoice#10064
Conversation
So that we can properly convert this NotFound error back to our `graphdb.ErrEdgeNotFound` error if we are making this call to another node via RPC.
For a kvdb backed invoices DB, we sometimes will return the ErrNoInviocesCreated error if no invoices have ever been created on the node. In these cases we should still wrap the returned error in the grpc NotFound code.
ellemouton
force-pushed
the
getChanInfoGrpcErr
branch
from
0e04c0e to
a5d5172
Compare
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ellemouton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on improving the consistency and predictability of gRPC error responses within the rpcserver, specifically for 'not found' scenarios. By standardizing the NotFound status code for channel and invoice lookups, it enhances the reliability of API interactions and supports downstream systems that rely on precise error interpretation. The changes are thoroughly validated with new integration tests.
Highlights
- Consistent gRPC Error Handling: I've updated the
rpcserverto return a consistentcodes.NotFoundgRPC status code forGetChanInfowhen a channel is not found (specifically, whengraphdb.ErrEdgeNotFoundoccurs). This is crucial for external projects, like the 'remote graph' project, to correctly interpret these errors. - Invoice Lookup Error Consistency: The
LookupInvoiceRPC now also returnscodes.NotFoundnot only when a specific invoice is not found (invoices.ErrInvoiceNotFound) but also when no invoices have been added to the database yet (invoices.ErrNoInvoicesCreated), ensuring more consistent error reporting. - New Integration Test: I've added a new integration test,
testGRPCNotFound, to verify that various RPC endpoints, includingGetChanInfo,GetNodeInfo,SendCustomMessage,LookupHTLCResolution, andLookupInvoice, correctly return thecodes.NotFoundgRPC status when querying for non-existent entities. - Testing Helper Functions: To facilitate robust testing of gRPC error codes, I've introduced new
AssertErrhelper functions in thelntest/rpc/lnd.gopackage forGetChanInfo,GetNodeInfo,SendCustomMessage,LookupInvoice, andLookupHTLCResolution.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly updates GetChanInfo and LookupInvoice to return a consistent gRPC NotFound error, which is a good improvement for API consistency. The changes are well-supported by a new integration test that also covers other related RPCs, ensuring broader correctness.
My feedback is mainly focused on improving documentation and comments to align with the project's style guide, particularly regarding the 80-character line length limit. Overall, this is a solid contribution.
ellemouton
force-pushed
the
getChanInfoGrpcErr
branch
from
a5d5172 to
ed7b056
Compare
| require.ErrorContains(h, err, errStr) | ||
| } | ||
|
|
||
| // GetChanInfoAssertErr makes an RPC call to GetChanInfo and asserts an error |
There was a problem hiding this comment.
TIL about creating such test utility methods and can see how useful they are. My question is, when one needs to use them and is not sure of the exact error msg, is it safe to pass an empty string "" as an arg?
There was a problem hiding this comment.
Hmm, that would look weird to me and I'd probably point it out in a PR I was reviewing. The other pattern we sometimes do is instead have a GetXXXErr (instead of GetXXXAssertErr) function that asserts there is an error and then returns the error. Then you can inspect the type/message of the error in the itest (if there are multiple options).
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Thanks for picking this up! LGTM🦾
| require.NoError(ht, err) | ||
|
|
||
| _, err = rand.Read(rHash) | ||
| require.NoError(ht, err) |
There was a problem hiding this comment.
nit: just fyi there are ht.Random32Bytes() and ht.RandomPreimage()
There was a problem hiding this comment.
cool - gonna do this in a commit in the migration plugin /itest follow up PR I open today 👍
5 participants
Replaces #9810
Commit author credit has been retained 👍
This PR fixes the GetChanInfo endpoint so that it returns the correct grpc status code in the
case where the channel is not found.
LookupInvoice is also fixed to behave correctly in regards to this grpc status code.
The GetChanInfo fix is needed for the "remote graph" project so that we can correctly convert
NotFounderrors back to
graphdb.ErrEdgeNotFounderrors.