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

feat: add model validation for types #708

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
4cc6762
add type validation
mvadari May 7, 2024
8dcd7dd
add tests, fix tests
mvadari May 7, 2024
cb15f44
improve test coverage of BaseModel, fix issues
mvadari May 7, 2024
aa039d2
split out from_xrpl tests from from_dict
mvadari May 8, 2024
8ae0e1c
update changelog
mvadari May 17, 2024
310363e
fix tests
mvadari May 17, 2024
8f2460a
fix snippet
mvadari May 17, 2024
eaf2f8b
respond to comments
mvadari Jun 5, 2024
0e04538
fix tests
mvadari Jun 5, 2024
8820926
Merge branch 'main' into model-type-validation
mvadari Jun 5, 2024
84b29a7
fix test
mvadari Jun 5, 2024
3578749
add comment
mvadari Jun 5, 2024
cfea264
Merge branch 'main' into model-type-validation
mvadari Jul 1, 2024
5606e0d
Merge branch 'main' into model-type-validation
mvadari Jul 3, 2024
2bdc65a
Merge branch 'main' into model-type-validation
mvadari Jul 3, 2024
eeaa426
Merge branch 'main' into model-type-validation
mvadari Jul 3, 2024
ac0b4c6
Merge branch 'main' into model-type-validation
mvadari Jul 8, 2024
e94d979
fix merge issue
mvadari Jul 8, 2024
faf75fd
Merge branch 'main' into model-type-validation
mvadari Jul 9, 2024
68907d4
Merge branch 'main' into model-type-validation
mvadari Jul 11, 2024
aa9d110
Merge branch 'main' into model-type-validation
mvadari Jul 22, 2024
525c15d
Merge branch 'main' into model-type-validation
mvadari Aug 27, 2024
a3a4efe
fix mypy
mvadari Sep 10, 2024
e08ccdf
fix comment
mvadari Sep 10, 2024
c745f69
fix other syntax issues
mvadari Sep 10, 2024
786ad8d
fix issues
mvadari Sep 19, 2024
71b0a04
Merge branch 'main' into model-type-validation
mvadari Sep 26, 2024
850998d
Merge branch 'main' into model-type-validation
mvadari Sep 26, 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 @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- Support for the Price Oracles amendment (XLS-47).
- Improved validation for models to also check param types

### Fixed
- Added support for `XChainModifyBridge` flag maps (fixing an issue with `NFTokenCreateOffer` flag names)
Expand Down
4 changes: 3 additions & 1 deletion snippets/paths.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""Example of how to find the best path to trade with"""

from xrpl.clients import JsonRpcClient
from xrpl.models import XRP, IssuedCurrencyAmount, Payment, RipplePathFind
from xrpl.models.path import PathStep
from xrpl.transaction import autofill_and_sign
from xrpl.wallet import generate_faucet_wallet

Expand Down Expand Up @@ -41,7 +43,7 @@
account=wallet.address,
amount=destination_amount,
destination=destination_account,
paths=paths,
paths=[[PathStep(**path_data) for path_data in path] for path in paths],
mvadari marked this conversation as resolved.
Show resolved Hide resolved
)

print("signed: ", autofill_and_sign(payment_tx, client, wallet))
119 changes: 116 additions & 3 deletions tests/unit/models/test_base_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
SignerListSet,
TrustSet,
TrustSetFlag,
XChainAddAccountCreateAttestation,
XChainClaim,
)
from xrpl.models.transactions.transaction import Transaction
Expand Down Expand Up @@ -82,6 +83,35 @@ def test_is_dict_of_model_when_not_true(self):
),
)

def test_bad_type(self):
transaction_dict = {
"account": 1,
"amount": 10,
"destination": 1,
}
with self.assertRaises(XRPLModelException):
Payment(**transaction_dict)

def test_bad_type_flags(self):
transaction_dict = {
"account": 1,
"amount": 10,
"destination": 1,
"flags": "1234",
}
with self.assertRaises(XRPLModelException):
Payment(**transaction_dict)
mvadari marked this conversation as resolved.
Show resolved Hide resolved

def test_bad_type_enum(self):
path_find_dict = {
"subcommand": "blah",
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
"source_account": "raoV5dkC66XvGWjSzUhCUuuGM3YFTitMxT",
"destination_account": "rJjusz1VauNA9XaHxJoiwHe38bmQFz1sUV",
"destination_amount": "100",
}
with self.assertRaises(XRPLModelException):
PathFind(**path_find_dict)


class TestFromDict(TestCase):
maxDiff = 2000
Expand Down Expand Up @@ -388,6 +418,83 @@ def test_from_dict_submit(self):
actual = Request.from_dict(request)
self.assertEqual(actual, expected)

def test_from_dict_nonexistent_field(self):
tx = {
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
"bad_field": "random",
"flags": 131072,
"limit_amount": {
"currency": "USD",
"issuer": "raoV5dkC66XvGWjSzUhCUuuGM3YFTitMxT",
"value": "100",
},
}
with self.assertRaises(XRPLModelException):
TrustSet.from_dict(tx)

def test_from_dict_bad_literal(self):
tx = {
mvadari marked this conversation as resolved.
Show resolved Hide resolved
"account": issuer,
"xchain_bridge": {
"locking_chain_door": issuer,
"locking_chain_issue": {"currency": "XRP"},
"issuing_chain_door": issuer,
"issuing_chain_issue": {"currency": "XRP"},
},
"public_key": "0342E083EA762D91D621714C394",
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024

Choose a reason for hiding this comment

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

Ensure public_key values do not expose sensitive information

The hard-coded public_key values at lines 448 and 471 may inadvertently contain sensitive data. Please confirm that these are not actual cryptographic keys. If they are placeholders for testing, consider annotating them as such or using clearly non-sensitive values to prevent any security concerns.

Also applies to: 471-471

Tools
Gitleaks

448-448: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a sample string (that isn't even a valid public key), just used as a placeholder.

Copy link

Choose a reason for hiding this comment

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

@mvadari, thank you for the clarification. I've noted that the public_key values in this test file are placeholders and do not represent actual cryptographic keys.

(_/)
(•‿•)


Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl-py#708
File: tests/unit/models/test_base_model.py:448-448
Timestamp: 2024-09-19T20:01:37.621Z
Learning: In `tests/unit/models/test_base_model.py`, the `public_key` values are sample strings used as placeholders and do not contain actual cryptographic keys.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

"signature": "3044022053B26DAAC9C886192C95",
"other_chain_source": issuer,
"amount": amount_dict,
"attestation_reward_account": issuer,
"attestation_signer_account": issuer,
"was_locking_chain_send": 2,
"xchain_account_create_count": 12,
"destination": issuer,
"signature_reward": "200",
}
with self.assertRaises(XRPLModelException):
XChainAddAccountCreateAttestation.from_dict(tx)

def test_from_dict_good_literal(self):
tx = {
"account": issuer,
"xchain_bridge": {
"locking_chain_door": issuer,
"locking_chain_issue": {"currency": "XRP"},
"issuing_chain_door": issuer,
"issuing_chain_issue": {"currency": "XRP"},
},
"public_key": "0342E083EA762D91D621714C394",
"signature": "3044022053B26DAAC9C886192C95",
"other_chain_source": issuer,
"amount": "100",
"attestation_reward_account": issuer,
"attestation_signer_account": issuer,
"was_locking_chain_send": 1,
"xchain_account_create_count": 12,
"destination": issuer,
"signature_reward": "200",
}
expected_dict = {
**tx,
"xchain_bridge": XChainBridge.from_dict(tx["xchain_bridge"]),
}
expected = XChainAddAccountCreateAttestation(
**expected_dict,
)
self.assertEqual(XChainAddAccountCreateAttestation.from_dict(tx), expected)

def test_from_dict_enum(self):
path_find_dict = {
"subcommand": "create",
"source_account": "raoV5dkC66XvGWjSzUhCUuuGM3YFTitMxT",
"destination_account": "rJjusz1VauNA9XaHxJoiwHe38bmQFz1sUV",
"destination_amount": "100",
}
self.assertEqual(PathFind.from_dict(path_find_dict), PathFind(**path_find_dict))


class TestToFromXrpl(TestCase):
def test_from_xrpl(self):
dirname = os.path.dirname(__file__)
full_filename = "x-codec-fixtures.json"
Expand All @@ -397,14 +504,18 @@ def test_from_xrpl(self):
for test in fixtures_json["transactions"]:
x_json = test["xjson"]
r_json = test["rjson"]
with self.subTest(json=x_json):
with self.subTest(json=x_json, use_json=False):
mvadari marked this conversation as resolved.
Show resolved Hide resolved
tx = Transaction.from_xrpl(x_json)
translated_tx = tx.to_xrpl()
self.assertEqual(x_json, translated_tx)
with self.subTest(json=r_json):
with self.subTest(json=r_json, use_json=False):
tx = Transaction.from_xrpl(r_json)
translated_tx = tx.to_xrpl()
self.assertEqual(r_json, translated_tx)
with self.subTest(json=r_json, use_json=True):
tx = Transaction.from_xrpl(json.dumps(r_json))
translated_tx = tx.to_xrpl()
self.assertEqual(r_json, translated_tx)

def test_from_xrpl_signers(self):
txn_sig1 = (
Expand Down Expand Up @@ -536,7 +647,9 @@ def test_to_xrpl_paths(self):
issuer="rweYz56rfmQ98cAdRaeTxQS9wVMGnrdsFp",
value="0.0000002831214446",
),
paths=paths_json,
paths=[
[PathStep(**path_data) for path_data in path] for path in paths_json
],
sequence=290,
)
tx_json = p.to_xrpl()
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/models/transactions/test_check_cash.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
_ACCOUNT = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ"
_FEE = "0.00001"
_SEQUENCE = 19048
_CHECK_ID = 19048
_CHECK_ID = "838766BA2B995C00744175F69A1B11E32C3DBC40E64801A4056FCBD657F57334"
_AMOUNT = "300"


Expand Down
2 changes: 1 addition & 1 deletion tests/unit/models/transactions/test_oracle_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ def test_early_last_update_time_field(self):
self.assertEqual(
err.exception.args[0],
"{'last_update_time': 'LastUpdateTime"
+ " must be greater than or equal to Ripple-Epoch 946684800.0 seconds'}",
+ " must be greater than or equal to ripple epoch - 946684800 seconds'}",
)

# Validity depends on the time of the Last Closed Ledger. This test verifies the
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/models/transactions/test_xchain_claim.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def test_successful_claim_destination_tag(self):
xchain_bridge=_XRP_BRIDGE,
xchain_claim_id=_CLAIM_ID,
destination=_DESTINATION,
destination_tag="12345",
destination_tag=12345,
amount=_XRP_AMOUNT,
)

Expand Down
90 changes: 74 additions & 16 deletions xrpl/models/base_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from enum import Enum
from typing import Any, Dict, List, Pattern, Type, TypeVar, Union, cast, get_type_hints

from typing_extensions import Final, Literal, get_args, get_origin
from typing_extensions import Final, Literal, Self, get_args, get_origin

from xrpl.models.exceptions import XRPLModelException
from xrpl.models.required import REQUIRED
Expand Down Expand Up @@ -242,11 +242,11 @@ def from_xrpl(cls: Type[BM], value: Union[str, Dict[str, Any]]) -> BM:

return cls.from_dict(formatted_dict)

def __post_init__(self: BaseModel) -> None:
def __post_init__(self: Self) -> None:
"""Called by dataclasses immediately after __init__."""
self.validate()

def validate(self: BaseModel) -> None:
def validate(self: Self) -> None:
"""
Raises if this object is invalid.

Expand All @@ -257,7 +257,7 @@ def validate(self: BaseModel) -> None:
if len(errors) > 0:
raise XRPLModelException(str(errors))

def is_valid(self: BaseModel) -> bool:
def is_valid(self: Self) -> bool:
"""
Returns whether this BaseModel is valid.

Expand All @@ -266,20 +266,78 @@ def is_valid(self: BaseModel) -> bool:
"""
return len(self._get_errors()) == 0

def _get_errors(self: BaseModel) -> Dict[str, str]:
def _check_type(
self: Self, attr: str, value: Any, expected_type: Type[Any]
) -> Dict[str, str]:
# returns error message if type is bad, None if type is good
expected_type_origin = get_origin(expected_type)
if expected_type_origin is Union:
if any(
len(self._check_type(attr, value, expected_type_option)) == 0
for expected_type_option in get_args(expected_type)
):
return {}
return {attr: f"{attr} is {type(value)}, expected {expected_type}"}

if expected_type is Any:
return {}

if expected_type_origin is list:
# expected a List, received a List
# TODO: check for
mvadari marked this conversation as resolved.
Show resolved Hide resolved
if not isinstance(value, list):
return {"attr": f"{attr} is {type(value)}, expected {expected_type}"}
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024

Choose a reason for hiding this comment

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

Fix the typo in error dictionary keys to correctly reference the variable attr.

In the return statements on lines 314, 328, and 348, the key of the error dictionary is mistakenly set to the string "attr" instead of the variable attr. This will result in error dictionaries with the key "attr" instead of the actual attribute name, which could lead to confusion when handling errors.

Apply this diff to correct the error:

- return {"attr": f"{attr} is {type(value)}, expected {expected_type}"}
+ return {attr: f"{attr} is {type(value)}, expected {expected_type}"}

Also applies to: 328-328, 348-348

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 786ad8d

Copy link

Choose a reason for hiding this comment

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

@mvadari Thank you for fixing the issue! 🐰🎉


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

result = {}
for i in range(len(value)):
result.update(
self._check_type(
f"{attr}[{i}]", value[i], get_args(expected_type)[0]
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
)
)
return result

if expected_type_origin is dict:
return (
{}
if isinstance(value, dict)
else {attr: f"{attr} is {type(value)}, expected {expected_type}"}
)

if isinstance(expected_type, type) and issubclass(expected_type, Enum):
return (
{}
if value in list(expected_type)
else {
attr: f"{attr} is {value}, expected member of {expected_type} enum"
}
)

if expected_type_origin is Literal:
arg = get_args(expected_type)
return {} if value in arg else {attr: f"{attr} is {value}, expected {arg}"}

if not isinstance(value, expected_type):
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
return {attr: f"{attr} is {type(value)}, expected {expected_type}"}

return {}

def _get_errors(self: Self) -> Dict[str, str]:
"""
Extended in subclasses to define custom validation logic.

Returns:
Dictionary of any errors found on self.
"""
return {
attr: f"{attr} is not set"
for attr, value in self.__dict__.items()
if value is REQUIRED
}

def to_dict(self: BaseModel) -> Dict[str, Any]:
class_types = get_type_hints(self.__class__)
result: Dict[str, str] = {}
for attr, value in self.__dict__.items():
if value is REQUIRED:
result[attr] = f"{attr} is not set"
else:
result.update(self._check_type(attr, value, class_types[attr]))
return result

def to_dict(self: Self) -> Dict[str, Any]:
"""
Returns the dictionary representation of a BaseModel.

Expand All @@ -296,7 +354,7 @@ def to_dict(self: BaseModel) -> Dict[str, Any]:
if getattr(self, key) is not None
}

def _to_dict_elem(self: BaseModel, elem: Any) -> Any:
def _to_dict_elem(self: Self, elem: Any) -> Any:
if isinstance(elem, BaseModel):
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
return elem.to_dict()
if isinstance(elem, Enum):
Expand All @@ -309,11 +367,11 @@ def _to_dict_elem(self: BaseModel, elem: Any) -> Any:
]
return elem

def __eq__(self: BaseModel, other: object) -> bool:
def __eq__(self: Self, other: object) -> bool:
"""Compares a BaseModel to another object to determine if they are equal."""
return isinstance(other, BaseModel) and self.to_dict() == other.to_dict()

def __repr__(self: BaseModel) -> str:
"""Returns a string representation of a BaseModel object"""
def __repr__(self: Self) -> str:
"""Returns a string representation of a BaseModel object."""
repr_items = [f"{key}={repr(value)}" for key, value in self.to_dict().items()]
return f"{type(self).__name__}({repr_items})"
1 change: 1 addition & 0 deletions xrpl/models/currencies/xrp.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

See https://xrpl.org/currency-formats.html#specifying-currency-amounts
"""

from __future__ import annotations

from dataclasses import dataclass, field
Expand Down
5 changes: 3 additions & 2 deletions xrpl/models/requests/ledger_entry.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
different types of objects you can retrieve.
`See ledger entry <https://xrpl.org/ledger_entry.html>`_
"""

from __future__ import annotations

from dataclasses import dataclass, field
Expand Down Expand Up @@ -253,9 +254,9 @@ class LedgerEntry(Request, LookupByLedgerRequest):
ticket: Optional[Union[str, Ticket]] = None
bridge_account: Optional[str] = None
bridge: Optional[XChainBridge] = None
xchain_claim_id: Optional[Union[str, XChainClaimID]] = None
xchain_claim_id: Optional[Union[int, str, XChainClaimID]] = None
xchain_create_account_claim_id: Optional[
Union[str, XChainCreateAccountClaimID]
Union[int, str, XChainCreateAccountClaimID]
] = None

binary: bool = False
Expand Down
Loading
Loading