Added Error messages on Gateway_Balance and Channel_Authorize #4577

PeterChen13579 · 2023-06-20T19:07:45Z

High Level Overview of Change

Fixed/added error messages for Gateway_balance(issue #4290 | #4548 ) and channel_authorize(#4289 )

Context of Change

Added error messages when user sends an incorrect request. (Check the above Github issue link for more info.) Everything is under API Version 2 as it is a breaking change.

Type of Change

Breaking change (fix or feature that would cause existing functionality to not work as expected)
[X ] Tests (You added tests for code that already exists, or your new feature included in this PR)

Added own unit test for API Version 2;

src/test/rpc/GatewayBalances_test.cpp

This reverts commit 690ce23.

src/test/app/PayChan_test.cpp

Co-authored-by: Arihant Kothari <40741486+arihantkothari@users.noreply.github.com>

src/test/app/PayChan_test.cpp

src/ripple/rpc/handlers/GatewayBalances.cpp

intelliot · 2023-06-30T03:18:38Z

Hi @PeterChen13579 - please sign your commits.

intelliot · 2023-07-01T06:43:00Z

Commit signature looks good. Can you confirm that this PR is ready for review?

PeterChen13579 · 2023-07-03T14:50:44Z

Commit signature looks good. Can you confirm that this PR is ready for review?

Yes, it's ready for review 👍

arihantkothari · 2023-07-04T22:01:26Z

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -900,7 +903,14 @@ keypairForSignature(Json::Value const& params, Json::Value& error)

        if (!keyType)
        {
-            error = RPC::invalid_field_error(jss::key_type);
+            if (context.has_value() && context.value().get().apiVersion > 1)


nit: keep it consistent - use 1u if you are using it everywhere else in the PR. Or keep it 1 if you want to use that everywhere but keep it consistent.

intelliot · 2023-07-06T20:10:10Z

@mounikakun would you be able to review this?

mounikakun · 2023-07-07T17:35:02Z

@intelliot I am not familiar with c++ code, so I cannot review this.

gregtatcam · 2023-07-07T18:42:22Z

src/ripple/rpc/handlers/GatewayBalances.cpp

+            // One or more fields are specified incorrectly.
+            // invalidHotwallet should be used when the account exists, but does
+            // not have currency issued by the account from the request.
+            if (context.apiVersion < 2u)


Please add rpcINALID_HOT_WALLET error and inject it into the result.

gregtatcam · 2023-07-07T18:42:59Z

src/ripple/rpc/handlers/GatewayBalances.cpp

+            }
+            else
+            {
+                result[jss::error] = "invalidParams";


Please inject rpcINVALID_PARAMS into result.

gregtatcam · 2023-07-07T18:44:37Z

src/ripple/rpc/handlers/PayChanClaim.cpp

@@ -55,7 +55,7 @@ doChannelAuthorize(RPC::JsonContext& context)
        return RPC::missing_field_error(jss::secret);

    Json::Value result;
-    auto const [pk, sk] = RPC::keypairForSignature(params, result);
+    auto const [pk, sk] = RPC::keypairForSignature(params, result, context);


consider passing context.apiVersion instead.

gregtatcam · 2023-07-07T18:46:01Z

src/ripple/rpc/impl/RPCHelpers.cpp

+keypairForSignature(
+    Json::Value const& params,
+    Json::Value& error,
+    std::optional<std::reference_wrapper<JsonContext>> context)


consider std::optional<uint> const& apiVersion instead.

gregtatcam · 2023-07-07T18:47:52Z

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -900,7 +903,14 @@ keypairForSignature(Json::Value const& params, Json::Value& error)

        if (!keyType)
        {
-            error = RPC::invalid_field_error(jss::key_type);
+            if (context.has_value() && context.value().get().apiVersion > 1u)


Then this would be replaced with:

if (apiVersion > 1u)
...
else
...

gregtatcam · 2023-07-07T19:03:35Z

src/test/app/PayChan_test.cpp

@@ -24,9 +24,9 @@
 #include <ripple/protocol/PayChan.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
-#include <test/jtx.h>
-
+#include <ripple/rpc/impl/RPCHelpers.h>


Why did you change the headers order? The original order and the new line looks right to me.

gregtatcam · 2023-07-07T19:18:48Z

src/test/app/PayChan_test.cpp

+        args[jss::key_type] = "ed255191";
+        args[jss::seed] = "snHq1rzQoN2qiUkC3XF5RyxBzUtN";
+        args[jss::amount] = 51110000;
+        if (apiVersion < 2u)


Consider moving the testVersion loop inside this function. The channel above doesn't need to be created twice. Then this is going to be:

for (auto apiVersion = RPC::apiMinimumSupportedVersion; apiVersion <= RPC::apiBetaVersion; ++apiVersion) { testcase("PayChan Channel_Auth RPC Api " + std::to_string(apiVersion)); args[jss::api_version] = apiVersion; auto const rs = env.rpc( "json", "channel_authorize", args.toStyledString())[jss::result]; auto const error = apiVersion < 2u ? "invalidParams" : "badKeyType"; BEAST_EXPECT(rs[jss::error] == error); }

gregtatcam · 2023-07-07T19:19:10Z

src/test/app/PayChan_test.cpp

@@ -2139,6 +2183,12 @@ struct PayChan_test : public beast::unit_test::suite
        testAccountChannelsRPC(features);
        testAccountChannelsRPCMarkers(features);
        testAccountChannelsRPCSenderOnly(features);
+        for (auto testVersion = RPC::apiMinimumSupportedVersion;


Consider moving the loop inside the function.

gregtatcam · 2023-07-07T19:24:36Z

src/test/rpc/GatewayBalances_test.cpp

@@ -28,123 +29,159 @@ class GatewayBalances_test : public beast::unit_test::suite
 {
 public:
    void
-    testGWB(FeatureBitset features)
+    testGWB(FeatureBitset features, unsigned int apiVersion)


This really looks to me like two different tests. Consider keeping the original testGWB() as is and adding say testGWBApiV2(FeatureBitset), which has the added code.

gregtatcam · 2023-07-07T19:33:20Z

src/test/rpc/GatewayBalances_test.cpp

+        {
+            testGWB(sa - featureFlowCross, testVersion);
+            testGWB(sa, testVersion);
+        }


Per above suggestion this then can be changed to:

for (auto feature: {sa - featureFlowCross, sa}) { testGWB(feature); testGWBApiV2(feature); }

gregtatcam

👍 LGTM

PeterChen13579 · 2023-07-10T19:52:52Z

@gregtatcam Thank you so much for reviewing my PR. I really appreciate it 😄

gregtatcam · 2023-07-10T19:57:11Z

@gregtatcam Thank you so much for reviewing my PR. I really appreciate it 😄

You are welcome!

shawnxie999 · 2023-07-10T19:59:26Z

src/ripple/protocol/impl/ErrorCodes.cpp

@@ -76,6 +76,7 @@ constexpr static ErrorInfo unorderedErrorInfos[]{
    {rpcINTERNAL,               "internal",             "Internal error.", 500},
    {rpcINVALID_LGR_RANGE,      "invalidLgrRange",      "Ledger range is invalid.", 400},
    {rpcINVALID_PARAMS,         "invalidParams",        "Invalid parameters.", 400},
+    {rpcINVALID_HOTWALLET,      "invalidHotWallet",     "Invalid hotwalloet.", 400},


Invalid hotwalloet.

is this a typo?

shawnxie999 · 2023-07-10T20:02:36Z

src/ripple/rpc/impl/RPCHelpers.cpp

+            else
+            {
+                error = RPC::invalid_field_error(jss::key_type);
+            }


are the brackets needed here?

shawnxie999 · 2023-07-10T20:10:24Z

src/ripple/rpc/impl/RPCHelpers.cpp

+keypairForSignature(
+    Json::Value const& params,
+    Json::Value& error,
+    uint apiVersion)


would it be better to use the standard type unsigned int than uint. like how it was defined in Context?

shawnxie999 · 2023-07-11T18:21:48Z

src/ripple/rpc/handlers/GatewayBalances.cpp

+            else
+            {
+                RPC::inject_error(rpcINVALID_PARAMS, result);
+            }


brackets here as well

intelliot · 2023-09-18T23:05:47Z

Replaced by APIv2(gateway_balances, channel_authorize): update errors #4618

gateway_balances * When `account` does not exist in the ledger, return `actNotFound` * (Previously, a normal response was returned) * Fix #4290 * When required field(s) are missing, return `invalidParams` * (Previously, `invalidHotWallet` was incorrectly returned) * Fix #4548 channel_authorize * When the specified `key_type` is invalid, return `badKeyType` * (Previously, `invalidParams` was returned) * Fix #4289 Since these are breaking changes, they apply only to API version 2. Supersedes #4577

gateway_balances * When `account` does not exist in the ledger, return `actNotFound` * (Previously, a normal response was returned) * Fix XRPLF#4290 * When required field(s) are missing, return `invalidParams` * (Previously, `invalidHotWallet` was incorrectly returned) * Fix XRPLF#4548 channel_authorize * When the specified `key_type` is invalid, return `badKeyType` * (Previously, `invalidParams` was returned) * Fix XRPLF#4289 Since these are breaking changes, they apply only to API version 2. Supersedes XRPLF#4577

PeterChen13579 added 4 commits June 20, 2023 11:08

Error messages for ApiV2

f7c5c65

added clang format

2b78768

changed ledger->read to exist

2eb175e

quick fix

e493534

intelliot requested review from injaelee and mounikakun June 20, 2023 20:26

PeterChen13579 changed the title ~~Error messages~~ Added Error messages on Gateway_Balance and Channel_Authorize Jun 21, 2023

arihantkothari reviewed Jun 22, 2023

View reviewed changes

src/test/rpc/GatewayBalances_test.cpp Outdated Show resolved Hide resolved

PeterChen13579 added 5 commits June 22, 2023 15:28

changed env unit test based on ED's commit

690ce23

Revert "changed env unit test based on ED's commit"

0cd5145

This reverts commit 690ce23.

Merge remote-tracking branch 'upstream/master' into Error_messages

c843248

Merge remote-tracking branch 'upstream/develop' into Error_messages

614cea4

changed unit test based on ED's API Beta commit

29971ca

arihantkothari reviewed Jun 22, 2023

View reviewed changes

src/test/app/PayChan_test.cpp Outdated Show resolved Hide resolved

Update src/test/app/PayChan_test.cpp

311e66c

Co-authored-by: Arihant Kothari <40741486+arihantkothari@users.noreply.github.com>

intelliot added API Change Will Need Documentation labels Jun 23, 2023

arihantkothari suggested changes Jun 27, 2023

View reviewed changes

src/test/app/PayChan_test.cpp Outdated Show resolved Hide resolved

PeterChen13579 added 3 commits June 27, 2023 14:30

changed unittest based on Arihant's comment

3adae5b

added clang format

dad2e8e

Fixed issue #4548 as dealing with same rpc

7344b30

arihantkothari suggested changes Jun 28, 2023

View reviewed changes

src/ripple/rpc/handlers/GatewayBalances.cpp Outdated Show resolved Hide resolved

PeterChen13579 added 2 commits June 28, 2023 14:06

Fix for arihant's comment

b1bae4d

compare to 2u, for faster step

da46159

sign commit attempt

41fcf31

intelliot added this to the 1.12 milestone Jul 4, 2023

intelliot requested a review from gregtatcam July 4, 2023 00:13

arihantkothari reviewed Jul 5, 2023

View reviewed changes

intelliot assigned PeterChen13579 Jul 5, 2023

quick fix on Arihant's comment

682a1a4

intelliot requested a review from arihantkothari July 6, 2023 03:22

arihantkothari approved these changes Jul 6, 2023

View reviewed changes

gregtatcam requested changes Jul 7, 2023

View reviewed changes

intelliot removed the request for review from mounikakun July 8, 2023 06:15

PeterChen13579 added 3 commits July 10, 2023 10:34

Merge remote-tracking branch 'upstream/develop' into Error_messages

920cd81

fixes based on Greg's comments

21c5ebd

move account/HW outside of loop

38e7f8e

gregtatcam approved these changes Jul 10, 2023

View reviewed changes

shawnxie999 reviewed Jul 10, 2023

View reviewed changes

typo fix

ce680b5

shawnxie999 reviewed Jul 10, 2023

View reviewed changes

refactor based on shawn's comments

092f6c3

shawnxie999 reviewed Jul 11, 2023

View reviewed changes

src/ripple/rpc/handlers/GatewayBalances.cpp

else

{

RPC::inject_error(rpcINVALID_PARAMS, result);

}

Copy link

Collaborator

shawnxie999 Jul 11, 2023

Choose a reason for hiding this comment

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

brackets here as well

PeterChen13579 closed this by deleting the head repository Jul 11, 2023

PeterChen13579 mentioned this pull request Jul 12, 2023

APIv2(gateway_balances, channel_authorize): update errors #4618

Merged

arihantkothari mentioned this pull request Jul 14, 2023

APIv2(account_tx, noripple_check): return error for invalid input (fix #4543) #4620

Merged

intelliot removed API Change Will Need Documentation labels Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Error messages on Gateway_Balance and Channel_Authorize #4577

Added Error messages on Gateway_Balance and Channel_Authorize #4577

PeterChen13579 commented Jun 20, 2023 •

edited

Loading

intelliot commented Jun 30, 2023

intelliot commented Jul 1, 2023

PeterChen13579 commented Jul 3, 2023

arihantkothari Jul 4, 2023

intelliot commented Jul 6, 2023

mounikakun commented Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam Jul 7, 2023

gregtatcam left a comment

PeterChen13579 commented Jul 10, 2023

gregtatcam commented Jul 10, 2023

shawnxie999 Jul 10, 2023

shawnxie999 Jul 10, 2023

shawnxie999 Jul 10, 2023

shawnxie999 Jul 11, 2023

intelliot commented Sep 18, 2023

Added Error messages on Gateway_Balance and Channel_Authorize #4577

Added Error messages on Gateway_Balance and Channel_Authorize #4577

Conversation

PeterChen13579 commented Jun 20, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

intelliot commented Jun 30, 2023

intelliot commented Jul 1, 2023

PeterChen13579 commented Jul 3, 2023

Choose a reason for hiding this comment

intelliot commented Jul 6, 2023

mounikakun commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

PeterChen13579 commented Jul 10, 2023

gregtatcam commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Sep 18, 2023

PeterChen13579 commented Jun 20, 2023 •

edited

Loading