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 15 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 @@ -149,6 +149,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
2 changes: 2 additions & 0 deletions Builds/CMake/RippledCore.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,7 @@ target_sources (rippled PRIVATE
src/ripple/rpc/handlers/Validators.cpp
src/ripple/rpc/handlers/WalletPropose.cpp
src/ripple/rpc/impl/DeliveredAmount.cpp
src/ripple/rpc/impl/DeliverMax.cpp
src/ripple/rpc/impl/Handler.cpp
src/ripple/rpc/impl/LegacyPathFind.cpp
src/ripple/rpc/impl/RPCHandler.cpp
Expand Down Expand Up @@ -854,6 +855,7 @@ if (tests)
src/test/basics/join_test.cpp
src/test/basics/mulDiv_test.cpp
src/test/basics/tagged_integer_test.cpp
src/test/basics/MultivarJson_test.cpp
ximinez marked this conversation as resolved.
Show resolved Hide resolved
#[===============================[
test sources:
subdir: beast
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/basics/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/basics/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 @@ -23,6 +23,7 @@
#include <ripple/basics/base_uint.h>
#include <ripple/core/Pg.h>
#include <ripple/rpc/Context.h>
#include <ripple/rpc/DeliverMax.h>
ximinez marked this conversation as resolved.
Show resolved Hide resolved
#include <ripple/rpc/DeliveredAmount.h>

namespace ripple {
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
Loading
Loading