APIv2(AccountTx): Added error messages on AccountTx #4571

PeterChen13579 · 2023-06-13T14:46:14Z

High Level Overview of Change

Fixed error messages for AccountTx method for Github Issue #4545 and #4288

Context of Change

Certain input for AccountTx method should of error'ed out

Type of Change

Invalid request by user results in Error message

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

Before and After: refer to github link. Using "api_version": 2 calls the updated error messages

Updated existing unit test to check error based off of VPI versioning as adding error messages is considered a breaking c hange.

thejohnfreeman · 2023-06-13T15:09:38Z

src/test/rpc/AccountTx_test.cpp

+ RPC::apiMaximumSupportedVersion == 1
+ ? BEAST_EXPECT(
+ hasTxs(env.rpc("json", "account_tx", to_string(p))))
+ : BEAST_EXPECT(isErr(
+ env.rpc("json", "account_tx", to_string(p)),
+ rpcLGR_IDX_MALFORMED));


You should copy these tests for an explicit api_version == 1, like this.

To follow on @thejohnfreeman's suggestion, loop over both versions for these tests, because we need to keep supporting version 1 indefinitely and ensure it's working correctly.

ximinez · 2023-06-13T15:53:08Z

src/ripple/rpc/impl/RPCHelpers.h

-constexpr unsigned int apiMaximumSupportedVersion = 1;
+constexpr unsigned int apiMaximumSupportedVersion = 2;
 constexpr unsigned int apiBetaVersion = 2;


Don't change the apiMaximumSupportedVersion until the updated version is locked in and final and ready for release. We have apiBetaVersion variable right here and the [beta_rpc_api] config option available to allow testing the changes. (It might not be a bad idea to enable BETA_RPC_API = true by default in envconfig.cpp.)

Just wondering what do you mean by "until the updated version is locked in and final and ready for release."? Do you you mean until PR #4522 is merged into develop, so I don't have to change apiMaximumSupportedVersion to 2?

I'll be popping over to add the same kind of comment to #4568 and #4552 after I'm done here.

"until the updated version is locked in and final and ready for release" means that once a version of rippled is released with apiMaximumSupportedVersion is set to 2, then we are saying that API v2 is Done. Once that happens we can not legitimately make any further changes to API version 2. To the best of my knowledge, API version 2 is still in beta, under active development, and still changing.

Now, if you (and #4568 and #4552) are saying that version 2 is done, let's talk about version 2 being done. This will be the first I've heard about it.

If you want a node to properly handle API v2 requests, the correct way to do that, for now, is with the [beta_rpc_api] config option. And if you want to do it in unit tests, set it up in the config similar to how it's done in https:/XRPLF/rippled/blob/develop/src/test/rpc/AccountInfo_test.cpp#L209-L212, or just by setting cfg.BETA_RPC_API=true; directly.

Please see also #4573.

Just to close the loop on this: API v2 will be considered "Done" in the 1.12 release. Any breaking changes in 1.13 will be API v3.

ximinez · 2023-06-13T15:55:02Z

src/test/rpc/AccountTx_test.cpp

+ RPC::apiMaximumSupportedVersion == 1
+ ? BEAST_EXPECT(
+ hasTxs(env.rpc("json", "account_tx", to_string(p))))
+ : BEAST_EXPECT(isErr(
+ env.rpc("json", "account_tx", to_string(p)),
+ rpcLGR_IDX_MALFORMED));


To follow on @thejohnfreeman's suggestion, loop over both versions for these tests, because we need to keep supporting version 1 indefinitely and ensure it's working correctly.

src/test/rpc/AccountTx_test.cpp

arihantkothari · 2023-06-27T15:38:18Z

src/test/rpc/AccountTx_test.cpp

+ void
+ testParametersApiV2()


Might wanna:

Add a parameter for api_version.

Call this method in a loop over versions from apiMinimumSupportedVersion to apiBetaVersion.

Remove the other method.

Make the assertion on the error value conditional on the api_version parameter, using condition < 2 for the old behavior and else for the new behavior.

#4552 (comment)

arihantkothari · 2023-06-27T21:56:17Z

src/ripple/rpc/handlers/AccountTx.cpp

 {
 Json::Value response;
+ // if ledger_index_min or max is specified, then ledger_hash or ledger_index
+ // should not be specified. Error out if it is
+ if (context.apiVersion > 1)


not so important, but I'm wondering if it'll be nice to make constants like API_VERSION_1 and API_VERSION_2 instead of using raw numbers 1 and 2 since we are using it in a lot of places ?

I don't think it's worth it when we're just using consecutive integers to name the versions.

arihantkothari · 2023-06-27T22:05:31Z

src/test/rpc/AccountTx_test.cpp

@@ -108,7 +108,7 @@ class AccountTx_test : public beast::unit_test::suite
 };

 void
- testParameters()
+ testParameters(unsigned int apiVersion)


Not related to this PR, but wondering why the test cases in this file do not have testcase(testCaseDesctiption) ?

My guess is that we don't have any written, enforced guidelines for tests.

I think it might be useful to add testcase(testCaseDescription + v<apiVersion>) so that users can see if the unit test is actually running on different versions when they run it. what do you think ? (maybe out of scope for this PR ?)

I think our test framework and tests are long overdue for a redesign with firm and written guidelines, but I won't worry about the inconsistency in this PR.

Certain inputs for the AccountTx method should return an error. In other words, an invalid request from a user or client now results in an error message. Since this can change the response from the API, it is an API breaking change. This commit maintains backward compatibility by keeping the existing behavior for existing requests. When clients specify "api_version": 2, they will be able to get the updated error messages. Update unit tests to check the error based on the API version. * Fix XRPLF#4288 * Fix XRPLF#4545

Changed error messages on AccountTx

4dadab6

PeterChen13579 changed the title ~~Changed error messages on AccountTx~~ Added error messages on AccountTx Jun 13, 2023

thejohnfreeman requested changes Jun 13, 2023

View reviewed changes

ximinez requested changes Jun 13, 2023

View reviewed changes

This was referenced Jun 13, 2023

Enable api_version 2 #4568

Closed

APIv2(ledger_entry): return "invalidParams" when fields missing #4552

Merged

Enable the Beta RPC API (v2) for all unit tests: #4573

Merged

PeterChen13579 added 2 commits June 14, 2023 10:48

Fixed unit test based on comments given

2f3c7e3

Added clang format

f123188

ximinez approved these changes Jun 14, 2023

View reviewed changes

intelliot requested a review from thejohnfreeman June 15, 2023 20:26

Merge branch 'XRPLF:develop' into AccountTxErrorFixes

42ca45f

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

thejohnfreeman approved these changes Jun 20, 2023

View reviewed changes

intelliot added this to the 1.12 milestone Jun 20, 2023

intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 21, 2023

arihantkothari suggested changes Jun 22, 2023

View reviewed changes

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

PeterChen13579 added 2 commits June 23, 2023 09:47

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

7c838eb

changed unit test based on Ed's change

ac88045

intelliot removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 23, 2023

arihantkothari suggested changes Jun 27, 2023

View reviewed changes

changed unittest based on Arihant's comment

86a9b31

intelliot requested a review from arihantkothari June 27, 2023 21:28

arihantkothari reviewed Jun 27, 2023

View reviewed changes

arihantkothari approved these changes Jun 27, 2023

View reviewed changes

Merge branch 'develop' into AccountTxErrorFixes

2e1e1a8

intelliot merged commit d34e8be into XRPLF:develop Jun 29, 2023

intelliot mentioned this pull request Jul 18, 2023

Proposed 1.12.0-b2 #4627

Merged

1 task

intelliot added the Clio Reviewed label Aug 1, 2023

intelliot changed the title ~~Added error messages on AccountTx~~ APIv2(AccountTx): Added error messages on AccountTx Aug 1, 2023

intelliot mentioned this pull request Aug 1, 2023

APIv2(AccountTx): Added error messages on AccountTx XRPLF/clio#799

Closed

intelliot added the API V2 Testing label Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APIv2(AccountTx): Added error messages on AccountTx #4571

APIv2(AccountTx): Added error messages on AccountTx #4571

PeterChen13579 commented Jun 13, 2023 •

edited

Loading

thejohnfreeman Jun 13, 2023

ximinez Jun 13, 2023

ximinez Jun 13, 2023

PeterChen13579 Jun 13, 2023

ximinez Jun 13, 2023

ximinez Jun 14, 2023

intelliot Jun 28, 2023

ximinez Jun 13, 2023

arihantkothari Jun 27, 2023

arihantkothari Jun 27, 2023

arihantkothari Jun 27, 2023

thejohnfreeman Jun 27, 2023

arihantkothari Jun 27, 2023

thejohnfreeman Jun 27, 2023

arihantkothari Jun 28, 2023 •

edited

Loading

thejohnfreeman Jun 28, 2023

APIv2(AccountTx): Added error messages on AccountTx #4571

APIv2(AccountTx): Added error messages on AccountTx #4571

Conversation

PeterChen13579 commented Jun 13, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

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

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

arihantkothari Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterChen13579 commented Jun 13, 2023 •

edited

Loading

arihantkothari Jun 28, 2023 •

edited

Loading