incorporate DeliverMax in Payment transactions #684

ckeshava · 2024-02-08T21:34:05Z

High Level Overview of Change

Add support for DeliverMax field in the Payment transactions. The following links will give you more context on the DeliverMax field:
XRPLF/rippled#4733, XRPLF/rippled#3484

Context of Change

Payment transactions accept a new field DeliverMax, which serves as an alias to the Amount field. This is intended to prevent misinterpretation of the response of Payment transactions and the consequent loss of funds.

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

Did you update CHANGELOG.md?

Yes
No, this change does not impact library users

Test Plan

Additional unit tests have been added to validate the usage of Payment transactions.

ckeshava · 2024-02-08T22:00:29Z

@mvadari I felt it is more appropriate to make the deliver max changes in the from_dict overload. The autofill and other utility functions work with a Transaction object. Since DeliverMax is not a part of protocol, it is not a recognized field in the Payment request.

mvadari · 2024-02-13T18:38:27Z

@mvadari I felt it is more appropriate to make the deliver max changes in the from_dict overload. The autofill and other utility functions work with a Transaction object. Since DeliverMax is not a part of protocol, it is not a recognized field in the Payment request.

This won't work in a lot of cases (including internal uses of from_dict). A lot of people use Transaction.from_dict instead of the specific transaction type.

ckeshava · 2024-02-20T23:56:55Z

@mvadari I agree that there are limitations in my current design.

If I were to make this change in autofill method, how can I go about it? autofill takes in a Transaction object and I cannot address any field titled deliver_max because its not a part of the Payment transaction model here:

xrpl-py/xrpl/models/transactions/payment.py

Line 62 in 2e1435c

class Payment(Transaction):

I'd like to test inputs such as

transaction_dict = {
            "account": _ACCOUNT,
            "fee": _FEE,
            "sequence": _SEQUENCE,
            "deliver_max": _ISSUED_CURRENCY_AMOUNT,
            "send_max": _XRP_AMOUNT,
            "destination": _DESTINATION,
        }
# convert the above JSON into a Payment object, without throwing any errors.

We are passing along Transaction object to methods like autofill, autofill_and_sign, sign_and_submit etc. Where can I get the opportunity to modify the JSON version of a Transaction?

mvadari · 2024-02-21T17:10:35Z

@mvadari I agree that there are limitations in my current design.

If I were to make this change in autofill method, how can I go about it? autofill takes in a Transaction object and I cannot address any field titled deliver_max because its not a part of the Payment transaction model here:

That's a fair point. In that case, perhaps from_xrpl is the right place to have this change - but it should be in Transaction.from_xrpl, not Payment.from_xrpl.

…d of Payment.from_dict

ckeshava · 2024-02-21T21:35:24Z

I have moved the changes to Transaction.from_dict from the Payment class. from_xrpl invokes from_dict in the course of its actions. I felt the former is most appropriate to accomodate these changes.

mvadari · 2024-02-21T21:57:28Z

I felt the former is most appropriate to accomodate these changes.

I disagree, because it's only ever going to be necessary when pulling from XRPL data, not from any other dictionary.

…function

xrpl/models/base_model.py

mvadari · 2024-02-23T22:41:34Z

xrpl/models/transactions/transaction.py

@@ -468,3 +472,42 @@ def from_blob(tx_blob: str) -> Transaction:
            The formatted transaction.
        """
        return Transaction.from_xrpl(decode(tx_blob))
+
+    @classmethod
+    def from_xrpl(cls: Type[T], inp_params: Union[str, Dict[str, Any]]) -> T:


Suggested change

def from_xrpl(cls: Type[T], inp_params: Union[str, Dict[str, Any]]) -> T:

def from_xrpl(cls: Type[T], value: Union[str, Dict[str, Any]]) -> T:

This should use the same formatting as the function it's overriding.

xrpl/models/transactions/transaction.py

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

ckeshava · 2024-02-25T05:52:43Z

I've addressed your comments @mvadari 👍

xrpl/models/transactions/transaction.py

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

ckeshava · 2024-03-18T21:53:23Z

@mvadari does this design look okay? Can I do the same for JS too?

justinr1234

nit changes only

justinr1234 · 2024-03-20T18:41:20Z

xrpl/models/base_model.py

@@ -73,6 +73,28 @@ def _value_to_json(value: XRPL_VALUE_TYPE) -> XRPL_VALUE_TYPE:
    return value


+def process_xrpl_json(value: Union[str, Dict[str, Any]]) -> Dict[str, Any]:


Seems like this function can return exceptions which we should note and/or handle

I don't know which exceptions might be thrown by this code. _key_to_json and _value_to_json do not throw any exceptions themselves

Do you suspect that json.load could throw a json.JSONDecodeError exception?

This function will never be directly used by end users.

@mvadari Yes, I agree. Do you mean to say we don't have to document all possible exceptions?

justinr1234 · 2024-03-20T18:41:51Z

xrpl/models/transactions/transaction.py

+            XRPLModelException: If Payment transactions have different values for
+                                amount and deliver_max fields
+        """
+        processed_value = process_xrpl_json(value)


This may not return successfully so we should likely note that in the docs

xrpl/models/transactions/transaction.py

justinr1234 · 2024-03-20T18:43:40Z

xrpl/models/transactions/transaction.py

+                if "amount" in processed_value:
+                    if processed_value["amount"] != processed_value["deliver_max"]:
+                        raise XRPLModelException(
+                            "Error: Amount and DeliverMax fields are not identical"


Is there maybe a link we can give to explain why they should be equal in the error message?

Hmm, I can point to the issue discussion here: XRPLF/rippled#3484

Here is an implementation commit to the rippled codebase: XRPLF/rippled@3972683

I'm not aware of any other documents summarizing this issue.

DeliverMax is an alias for the Amount field. So if they contain different values, it introduces semantic ambiguity. Which one should we use? Both of them refer to the same logical quantity -- the value of transferred money

If both fields are passed in, then they have to be equal. We can update the error message for clarity: "Error: Amount and DeliverMax fields must be equal if both are provided"

Don't link rippled issues from the libraries, point to the xrpl.org documentation. It's not useful to do so in error messages, because that makes the error message really long, but it can be in the docstring.

I was brainstorming ideas for resource pages for this issue, I won't include it in the error message 👍

I couldn't find a statement that says "If present, DeliverMax field must equal Amount" in the Payments docs. https://xrpl.org/docs/references/protocol/transactions/types/payment#payment-fields

I feel it's a better developer experience to provide an elaborate error message, instead of making them search through a website.

DeliverMax is only an RPC field, rippled doesn't care about it. It's only ever returned. That documentation should probably elaborate on that a bit.

Yes, documentation will help. DeliverMax can also be used as an input in APIv2 rippled.

Every instance of DeliverMax, at the protocol (or) binary-codec-level, is translated into Amount.

No, it was not changed at the protocol level, just at the RPC level.

… booleans

xrpl/models/transactions/transaction.py

Co-authored-by: Omar Khan <khancodegt@gmail.com>

mvadari · 2024-03-22T15:04:02Z

xrpl/models/base_model.py

@@ -73,6 +73,28 @@ def _value_to_json(value: XRPL_VALUE_TYPE) -> XRPL_VALUE_TYPE:
    return value


+def process_xrpl_json(value: Union[str, Dict[str, Any]]) -> Dict[str, Any]:


I'd recommend making this a helper class method, instead of a separate function - it saves you some imports. Similar to _get_only_init_args.

okay, I have done this. But I dislike the fact that the self/cls argument is not useful inside the process_xrpl_json function. I'm only using it to hook into this function

I think the tradeoff is worth it given that it makes it easier to use, but if someone strongly objects I'll withdraw

yeah, I think it's a reasonable tradeoff 👍

…ent transactions

ckeshava · 2024-04-22T21:37:05Z

@khancode can you please take a look at this PR? does this design look alright?

@justinr1234 I need some clarification on your prior comments. Let me know if I need to make any more changes.

thnks

khancode

LGTM, just need to change a method to be private.

xrpl/models/base_model.py

ckeshava · 2024-04-25T21:00:23Z

the snippets tests are failing due to an unfunded account (We believe it lacks "USD" tokens). It will need to be addressed in a separate PR

ckeshava · 2024-06-04T21:45:52Z

If there are no objections, I'd like to merge this PR.

Thank you all for your feedback

ckeshava added 2 commits February 8, 2024 13:28

incorporate DeliverMax in Payment transactions

d83c94c

update CHANGELOG. fix the incorrect comments on Payment.from_dict()

11279cc

ckeshava requested a review from khancode February 8, 2024 22:00

Merge branch 'main' into dMax

e990232

khancode requested a review from mvadari February 12, 2024 17:14

Merge branch 'main' of https://github.com/XRPLF/xrpl-py into dMax

eeafc40

Merge branch 'dMax' of https://github.com/XRPLF/xrpl-py into dMax

f713c07

refactor: Transaction.from_dict handles the deliver_max alias, instea…

6637014

…d of Payment.from_dict

revert changes to the Payment class

e43c974

ckeshava added 2 commits February 21, 2024 15:38

move the deliver_max alias logic into Transaction.from_xrpl

1926c42

additional comments to explain the process_json_binary_codec utility …

7de639c

…function

mvadari reviewed Feb 23, 2024

View reviewed changes

xrpl/models/base_model.py Outdated Show resolved Hide resolved

mvadari reviewed Feb 23, 2024

View reviewed changes

xrpl/models/base_model.py Outdated Show resolved Hide resolved

mvadari reviewed Feb 23, 2024

View reviewed changes

xrpl/models/base_model.py Outdated Show resolved Hide resolved

mvadari reviewed Feb 23, 2024

View reviewed changes

xrpl/models/transactions/transaction.py Outdated Show resolved Hide resolved

ckeshava and others added 5 commits February 24, 2024 21:41

Update xrpl/models/base_model.py

56b8f32

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

Update xrpl/models/base_model.py

c47fac1

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

Update xrpl/models/base_model.py

d47260c

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

Update xrpl/models/transactions/transaction.py

3f417a0

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

addresing PR comments

b8bd5a2

mvadari reviewed Feb 26, 2024

View reviewed changes

xrpl/models/transactions/transaction.py Outdated Show resolved Hide resolved

Update xrpl/models/transactions/transaction.py

211e378

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

khancode approved these changes Mar 5, 2024

View reviewed changes

justinr1234 approved these changes Mar 20, 2024

View reviewed changes

ckeshava added 3 commits March 20, 2024 14:49

Merge branch 'main' of https://github.com/XRPLF/xrpl-py into dMax

79fd8fd

address Justin's PR comments: refactor nested if conditions using and…

e3d0b7b

… booleans

Merge branch 'dMax' of https://github.com/XRPLF/xrpl-py into dMax

59c77d7

ckeshava requested a review from justinr1234 March 20, 2024 22:03

khancode requested changes Mar 21, 2024

View reviewed changes

xrpl/models/transactions/transaction.py Outdated Show resolved Hide resolved

Update xrpl/models/transactions/transaction.py

051e13e

Co-authored-by: Omar Khan <khancodegt@gmail.com>

ckeshava requested a review from khancode March 22, 2024 00:08

mvadari reviewed Mar 22, 2024

View reviewed changes

ckeshava added 2 commits March 22, 2024 10:14

refactor process_xrpl_json into a classmethod

f5fae35

Merge branch 'dMax' of https://github.com/XRPLF/xrpl-py into dMax

47c6b07

ckeshava requested a review from mvadari March 22, 2024 17:16

integration tests: verify the deliver_max/amount alias fields in Paym…

052ba00

…ent transactions

ckeshava mentioned this pull request Apr 23, 2024

introduce DeliverMax alias in Payment transactions XRPLF/xrpl.js#2684

Closed

9 tasks

khancode approved these changes Apr 25, 2024

View reviewed changes

xrpl/models/base_model.py Outdated Show resolved Hide resolved

ckeshava and others added 2 commits April 25, 2024 13:48

address Omar's comments. update process_xrpl_json into a private method

57aa59a

Merge branch 'main' into dMax

eeaf241

Merge branch 'main' into dMax

6af9177

ckeshava merged commit 8f7472a into main Jun 4, 2024
21 checks passed

ckeshava deleted the dMax branch June 4, 2024 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

incorporate DeliverMax in Payment transactions #684

incorporate DeliverMax in Payment transactions #684

ckeshava commented Feb 8, 2024

ckeshava commented Feb 8, 2024

mvadari commented Feb 13, 2024

ckeshava commented Feb 20, 2024

mvadari commented Feb 21, 2024

ckeshava commented Feb 21, 2024

mvadari commented Feb 21, 2024

mvadari Feb 23, 2024

ckeshava commented Feb 25, 2024

ckeshava commented Mar 18, 2024

justinr1234 left a comment

justinr1234 Mar 20, 2024

ckeshava Mar 20, 2024

ckeshava Mar 20, 2024

mvadari Mar 22, 2024

ckeshava Mar 22, 2024

justinr1234 Mar 20, 2024

justinr1234 Mar 20, 2024

ckeshava Mar 20, 2024

khancode Mar 21, 2024

mvadari Mar 22, 2024

ckeshava Mar 22, 2024

mvadari Mar 22, 2024

ckeshava Mar 22, 2024

mvadari Mar 22, 2024

mvadari Mar 22, 2024

ckeshava Mar 22, 2024

mvadari Mar 22, 2024

ckeshava Mar 22, 2024

ckeshava commented Apr 22, 2024

khancode left a comment

ckeshava commented Apr 25, 2024

ckeshava commented Jun 4, 2024

	def from_xrpl(cls: Type[T], inp_params: Union[str, Dict[str, Any]]) -> T:
	def from_xrpl(cls: Type[T], value: Union[str, Dict[str, Any]]) -> T:

		@@ -73,6 +73,28 @@ def _value_to_json(value: XRPL_VALUE_TYPE) -> XRPL_VALUE_TYPE:
		return value


		def process_xrpl_json(value: Union[str, Dict[str, Any]]) -> Dict[str, Any]:

incorporate DeliverMax in Payment transactions #684

incorporate DeliverMax in Payment transactions #684

Conversation

ckeshava commented Feb 8, 2024

High Level Overview of Change

Context of Change

Type of Change

Did you update CHANGELOG.md?

Test Plan

ckeshava commented Feb 8, 2024

mvadari commented Feb 13, 2024

ckeshava commented Feb 20, 2024

mvadari commented Feb 21, 2024

ckeshava commented Feb 21, 2024

mvadari commented Feb 21, 2024

Choose a reason for hiding this comment

ckeshava commented Feb 25, 2024

ckeshava commented Mar 18, 2024

justinr1234 left a 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

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

ckeshava commented Apr 22, 2024

khancode left a comment

Choose a reason for hiding this comment

ckeshava commented Apr 25, 2024

ckeshava commented Jun 4, 2024