incorporate DeliverMax in Payment transactions

ckeshava commented 7 months ago

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: https://github.com/XRPLF/rippled/pull/4733, https://github.com/XRPLF/rippled/issues/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)
[x] 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)
[x] 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?

[x] 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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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: https://github.com/XRPLF/xrpl-py/blob/2e1435c37c3f2e1a3b41ac21f02f1a692258cee0/xrpl/models/transactions/payment.py#L62

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 commented 7 months ago

@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.

ckeshava commented 7 months ago

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 commented 7 months ago

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.

ckeshava commented 7 months ago

I've addressed your comments @mvadari 👍

ckeshava commented 6 months ago

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

ckeshava commented 5 months ago

@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

ckeshava commented 5 months ago

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 commented 4 months ago

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

Thank you all for your feedback

XRPLF / xrpl-py