Skip to content

Commit

Permalink
Price Oracle: validate input parameters and extend test coverage: (#5013
Browse files Browse the repository at this point in the history 
)

* 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]>
  • Loading branch information
@gregtatcam @seelabs
gregtatcam and seelabs authored May 9, 2024
1 parent f650949 commit f4da2e3
Show file tree
Hide file tree
Showing 9 changed files with 472 additions and 157 deletions.
12 changes: 8 additions & 4 deletions src/ripple/app/tx/impl/DeleteOracle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ TER
DeleteOracle::preclaim(PreclaimContext const& ctx)
{
if (!ctx.view.exists(keylet::account(ctx.tx.getAccountID(sfAccount))))
return terNO_ACCOUNT;
return terNO_ACCOUNT; // LCOV_EXCL_LINE

if (auto const sle = ctx.view.read(keylet::oracle(
ctx.tx.getAccountID(sfAccount), ctx.tx[sfOracleDocumentID]));
Expand All @@ -60,8 +60,10 @@ DeleteOracle::preclaim(PreclaimContext const& ctx)
else if (ctx.tx.getAccountID(sfAccount) != sle->getAccountID(sfOwner))
{
// this can't happen because of the above check
// LCOV_EXCL_START
JLOG(ctx.j.debug()) << "Oracle Delete: invalid account.";
return tecINTERNAL;
// LCOV_EXCL_STOP
}
return tesSUCCESS;
}
Expand All @@ -74,18 +76,20 @@ DeleteOracle::deleteOracle(
beast::Journal j)
{
if (!sle)
return tesSUCCESS;
return tecINTERNAL; // LCOV_EXCL_LINE

if (!view.dirRemove(
keylet::ownerDir(account), (*sle)[sfOwnerNode], sle->key(), true))
{
// LCOV_EXCL_START
JLOG(j.fatal()) << "Unable to delete Oracle from owner.";
return tefBAD_LEDGER;
// LCOV_EXCL_STOP
}

auto const sleOwner = view.peek(keylet::account(account));
if (!sleOwner)
return tecINTERNAL;
return tecINTERNAL; // LCOV_EXCL_LINE

auto const count =
sle->getFieldArray(sfPriceDataSeries).size() > 5 ? -2 : -1;
Expand All @@ -104,7 +108,7 @@ DeleteOracle::doApply()
keylet::oracle(account_, ctx_.tx[sfOracleDocumentID])))
return deleteOracle(ctx_.view(), sle, account_, j_);

return tecINTERNAL;
return tecINTERNAL; // LCOV_EXCL_LINE
}

} // namespace ripple
13 changes: 6 additions & 7 deletions src/ripple/app/tx/impl/SetOracle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ SetOracle::preclaim(PreclaimContext const& ctx)
auto const sleSetter =
ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount)));
if (!sleSetter)
return terNO_ACCOUNT;
return terNO_ACCOUNT; // LCOV_EXCL_LINE

// lastUpdateTime must be within maxLastUpdateTimeDelta seconds
// of the last closed ledger
Expand All @@ -88,8 +88,7 @@ SetOracle::preclaim(PreclaimContext const& ctx)
std::size_t const lastUpdateTimeEpoch =
lastUpdateTime - epoch_offset.count();
if (closeTime < maxLastUpdateTimeDelta)
Throw<std::runtime_error>(
"Oracle: close time is less than maxLastUpdateTimeDelta");
return tecINTERNAL; // LCOV_EXCL_LINE
if (lastUpdateTimeEpoch < (closeTime - maxLastUpdateTimeDelta) ||
lastUpdateTimeEpoch > (closeTime + maxLastUpdateTimeDelta))
return tecINVALID_UPDATE_TIME;
Expand Down Expand Up @@ -194,7 +193,7 @@ adjustOwnerCount(ApplyContext& ctx, int count)
return true;
}

return false;
return false; // LCOV_EXCL_LINE
}

static void
Expand Down Expand Up @@ -274,7 +273,7 @@ SetOracle::doApply()
auto const newCount = pairs.size() > 5 ? 2 : 1;
auto const adjust = newCount - oldCount;
if (adjust != 0 && !adjustOwnerCount(ctx_, adjust))
return tefINTERNAL;
return tefINTERNAL; // LCOV_EXCL_LINE

ctx_.view().update(sle);
}
Expand All @@ -295,13 +294,13 @@ SetOracle::doApply()
auto page = ctx_.view().dirInsert(
keylet::ownerDir(account_), sle->key(), describeOwnerDir(account_));
if (!page)
return tecDIR_FULL;
return tecDIR_FULL; // LCOV_EXCL_LINE

(*sle)[sfOwnerNode] = *page;

auto const count = series.size() > 5 ? 2 : 1;
if (!adjustOwnerCount(ctx_, count))
return tefINTERNAL;
return tefINTERNAL; // LCOV_EXCL_LINE

ctx_.view().insert(sle);
}
Expand Down
68 changes: 55 additions & 13 deletions src/ripple/rpc/handlers/GetAggregatePrice.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,18 @@ iteratePriceData(

auto const ledger = context.ledgerMaster.getLedgerBySeq(prevSeq);
if (!ledger)
return;
return; // LCOV_EXCL_LINE

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

prevChain = chain;
for (STObject const& node : meta->getFieldArray(sfAffectedNodes))
{
if (node.getFieldU16(sfLedgerEntryType) != ltORACLE)
{
continue;
}

prevChain = chain;
chain = &node;
isNew = node.isFieldPresent(sfNewFields);
// if a meta is for the new and this is the first
Expand Down Expand Up @@ -170,21 +170,49 @@ doGetAggregatePrice(RPC::JsonContext& context)
if (!params.isMember(jss::quote_asset))
return RPC::missing_field_error(jss::quote_asset);

// Lambda to validate uint type
// support positive int, uint, and a number represented as a string
auto validUInt = [](Json::Value const& params,
Json::StaticString const& field) {
auto const& jv = params[field];
std::uint32_t v;
return jv.isUInt() || (jv.isInt() && jv.asInt() >= 0) ||
(jv.isString() && beast::lexicalCastChecked(v, jv.asString()));
};

// Lambda to get `trim` and `time_threshold` fields. If the field
// is not included in the input then a default value is returned.
auto getField = [&params](
auto getField = [&params, &validUInt](
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))
return rpcINVALID_PARAMS;
return params[field].asUInt();
}
return def;
};

// Lambda to get `base_asset` and `quote_asset`. The values have
// to conform to the Currency type.
auto getCurrency =
[&params](SField const& sField, Json::StaticString const& field)
-> std::variant<Json::Value, error_code_i> {
try
{
if (params[field].asString().empty())
return rpcINVALID_PARAMS;
currencyFromJson(sField, params[field]);
return params[field];
}
catch (...)
{
return rpcINVALID_PARAMS;
}
};

auto const trim = getField(jss::trim);
if (std::holds_alternative<error_code_i>(trim))
{
Expand All @@ -206,8 +234,18 @@ doGetAggregatePrice(RPC::JsonContext& context)
return result;
}

auto const& baseAsset = params[jss::base_asset];
auto const& quoteAsset = params[jss::quote_asset];
auto const baseAsset = getCurrency(sfBaseAsset, jss::base_asset);
if (std::holds_alternative<error_code_i>(baseAsset))
{
RPC::inject_error(std::get<error_code_i>(baseAsset), result);
return result;
}
auto const quoteAsset = getCurrency(sfQuoteAsset, jss::quote_asset);
if (std::holds_alternative<error_code_i>(quoteAsset))
{
RPC::inject_error(std::get<error_code_i>(quoteAsset), result);
return result;
}

// Collect the dataset into bimap keyed by lastUpdateTime and
// STAmount (Number is int64 and price is uint64)
Expand All @@ -220,8 +258,7 @@ doGetAggregatePrice(RPC::JsonContext& context)
RPC::inject_error(rpcORACLE_MALFORMED, result);
return result;
}
auto const documentID = oracle[jss::oracle_document_id].isConvertibleTo(
Json::ValueType::uintValue)
auto const documentID = validUInt(oracle, jss::oracle_document_id)
? std::make_optional(oracle[jss::oracle_document_id].asUInt())
: std::nullopt;
auto const account =
Expand All @@ -235,7 +272,7 @@ doGetAggregatePrice(RPC::JsonContext& context)
std::shared_ptr<ReadView const> ledger;
result = RPC::lookupLedger(ledger, context);
if (!ledger)
return result;
return result; // LCOV_EXCL_LINE

auto const sle = ledger->read(keylet::oracle(*account, *documentID));
iteratePriceData(context, sle, [&](STObject const& node) {
Expand All @@ -246,9 +283,9 @@ doGetAggregatePrice(RPC::JsonContext& context)
series.end(),
[&](STObject const& o) -> bool {
return o.getFieldCurrency(sfBaseAsset).getText() ==
baseAsset &&
std::get<Json::Value>(baseAsset) &&
o.getFieldCurrency(sfQuoteAsset).getText() ==
quoteAsset &&
std::get<Json::Value>(quoteAsset) &&
o.isFieldPresent(sfAssetPrice);
});
iter != series.end())
Expand Down Expand Up @@ -287,10 +324,15 @@ doGetAggregatePrice(RPC::JsonContext& context)
prices.left.erase(
prices.left.upper_bound(upperBound), prices.left.end());

// At least one element should remain since upperBound is either
// equal to oldestTime or is less than latestTime, in which case
// the data is deleted between the oldestTime and upperBound.
if (prices.empty())
{
RPC::inject_error(rpcOBJECT_NOT_FOUND, result);
// LCOV_EXCL_START
RPC::inject_error(rpcINTERNAL, result);
return result;
// LCOV_EXCL_STOP
}
}
result[jss::time] = latestTime;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/handlers/LedgerEntry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ doLedgerEntry(RPC::JsonContext& context)
auto const& oracle = context.params[jss::oracle];
auto const documentID = [&]() -> std::optional<std::uint32_t> {
auto const& id = oracle[jss::oracle_document_id];
if (id.isConvertibleTo(Json::ValueType::uintValue))
if (id.isUInt() || (id.isInt() && id.asInt() >= 0))
return std::make_optional(id.asUInt());
else if (id.isString())
{
Expand Down
Loading

0 comments on commit f4da2e3

Please sign in to comment.