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.

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

incorporate DeliverMax in Payment transactions #684

Merged
merged 25 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d83c94c
incorporate DeliverMax in Payment transactions
ckeshava Feb 8, 2024
11279cc
update CHANGELOG. fix the incorrect comments on Payment.from_dict()
ckeshava Feb 8, 2024
e990232
Merge branch 'main' into dMax
ckeshava Feb 8, 2024
eeafc40
Merge branch 'main' of https://github.com/XRPLF/xrpl-py into dMax
ckeshava Feb 20, 2024
f713c07
Merge branch 'dMax' of https://github.com/XRPLF/xrpl-py into dMax
ckeshava Feb 20, 2024
6637014
refactor: Transaction.from_dict handles the deliver_max alias, instea…
ckeshava Feb 21, 2024
e43c974
revert changes to the Payment class
ckeshava Feb 21, 2024
1926c42
move the deliver_max alias logic into Transaction.from_xrpl
ckeshava Feb 21, 2024
7de639c
additional comments to explain the process_json_binary_codec utility …
ckeshava Feb 21, 2024
56b8f32
Update xrpl/models/base_model.py
ckeshava Feb 25, 2024
c47fac1
Update xrpl/models/base_model.py
ckeshava Feb 25, 2024
d47260c
Update xrpl/models/base_model.py
ckeshava Feb 25, 2024
3f417a0
Update xrpl/models/transactions/transaction.py
ckeshava Feb 25, 2024
b8bd5a2
addresing PR comments
ckeshava Feb 25, 2024
211e378
Update xrpl/models/transactions/transaction.py
ckeshava Feb 26, 2024
79fd8fd
Merge branch 'main' of https://github.com/XRPLF/xrpl-py into dMax
ckeshava Mar 20, 2024
e3d0b7b
address Justin's PR comments: refactor nested if conditions using and…
ckeshava Mar 20, 2024
59c77d7
Merge branch 'dMax' of https://github.com/XRPLF/xrpl-py into dMax
ckeshava Mar 20, 2024
051e13e
Update xrpl/models/transactions/transaction.py
ckeshava Mar 22, 2024
f5fae35
refactor process_xrpl_json into a classmethod
ckeshava Mar 22, 2024
47c6b07
Merge branch 'dMax' of https://github.com/XRPLF/xrpl-py into dMax
ckeshava Mar 22, 2024
052ba00
integration tests: verify the deliver_max/amount alias fields in Paym…
ckeshava Apr 22, 2024
57aa59a
address Omar's comments. update process_xrpl_json into a private method
ckeshava Apr 25, 2024
eeaf241
Merge branch 'main' into dMax
ckeshava Apr 25, 2024
6af9177
Merge branch 'main' into dMax
ckeshava Jun 4, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [[Unreleased]]
- Add support for the DeliverMax field in Payment transactions
- Included `ctid` field in the `tx` request.

### Fixed
Expand Down
68 changes: 67 additions & 1 deletion tests/unit/models/transactions/test_transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from xrpl.asyncio.transaction.main import sign
from xrpl.models.exceptions import XRPLModelException
from xrpl.models.transactions import AccountSet, OfferCreate
from xrpl.models.transactions import AccountSet, OfferCreate, Payment
from xrpl.models.transactions.transaction import Transaction
from xrpl.models.transactions.types.transaction_type import TransactionType
from xrpl.transaction.multisign import multisign
Expand Down Expand Up @@ -157,3 +157,69 @@ def test_is_signed_for_multisigned_transaction(self):

multisigned_tx = multisign(tx, [tx_1, tx_2])
self.assertTrue(multisigned_tx.is_signed())

# test the usage of DeliverMax field in Payment transactions
def test_payment_txn_API_no_deliver_max(self):
delivered_amount = "200000"
payment_tx_json = {
"Account": "rGWTUVmm1fB5QUjMYn8KfnyrFNgDiD9H9e",
"Destination": "rw71Qs1UYQrSQ9hSgRohqNNQcyjCCfffkQ",
"TransactionType": "Payment",
"Amount": delivered_amount,
"Fee": "15",
"Flags": 0,
"Sequence": 144,
"LastLedgerSequence": 6220218,
}

payment_txn = Payment.from_xrpl(payment_tx_json)
self.assertEqual(delivered_amount, payment_txn.to_dict()["amount"])

def test_payment_txn_API_no_amount(self):
delivered_amount = "200000"
payment_tx_json = {
"Account": "rGWTUVmm1fB5QUjMYn8KfnyrFNgDiD9H9e",
"Destination": "rw71Qs1UYQrSQ9hSgRohqNNQcyjCCfffkQ",
"TransactionType": "Payment",
"DeliverMax": delivered_amount,
"Fee": "15",
"Flags": 0,
"Sequence": 144,
"LastLedgerSequence": 6220218,
}

payment_txn = Payment.from_xrpl(payment_tx_json)
self.assertEqual(delivered_amount, payment_txn.to_dict()["amount"])

def test_payment_txn_API_different_amount_and_deliver_max(self):
payment_tx_json = {
"Account": "rGWTUVmm1fB5QUjMYn8KfnyrFNgDiD9H9e",
"Destination": "rw71Qs1UYQrSQ9hSgRohqNNQcyjCCfffkQ",
"TransactionType": "Payment",
"DeliverMax": "200000",
"Amount": "200010",
"Fee": "15",
"Flags": 0,
"Sequence": 144,
"LastLedgerSequence": 6220218,
}

with self.assertRaises(XRPLModelException):
Payment.from_xrpl(payment_tx_json)

def test_payment_txn_API_identical_amount_and_deliver_max(self):
delivered_amount = "200000"
payment_tx_json = {
"Account": "rGWTUVmm1fB5QUjMYn8KfnyrFNgDiD9H9e",
"Destination": "rw71Qs1UYQrSQ9hSgRohqNNQcyjCCfffkQ",
"TransactionType": "Payment",
"DeliverMax": delivered_amount,
"Amount": delivered_amount,
"Fee": "15",
"Flags": 0,
"Sequence": 144,
"LastLedgerSequence": 6220218,
}

payment_txn = Payment.from_xrpl(payment_tx_json)
self.assertEqual(delivered_amount, payment_txn.to_dict()["amount"])
30 changes: 23 additions & 7 deletions xrpl/models/base_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 don't know which exceptions might be thrown by this code. _key_to_json and _value_to_json do not throw any exceptions themselves

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function will never be directly used by end users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I think it's a reasonable tradeoff 👍

"""
Creates a dictionary object based on a JSON or dictionary in the standard XRPL
format.

Args:
value: The dictionary or JSON string to be processed.

Returns:
A formatted dictionary instantiated from the input.
"""
if isinstance(value, str):
value = json.loads(value)

formatted_dict = {
_key_to_json(k): _value_to_json(v)
for (k, v) in cast(Dict[str, XRPL_VALUE_TYPE], value).items()
}

return formatted_dict


@dataclass(frozen=True)
class BaseModel(ABC):
"""The base class for all model types."""
Expand Down Expand Up @@ -232,13 +254,7 @@ def from_xrpl(cls: Type[BM], value: Union[str, Dict[str, Any]]) -> BM:
Returns:
A BaseModel object instantiated from the input.
"""
if isinstance(value, str):
value = json.loads(value)

formatted_dict = {
_key_to_json(k): _value_to_json(v)
for (k, v) in cast(Dict[str, XRPL_VALUE_TYPE], value).items()
}
formatted_dict = process_xrpl_json(value)

return cls.from_dict(formatted_dict)

Expand Down
42 changes: 41 additions & 1 deletion xrpl/models/transactions/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from xrpl.core.binarycodec import decode, encode
from xrpl.models.amounts import IssuedCurrencyAmount
from xrpl.models.base_model import ABBREVIATIONS, BaseModel
from xrpl.models.base_model import ABBREVIATIONS, BaseModel, process_xrpl_json
from xrpl.models.exceptions import XRPLModelException
from xrpl.models.flags import check_false_flag_definition, interface_to_flag_list
from xrpl.models.nested_model import NestedModel
Expand Down Expand Up @@ -468,3 +468,43 @@ def from_blob(tx_blob: str) -> Transaction:
The formatted transaction.
"""
return Transaction.from_xrpl(decode(tx_blob))

@classmethod
def from_xrpl(cls: Type[T], value: Union[str, Dict[str, Any]]) -> T:
"""
Creates a Transaction object based on a JSON or JSON-string representation of
data

In Payment transactions, the DeliverMax field is renamed to the Amount field.

Args:
value: The dictionary or JSON string to be instantiated.

Returns:
A Transaction object instantiated from the input.

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

Choose a reason for hiding this comment

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

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


# handle the deliver_max alias in Payment transactions
if (
"transaction_type" in processed_value
and processed_value["transaction_type"] == "Payment"
):
if "deliver_max" in processed_value:
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
if "amount" in processed_value:
if processed_value["amount"] != processed_value["deliver_max"]:
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
raise XRPLModelException(
"Error: amount and deliver_max fields are not identical"
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
)
else:
processed_value["amount"] = processed_value["deliver_max"]

# deliver_max field is not recognised in the Payment Request format,
# nor is it supported in the serialization operations.
del processed_value["deliver_max"]

return cls.from_dict(processed_value)
Loading