rpcserver: return InvalidArgument for RPC validation errors#2112
rpcserver: return InvalidArgument for RPC validation errors#2112kaldun-tech wants to merge 2 commits into
Conversation
Summary of ChangesHello, 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 improves the robustness of the RPC server by standardizing error reporting for input validation. By switching from generic 'Unknown' errors to specific 'InvalidArgument' gRPC status codes, the API now provides clearer feedback to clients, enabling them to programmatically handle validation failures appropriately. The changes are accompanied by comprehensive unit tests to ensure consistent behavior. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the DecodeAddr, DecodeProof, and ExportProof RPC methods to return codes.InvalidArgument for validation errors, allowing clients to programmatically distinguish bad input from internal errors. The changes include updated release notes and a new unit test file, rpcserver/validation_test.go, to ensure correct error code returns. The review feedback points out several opportunities to condense error return statements to fewer lines, adhering to the repository's style guide for log and error message formatting.
kaldun-tech
force-pushed
the
rpc-handlers-invalid-argument-validation
branch
3 times, most recently
from
43719ba to
d42b490
Compare
|
I'm removing the TestDecodeAddrValidInput and TestExportProofValidInput happy path tests which are causing failures in CI These tests attempted to verify that valid-looking input passes validation and fails later (not with InvalidArgument). IIRC this coverage was recommended by Gemini in my previous PR draft which I closed because it was from the wrong branchpoint. The trouble I have is the tests require initializing the RPCServer with mocked TapAddrBook and ProofArchive dependencies, which is significant infrastructure for minimal value. The integration tests in itest/ already exercise these RPCs with valid input end-to-end. If the validation logic incorrectly rejected valid input, those tests would fail. The remaining validation tests comprehensively cover the InvalidArgument error paths, which is the core goal of this PR. |
Update DecodeAddr, DecodeProof, and ExportProof to return codes.InvalidArgument instead of codes.Unknown for request validation errors. This enables clients to programmatically distinguish bad input from internal errors. Add validation_test.go with tests for invalid input asserts InvalidArgument. Skipped mocking for valid input tests which are already covered by integration tests. Partial fix for Issue lightninglabs#2086 Signed-off-by: kaldun-tech <tsmereka@protonmail.com>
kaldun-tech
force-pushed
the
rpc-handlers-invalid-argument-validation
branch
from
d42b490 to
fe6c7ce
Compare
darioAnongba
left a comment
darioAnongba
left a comment
There was a problem hiding this comment.
Thanks for the PR! Only a small nit request on the validation tests that can be too verbose and specific.
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
Replace wantMsg with wantCode and use assertCode helper to check only the gRPC status code, not the error message. This makes tests less brittle and more focused on the programmatic error code. Signed-off-by: kaldun-tech <tsmereka@protonmail.com>
|
Great catch @darioAnongba I noticed two DecodeProof error paths are not covered by my unit tests:
Both paths correctly return codes.InvalidArgument as shown in the implementation. |
2 participants
Summary
DecodeAddr,DecodeProof, andExportProofto returncodes.InvalidArgumentinstead ofcodes.Unknownfor request validation errorsThis enables clients to programmatically distinguish bad input from internal errors.
Partial fix for #2086
Design Note
Issue #2086 proposed a pattern using a sentinel-error-to-status-code mapping helper at the RPC boundary. This would keep handler internals free of grpc imports.
This PR takes the simpler direct approach of using
status.Errorfinline for the three handlers identified in the issue.The helper pattern is worth considering for a broader refactor if we standardize error handling across all RPC handlers. For now, the direct approach is consistent with existing patterns in the codebase (e.g.,
ListAssets,SendAssetalready use similar inline error wrapping).Test plan
rpcserver/validation_test.gothat assertcodes.InvalidArgumentis returned for validation failuresmake lintmake unit pkg=rpcserver