Price Oracle: validate input parameters and extend test coverage: #5013

gregtatcam · 2024-05-03T18:53:43Z

High Level Overview of Change

Validate trim, time_threshold, document_id are valid Int, UInt, or string convertible to UInt. Validate base_asset and quote_asset are valid currency. Update error codes. Extend Oracle and GetAggregatePrice unit-tests.
Denote unreachable coverage code.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

Before / After

Fixes invalid input parameters conversion to uint. For instance, 1.2 was converted to 1 if passed to trim, time_threshold, or document_id. This only allows valid UInt values. Similarly, base_asset and quote_asset were not validated to be a valid currency code. This fix only allows valid currency codes in base_asset and quote_asset.

Test Plan

Extended Oracle and GetAggregatePrices tests.

Validate trim, time_threshold, document_id are valid Int, UInt, or string convertible to UInt. Validate base_asset and quote_asset are valid currency. Update error codes. Extend Oracle and GetAggregatePrice unit-tests. Denote unreachable coverage code.

codecov-commenter · 2024-05-03T19:12:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.0%. Comparing base (f650949) to head (848fa68).
Report is 79 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5013     +/-   ##
=========================================
+ Coverage     70.9%   71.0%   +0.1%     
=========================================
  Files          796     796             
  Lines        66792   66793      +1     
  Branches     10998   10986     -12     
=========================================
+ Hits         47377   47420     +43     
+ Misses       19415   19373     -42

Files with missing lines	Coverage Δ
src/ripple/app/tx/impl/DeleteOracle.cpp	`100.0% <ø> (+27.5%)`	⬆️
src/ripple/app/tx/impl/SetOracle.cpp	`100.0% <ø> (+5.6%)`	⬆️
src/ripple/rpc/handlers/GetAggregatePrice.cpp	`100.0% <100.0%> (+8.0%)`	⬆️
src/ripple/rpc/handlers/LedgerEntry.cpp	`81.9% <100.0%> (+1.0%)`	⬆️

... and 4 files with indirect coverage changes

mvadari · 2024-05-06T20:17:35Z

src/ripple/rpc/handlers/GetAggregatePrice.cpp

                        Json::StaticString const& field,
                        unsigned int def =
                            0) -> std::variant<std::uint32_t, error_code_i> {
        if (params.isMember(field))
        {
-            if (!params[field].isConvertibleTo(Json::ValueType::uintValue))
-                return rpcORACLE_MALFORMED;
+            if (!validUInt(params, field))


What is the difference between these two lines of code?

If I'm reading the code correctly, it's that the old version accepted booleans and floats and would just convert them?

isConvertible is

return (other == nullValue && value_.uint_ == 0) || (other == intValue && value_.uint_ <= (unsigned)maxInt) || other == uintValue || other == realValue || other == stringValue || other == booleanValue;

which means it can convert a wider range of other type. For instance, it'll convert 1.2 to 1, which is invalid in this case.

If I'm reading the code correctly, it's that the old version accepted booleans and floats and would just convert them?

Correct.

mvadari · 2024-05-06T20:39:29Z

src/ripple/rpc/handlers/GetAggregatePrice.cpp


        meta = ledger->txRead(prevTx).second;

+        prevChain = chain;


What does moving this line do? I don't really understand what prevChain is doing, and there don't appear to be any tests checking this behavior.

This handles an unlikely case when CreatedNode/ModifiedNode not found in the inner loop for AffectedNodes. If this happens then prevChain is going to be equal to chain and the iteration stops. I don't know if it's possible to reproduce it unless the ledger state is changed in a way, which is not possible to do with the transactions.

Does moving this line of code make any difference other than the slight efficiency improvement from running fewer times?

prevChain has to be assigned outside of the inner loop. Initially prevChain is nullptr and chain points to oracle. If there is no AffectedNodes then both prevChain and chain are unchanged and the check (prevChain == chain) is not going to fail.

mvadari · 2024-05-06T20:42:49Z

src/ripple/rpc/handlers/GetAggregatePrice.cpp

+            if (params[field].asString().empty())
+                return rpcINVALID_PARAMS;


Isn't this check already included in currencyFromJson?

Yes, it could be removed. It's just a bit faster.

mvadari · 2024-05-06T20:45:27Z

src/test/app/Oracle_test.cpp

-            BEAST_EXPECT(
-                oracle.expectLastUpdateTime(testStartTime.count() + 450));
+            BEAST_EXPECT(oracle.expectLastUpdateTime(
+                static_cast<std::uint32_t>(testStartTime.count() + 450)));


Is there a reason the other lines were switched to using closeTime() but this one still uses testStartTime?

The two lines that used testStartTime should have not actually used it because testStartTime is already added in Oracle::set to the passed in LastUpdateTime value.

mvadari · 2024-05-06T21:03:01Z

src/test/app/Oracle_test.cpp

@@ -622,6 +678,39 @@ struct Oracle_test : public beast::unit_test::suite
        }
    }

+    void
+    testInvalidLedgerEntry()


Should these tests be here or in LedgerRPC_test?

Thanks. It should be moved.

mvadari · 2024-05-06T21:04:15Z

src/test/jtx/Oracle.h

@@ -39,16 +55,16 @@ using DataSeries = std::vector<std::tuple<
 struct CreateArg
 {
    std::optional<AccountID> owner = std::nullopt;
-    std::optional<std::uint32_t> documentID = 1;
+    std::optional<AnyValue> documentID = 1;


Why are these types becoming less specific? Isn't that checked at the validation level?

I wanted to be able to test a wider range of valid/invalid values similar to how QA does it. Unfortunately the test framework does its own validation and will not call rpc if the parameters don't match their defined SField type. The validation is done because the transaction is signed. I left this change so that eventually can extend the testing to support unsigned transactions.

mvadari · 2024-05-07T17:09:10Z

src/test/jtx/impl/Oracle.cpp

-
-    auto jr = env.rpc("json", "get_aggregate_price", to_string(jv));
+        toJson(jv[jss::time_threshold], *timeThreshold);
+    // Convert "%None%" to None


Should"%None%" be a global test constant, since it's a special case?

…PLF#5013) * Price Oracle: validate input parameters and extend test coverage: Validate trim, time_threshold, document_id are valid Int, UInt, or string convertible to UInt. Validate base_asset and quote_asset are valid currency. Update error codes. Extend Oracle and GetAggregatePrice unit-tests. Denote unreachable coverage code. * Set one-line LCOV_EXCL_LINE * Move ledger_entry tests to LedgerRPC_test.cpp * Add constants for "None" * Fix LedgerRPC test --------- Co-authored-by: Scott Determan <[email protected]>

gregtatcam requested review from thejohnfreeman and mvadari May 3, 2024 19:22

Set one-line LCOV_EXCL_LINE

017fc2c

thejohnfreeman approved these changes May 6, 2024

View reviewed changes

mvadari reviewed May 6, 2024

View reviewed changes

Move ledger_entry tests to LedgerRPC_test.cpp

b67d3c3

mvadari reviewed May 7, 2024

View reviewed changes

gregtatcam added 2 commits May 7, 2024 14:58

Add constants for "None"

ee7247c

Fix LedgerRPC test

da269df

mvadari approved these changes May 7, 2024

View reviewed changes

Merge branch 'develop' into oracle-fixes1

848fa68

seelabs merged commit f4da2e3 into XRPLF:develop May 9, 2024
17 checks passed

seelabs mentioned this pull request May 16, 2024

Proposed 2.2.0-rc2 #5018

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Price Oracle: validate input parameters and extend test coverage: #5013

Price Oracle: validate input parameters and extend test coverage: #5013

gregtatcam commented May 3, 2024

codecov-commenter commented May 3, 2024 •

edited by codecov bot

Loading

mvadari May 6, 2024

mvadari May 6, 2024

gregtatcam May 6, 2024

gregtatcam May 6, 2024

mvadari May 6, 2024

gregtatcam May 6, 2024

mvadari May 6, 2024

gregtatcam May 6, 2024

mvadari May 6, 2024

gregtatcam May 6, 2024

mvadari May 6, 2024 •

edited

Loading

gregtatcam May 6, 2024

mvadari May 6, 2024

gregtatcam May 6, 2024

mvadari May 6, 2024

gregtatcam May 6, 2024

mvadari May 7, 2024 •

edited

Loading

		if (params[field].asString().empty())
		return rpcINVALID_PARAMS;

Price Oracle: validate input parameters and extend test coverage: #5013

Price Oracle: validate input parameters and extend test coverage: #5013

Conversation

gregtatcam commented May 3, 2024

High Level Overview of Change

Type of Change

Before / After

Test Plan

codecov-commenter commented May 3, 2024 • edited by codecov bot Loading

Codecov Report

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

mvadari May 6, 2024 • edited Loading

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

mvadari May 7, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 3, 2024 •

edited by codecov bot

Loading

mvadari May 6, 2024 •

edited

Loading

mvadari May 7, 2024 •

edited

Loading