Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DeliverMax (alias for Amount) in Payment transactions #4733

Merged
merged 26 commits into from
Oct 23, 2023
Merged

Add DeliverMax (alias for Amount) in Payment transactions #4733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a4adde5
Add DeliverMax to submit, sign, tx etc.
Bronek Sep 26, 2023
7d79f89
Add DeliverMax to subscribe
Bronek Sep 28, 2023
c9f7907
Factor out MultivarJson for transaction streams
Bronek Sep 28, 2023
775731b
Refactor transJson to remove repeated code
Bronek Oct 5, 2023
2959308
Add unit test for MultivarJson
Bronek Oct 6, 2023
ef6cbc0
Add test for TransactionSign.cpp
Bronek Oct 6, 2023
2ec189b
Add DeliverMax to transaction_entry tests
Bronek Oct 9, 2023
aa9f0a9
Add DeliverMax to subscribe tests
Bronek Oct 9, 2023
e35bf62
Allow both Amount and DeliverMax if identical
Bronek Oct 9, 2023
03edc89
Add DeliverMax to tx tests
Bronek Oct 10, 2023
9f4206a
Add DeliverMax to account_tx test
Bronek Oct 10, 2023
432fd16
Change apiVersion to unsigned
Bronek Oct 10, 2023
03713c1
Merge branch 'develop' into feature/partial-payment-fieldname
Bronek Oct 11, 2023
c99959e
Update API-CHANGELOG.md
Bronek Oct 11, 2023
d1a2c59
Merge branch 'develop' into feature/partial-payment-fieldname
Bronek Oct 16, 2023
44771d1
Merge branch 'develop' into feature/partial-payment-fieldname
Bronek Oct 18, 2023
48697e8
Minor fixes
Bronek Oct 19, 2023
ff41b90
Add MultivarJson::isMember and small fixes
Bronek Oct 19, 2023
5b30a25
Fill MultiApiJson in NetworkOPsImp::transJson
Bronek Oct 19, 2023
198e7cd
Merge branch 'develop' into feature/partial-payment-fieldname
Bronek Oct 20, 2023
bd53535
Minor test improvement
Bronek Oct 20, 2023
cf0798a
Add assert in MultivarJson::select
Bronek Oct 20, 2023
efbf39a
Minor improvement in unit tests
Bronek Oct 23, 2023
70bd1b7
Minor cleanup in include guards
Bronek Oct 23, 2023
5e7fa7c
Improve version loop and static asserts
Bronek Oct 23, 2023
f4bfe95
Merge branch 'develop' into feature/partial-payment-fieldname
intelliot Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions API-CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,15 @@ Since `api_version` 2 is in "beta", breaking changes to V2 can currently be made

- Attempting to use a non-boolean value (such as a string) for the `transactions` parameter returns `invalidParams` (`rpcINVALID_PARAMS`). Previously, in API version 1, no error was returned. (https://github.com/XRPLF/rippled/pull/4620)

##### In progress

- In `Payment` transaction type, JSON RPC field `Amount` is renamed to `DeliverMax`. To enable smooth client transition, `Amount` is still handled, as described below:
- On JSON RPC input (e.g. `submit_multisigned` etc. methods), `Amount` is recognized as an alias to `DeliverMax` for both API version 1 and version 2 clients.
- On JSON RPC input, submitting both `Amount` and `DeliverMax` fields is allowed _only_ if they are identical; otherwise such input is rejected with `rpcINVALID_PARAMS` error.
- On JSON RPC output (e.g. `subscribe`, `account_tx` etc. methods), `DeliverMax` is present in both API version 1 and version 2.
- On JSON RPC output, `Amount` is only present in API version 1 and _not_ in version 2.
Comment on lines +163 to +167
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON-RPC should have a hyphen, although I think the changes should apply to both WebSocket and JSON-RPC (and likely do, based on what I see in the code here). Frequently people use the term "RPC" to encompass both the JSON-RPC and WebSocket APIs, although I tend to avoid such usage because it can cause such confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we doing JSON-RPC over WebSocket? For HTTP JSON-RPC, maybe we should say "HTTP" 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kind-of like to use both parts, because they are important to set the context

  • "JSON" because we are talking about JSON payload of a command and its response, because that's where the field names appear
  • "RPC" because that's directly related to "JSON-RPC" as @mDuo13 correctly points out, e.g. as seen here https://xrpl.org/account_tx.html
  • There is an exception though - https://xrpl.org/subscribe.html which is only a websocket method, and yet it produces an "JSON RPC"-like stream, which is also impacted by this change. So not strictly JSON-RPC



# Unit tests for API changes

The following information is useful to developers contributing to this project:
Expand Down
3 changes: 3 additions & 0 deletions Builds/CMake/RippledCore.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ install (
install (
FILES
src/ripple/json/JsonPropertyStream.h
src/ripple/json/MultivarJson.h
src/ripple/json/Object.h
src/ripple/json/Output.h
src/ripple/json/Writer.h
Expand Down Expand Up @@ -467,6 +468,7 @@ target_sources (rippled PRIVATE
src/ripple/app/misc/detail/impl/WorkSSL.cpp
src/ripple/app/misc/impl/AccountTxPaging.cpp
src/ripple/app/misc/impl/AmendmentTable.cpp
src/ripple/app/misc/impl/DeliverMax.cpp
src/ripple/app/misc/impl/LoadFeeTrack.cpp
src/ripple/app/misc/impl/Manifest.cpp
src/ripple/app/misc/impl/Transaction.cpp
Expand Down Expand Up @@ -918,6 +920,7 @@ if (tests)
src/test/json/Output_test.cpp
src/test/json/Writer_test.cpp
src/test/json/json_value_test.cpp
src/test/json/MultivarJson_test.cpp
#[===============================[
test sources:
subdir: jtx
Expand Down
1 change: 1 addition & 0 deletions Builds/levelization/results/ordering.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ test.csf > ripple.protocol
test.csf > test.jtx
test.json > ripple.beast
test.json > ripple.json
test.json > ripple.rpc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding lines to the ordering file is great, as long as it follows the existing rules (which this does). It would be a problem if the line was added to loops.txt, or if the dependency didn't "fit" the existing order.

test.json > test.jtx
test.jtx > ripple.app
test.jtx > ripple.basics
Expand Down
5 changes: 3 additions & 2 deletions src/ripple/app/ledger/BookListeners.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ BookListeners::removeSubscriber(std::uint64_t seq)

void
BookListeners::publish(
Json::Value const& jvObj,
MultiApiJson const& jvObj,
hash_set<std::uint64_t>& havePublished)
{
std::lock_guard sl(mLock);
Expand All @@ -54,7 +54,8 @@ BookListeners::publish(
// Only publish jvObj if this is the first occurence
if (havePublished.emplace(p->getSeq()).second)
{
p->send(jvObj, true);
p->send(
jvObj.select(apiVersionSelector(p->getApiVersion())), true);
}
++it;
}
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/app/ledger/BookListeners.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
#ifndef RIPPLE_APP_LEDGER_BOOKLISTENERS_H_INCLUDED
#define RIPPLE_APP_LEDGER_BOOKLISTENERS_H_INCLUDED

#include <ripple/json/MultivarJson.h>
#include <ripple/net/InfoSub.h>

#include <memory>
#include <mutex>

Expand Down Expand Up @@ -58,7 +60,7 @@ class BookListeners

*/
void
publish(Json::Value const& jvObj, hash_set<std::uint64_t>& havePublished);
publish(MultiApiJson const& jvObj, hash_set<std::uint64_t>& havePublished);

private:
std::recursive_mutex mLock;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/ledger/OrderBookDB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ void
OrderBookDB::processTxn(
std::shared_ptr<ReadView const> const& ledger,
const AcceptedLedgerTx& alTx,
Json::Value const& jvObj)
MultiApiJson const& jvObj)
{
std::lock_guard sl(mLock);

Expand Down
4 changes: 3 additions & 1 deletion src/ripple/app/ledger/OrderBookDB.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include <ripple/app/ledger/AcceptedLedgerTx.h>
#include <ripple/app/ledger/BookListeners.h>
#include <ripple/app/main/Application.h>
#include <ripple/json/MultivarJson.h>

#include <mutex>

namespace ripple {
Expand Down Expand Up @@ -63,7 +65,7 @@ class OrderBookDB
processTxn(
std::shared_ptr<ReadView const> const& ledger,
const AcceptedLedgerTx& alTx,
Json::Value const& jvObj);
MultiApiJson const& jvObj);

private:
Application& app_;
Expand Down
2 changes: 2 additions & 0 deletions src/ripple/app/ledger/impl/LedgerToJson.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <ripple/app/ledger/LedgerToJson.h>
#include <ripple/app/main/Application.h>
#include <ripple/app/misc/DeliverMax.h>
#include <ripple/app/misc/TxQ.h>
#include <ripple/basics/base_uint.h>
#include <ripple/core/Pg.h>
Expand Down Expand Up @@ -123,6 +124,7 @@ fillJsonTx(
else
{
copyFrom(txJson, txn->getJson(JsonOptions::none));
RPC::insertDeliverMax(txJson, txnType, fill.context->apiVersion);
if (stMeta)
{
txJson[jss::metaData] = stMeta->getJson(JsonOptions::none);
Expand Down
53 changes: 53 additions & 0 deletions src/ripple/app/misc/DeliverMax.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//------------------------------------------------------------------------------
/*
This file is part of rippled: https://github.com/ripple/rippled
Copyright (c) 2023 Ripple Labs Inc.

Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.

THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
//==============================================================================

#ifndef RIPPLE_APP_MISC_DELIVERMAX_H_INCLUDED
#define RIPPLE_APP_MISC_DELIVERMAX_H_INCLUDED

#include <ripple/protocol/TxFormats.h>

#include <functional>
#include <memory>

namespace Json {
class Value;
}

namespace ripple {

namespace RPC {

/**
Copy `Amount` field to `DeliverMax` field in transaction output JSON.
This only applies to Payment transaction type, all others are ignored.

When apiVersion > 1 will also remove `Amount` field, forcing users
to access this value using new `DeliverMax` field only.
@{
*/

void
insertDeliverMax(Json::Value& tx_json, TxType txnType, unsigned int apiVersion);

/** @} */

} // namespace RPC
} // namespace ripple

#endif
Loading
Loading