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

Bronek · 2023-09-28T19:28:48Z

Add `DeliverMax` alias to `Amount`, remove `Amount` from API ver 2.

Context of Change

Resolve #3484

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)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Bronek · 2023-10-04T14:53:44Z

Scope of the change.

I want to:

add handling of DeliverMax as an alias to Amount for all API operations taking tx_json on input. The operations I've identified are:

sign (obsolete, but could be used)
submit (in sign-and-submit mode, see above)
submit_multisigned
sign_for

add DeliverMax (and optionally remove Amount for API ver. 2) on output of all API operations that may publish transactions. The operations I've identified are:

ledger
tx
tx_history
account_tx
transaction_entry

add DeliverMax to published transactions stream https://xrpl.org/subscribe.html#transaction-streams (and optionally remove Amount for API ver. 2) for API operation

subscribe

For clarity, the following API operations do not publish transactions, hence are unaffected:

ledger_entry (only show objects stored on the ledger https://xrpl.org/ledger_entry.html , not transactions)
ledger_data (similar to ledger_entry)
account_objects (similar to ledger_data)
ledger_request (only shows ledger info/state)
ledger_closed (only shows ledger info/state)
ledger_current (only shows ledger info/state)
path_find (only publishes paths, not transactions)

In all cases where API takes tx_json on input, or shows it on output, the relevant transaction type where the change applies is:

payment (and that's all)

Bronek · 2023-10-05T13:55:36Z

I realised the old version of this draft was updating meta json element (which is where delivered_amount is), but it should instead update transaction json element (not necessarily under this name - we are unfortunately inconsistent), because that's where Amount is. New version of this PR fixes this.

Bronek · 2023-10-05T16:08:28Z

Also found that I missed proposed transactions and proposed account transaction, this is fixed now. I am considering some refactoring of my changes in NetworkOPs.cpp to improve cohesion and reduce coupling - i.e. the way I added virtually identical non-trivial block of code in several functions, while missing proposed transactions. So that will be probably next commit and then of course badly needed unit tests.

Bronek · 2023-10-06T18:28:33Z

Note, as seen in Add test for TransactionSign.cpp commit, using DeliverMax instead of Amount does not affect the transaction signature: both tests "Invalid Amount in submit_multisigned Payment." and "Invalid DeliverMax in submit_multisigned Payment." have the same signature. Also the error in the latter test refers to Amount and not DeliverMax field. This is consequence of the fact that DeliverMax is an alias to Amount.

ximinez

Just a couple of minor things.

BTW, please stop force pushing. I'd rather have 20 tweak commits than have to figure out what changed since the last time I looked.

src/ripple/app/misc/NetworkOPs.cpp

src/ripple/app/misc/DeliverMax.h

src/ripple/json/MultivarJson.h

Bronek · 2023-10-23T15:13:26Z

@ximinez thanks for very generous input - I addressed all your comments. I added one more test to 'MultivarJson_test.cpp` which required an extra line for levelization, but I think that should be OK.

arihantkothari · 2023-10-23T15:18:04Z

src/ripple/app/misc/NetworkOPs.cpp

+    static_assert(
+        apiVersionSelector(RPC::apiBetaVersion)() + 1  //
+        == MultiApiJson::size);
+    for (unsigned apiVersion = RPC::apiMinimumSupportedVersion,


For the intentions of looping over the apiVersions from minimumSupported to beta. I actually made a helper function long time back - may/ may not be useful :) [Now I feel, that the helper can be improved and is too strict. But I'll leave as it is.]
#4611

That function is cool, but as written, it's only for tests.

oh shoot, my bad - sorry about that. I did not notice this was not for tests 😆

ximinez · 2023-10-23T17:09:05Z

Builds/levelization/results/ordering.txt

@@ -135,6 +135,7 @@ test.csf > ripple.protocol
 test.csf > test.jtx
 test.json > ripple.beast
 test.json > ripple.json
+test.json > ripple.rpc


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.

intelliot · 2023-10-23T17:38:24Z

Suggested commit message:

APIv2(DeliverMax): add alias for Amount in Payment transactions (#4733)

Using the "Amount" field in Payment transactions can cause incorrect
interpretation. There continue to be problems from the use of this
field. "Amount" is rarely the correct field to use; instead,
"delivered_amount" (or "DeliveredAmount") should be used.

Rename the "Amount" field to "DeliverMax", a less misleading name. With
api_version: 2, remove the "Amount" field from Payment transactions.

- Input: "DeliverMax" in `tx_json` is an alias for "Amount"
  - sign
  - submit (in sign-and-submit mode)
  - submit_multisigned
  - sign_for
- Output: Add "DeliverMax" where transactions are provided by the API
  - ledger
  - tx
  - tx_history
  - account_tx
  - transaction_entry
  - subscribe (transactions stream)
- Output: Remove "Amount" from API version 2

Fix #3484

Fix #3902

Bronek · 2023-10-23T18:06:44Z

This is ready for merge

zgrguric · 2024-01-10T07:41:38Z

Is "Amount" removed from output on Payment transactions?

ximinez · 2024-01-10T19:06:07Z

Is "Amount" removed from output on Payment transactions?

Only in API v2.

…F#4733) Using the "Amount" field in Payment transactions can cause incorrect interpretation. There continue to be problems from the use of this field. "Amount" is rarely the correct field to use; instead, "delivered_amount" (or "DeliveredAmount") should be used. Rename the "Amount" field to "DeliverMax", a less misleading name. With api_version: 2, remove the "Amount" field from Payment transactions. - Input: "DeliverMax" in `tx_json` is an alias for "Amount" - sign - submit (in sign-and-submit mode) - submit_multisigned - sign_for - Output: Add "DeliverMax" where transactions are provided by the API - ledger - tx - tx_history - account_tx - transaction_entry - subscribe (transactions stream) - Output: Remove "Amount" from API version 2 Fix XRPLF#3484 Fix XRPLF#3902

Bronek changed the title ~~Feature/partial payment fieldname~~ Add DeliverMax alias to Amount, remove Amount from API ver 2. Sep 28, 2023

Bronek changed the title ~~Add DeliverMax alias to Amount, remove Amount from API ver 2.~~ WIP: Add DeliverMax alias to Amount, remove Amount from API ver 2. Sep 28, 2023

Bronek changed the title ~~WIP: Add DeliverMax alias to Amount, remove Amount from API ver 2.~~ [DRAFT] Add DeliverMax alias to Amount, remove Amount from API ver 2. Sep 28, 2023

Bronek force-pushed the feature/partial-payment-fieldname branch 2 times, most recently from b7f9bc1 to f7355e3 Compare October 2, 2023 12:04

intelliot requested review from HowardHinnant and ximinez October 2, 2023 15:04

Bronek force-pushed the feature/partial-payment-fieldname branch 2 times, most recently from 836dd87 to 1cbc61e Compare October 2, 2023 18:51

intelliot added the API Change label Oct 3, 2023

intelliot mentioned this pull request Oct 3, 2023

Add DeliverMax field in API responses; hide/remove Amount field for api_version 2 XRPLF/clio#883

Closed

intelliot added the Clio Reviewed label Oct 3, 2023

Bronek force-pushed the feature/partial-payment-fieldname branch from 1cbc61e to 4b3c895 Compare October 3, 2023 19:37

Bronek mentioned this pull request Oct 4, 2023

Rename Amount Field to DeliverMax (to prevent incorrect handling of Partial Payments) #3484

Closed

Bronek force-pushed the feature/partial-payment-fieldname branch 4 times, most recently from dd9a258 to c4f997a Compare October 5, 2023 13:54

Bronek force-pushed the feature/partial-payment-fieldname branch from c4f997a to 4d9a594 Compare October 5, 2023 16:05

Bronek force-pushed the feature/partial-payment-fieldname branch 7 times, most recently from 1e0a5f3 to 17e39bb Compare October 6, 2023 16:33

Add assert in MultivarJson::select

cf0798a

Bronek force-pushed the feature/partial-payment-fieldname branch from a738132 to cf0798a Compare October 20, 2023 21:07

ximinez requested changes Oct 20, 2023

View reviewed changes

src/ripple/app/misc/NetworkOPs.cpp Outdated Show resolved Hide resolved

src/ripple/app/misc/DeliverMax.h Outdated Show resolved Hide resolved

src/ripple/json/MultivarJson.h Outdated Show resolved Hide resolved

intelliot assigned Bronek and unassigned ximinez and arihantkothari Oct 23, 2023

Bronek added 3 commits October 23, 2023 12:51

Minor improvement in unit tests

efbf39a

Minor cleanup in include guards

70bd1b7

Improve version loop and static asserts

5e7fa7c

Bronek force-pushed the feature/partial-payment-fieldname branch from 1cb93ec to 5e7fa7c Compare October 23, 2023 14:38

arihantkothari reviewed Oct 23, 2023

View reviewed changes

intelliot requested a review from ximinez October 23, 2023 15:21

intelliot assigned ximinez Oct 23, 2023

Bronek added a commit to Bronek/rippled that referenced this pull request Oct 23, 2023

Squashed XRPLF#4733

ed273af

ximinez approved these changes Oct 23, 2023

View reviewed changes

Merge branch 'develop' into feature/partial-payment-fieldname

f4bfe95

intelliot changed the title ~~Add DeliverMax alias to Amount, remove Amount from API ver 2.~~ APIv2(DeliverMax): add alias for Amount in Payment transactions Oct 23, 2023

intelliot merged commit 3972683 into XRPLF:develop Oct 23, 2023
16 checks passed

intelliot added the Testable label Oct 23, 2023

intelliot mentioned this pull request Oct 23, 2023

Proposed 2.0.0-b4 #4782

Merged

1 task

intelliot changed the title ~~APIv2(DeliverMax): add alias for Amount in Payment transactions~~ Add DeliverMax (alias for Amount) in Payment transactions Oct 24, 2023

Bronek mentioned this pull request Nov 21, 2023

Show DeliverMax in submit_multisigned etc if API v2 selected #4827

Merged

12 tasks

Bronek mentioned this pull request Jan 31, 2024

Write proper forAllApiVersions used in NetworkOPs.cpp #4833

Merged

8 tasks

ckeshava mentioned this pull request Feb 8, 2024

incorporate DeliverMax in Payment transactions XRPLF/xrpl-py#684

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Bronek commented Sep 28, 2023

Bronek commented Oct 4, 2023 •

edited

Loading

Bronek commented Oct 5, 2023

Bronek commented Oct 5, 2023

Bronek commented Oct 6, 2023

ximinez left a comment

Bronek commented Oct 23, 2023

arihantkothari Oct 23, 2023 •

edited

Loading

ximinez Oct 23, 2023

arihantkothari Oct 23, 2023 •

edited

Loading

ximinez Oct 23, 2023

ximinez Oct 23, 2023

intelliot commented Oct 23, 2023

Bronek commented Oct 23, 2023

zgrguric commented Jan 10, 2024

ximinez commented Jan 10, 2024

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

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

Conversation

Bronek commented Sep 28, 2023

Add DeliverMax alias to Amount, remove Amount from API ver 2.

Context of Change

Type of Change

Bronek commented Oct 4, 2023 • edited Loading

Scope of the change.

Bronek commented Oct 5, 2023

Bronek commented Oct 5, 2023

Bronek commented Oct 6, 2023

ximinez left a comment

Choose a reason for hiding this comment

Bronek commented Oct 23, 2023

arihantkothari Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

ximinez Oct 23, 2023

Choose a reason for hiding this comment

arihantkothari Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

ximinez Oct 23, 2023

Choose a reason for hiding this comment

ximinez Oct 23, 2023

Choose a reason for hiding this comment

intelliot commented Oct 23, 2023

Bronek commented Oct 23, 2023

zgrguric commented Jan 10, 2024

ximinez commented Jan 10, 2024

Add `DeliverMax` alias to `Amount`, remove `Amount` from API ver 2.

Bronek commented Oct 4, 2023 •

edited

Loading

arihantkothari Oct 23, 2023 •

edited

Loading

arihantkothari Oct 23, 2023 •

edited

Loading