fix: Issue 6787 catch all std::exception in getAsset (AMMInfo)#6845
Open
shortthefomo wants to merge 1 commit intoXRPLF:developfrom
Open
fix: Issue 6787 catch all std::exception in getAsset (AMMInfo)#6845shortthefomo wants to merge 1 commit intoXRPLF:developfrom
shortthefomo wants to merge 1 commit intoXRPLF:developfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Fix
getAssetin theamm_infoRPC handler catching onlystd::runtime_errorfromassetFromJson/issueFromJson, leavingJson::errorand otherstd::exceptionsubtypes uncaught. Such exceptions now propagate to the top-levelcallMethodhandler and return an opaquerpcINTERNALinstead of the expectedrpcISSUE_MALFORMEDerror.Fixes #6787
Context of Change
Bug — present since
getAsset(formerlygetIssue) was introduced in AMMInfo.cpp.assetFromJsondelegates toissueFromJson, which throwsJson::error(a subclass ofstd::exception, notstd::runtime_error) for invalid currency or issuer string values — for example:The original catch clause
catch (std::runtime_error const& ex)did not cover these paths. AnyJson::errorthrown byissueFromJsonwould escapegetAssetuncaught, propagate up through the RPC dispatch, be caught by the genericcallMethodhandler, and returnrpcINTERNAL— a confusing error that gives the client no indication of the actual problem (a malformed asset field).Fix: Widen the catch to
catch (std::exception const& ex)so that all exception types thrown byassetFromJson/issueFromJsonare handled locally and mapped torpcISSUE_MALFORMED.Before / After
Before — client sends
amm_infowith{"asset": {"currency": "!!!BAD!!!"}}:{ "result": { "error": "internal", "error_code": 73, "error_message": "Internal error.", "status": "error" } }After — same request returns the descriptive malformed-issue error:
{ "result": { "error": "issueMalformed", "error_code": 46, "error_message": "Issue is malformed.", "status": "error" } }API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)No API impact. The change only affects the error-path for malformed
asset/asset2inputs toamm_info; all well-formed requests are unaffected.Test Plan
Added
testGetAssetExceptionCoveragetoAMMInfo_test.cpp. It sends anamm_inforequest with anassetfield containing an invalid currency string ("!!!INVALID!!!") — the path that previously triggeredJson::errorand returnedrpcINTERNAL. The test asserts:error(the request was correctly rejected)."internal"— confirming the exception is now caught locally and mapped to the correct error code rather than escaping to the generic handler.