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 10 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ 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]]

### Added
- Add support for the DeliverMax field in Payment transactions
- Improved validation for models to also check param types

## [2.6.0] - 2024-06-03

Expand Down
1 change: 1 addition & 0 deletions snippets/paths.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""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.transaction import autofill_and_sign
Expand Down
145 changes: 128 additions & 17 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,15 +83,44 @@ 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

def test_from_dict_basic(self):
def test_basic(self):
amount = IssuedCurrencyAmount.from_dict(amount_dict)
self.assertEqual(amount, IssuedCurrencyAmount(**amount_dict))

def test_from_dict_recursive_amount(self):
def test_recursive_amount(self):
check_create = CheckCreate.from_dict(check_create_dict)

expected_dict = {
Expand All @@ -101,7 +131,7 @@ def test_from_dict_recursive_amount(self):
}
self.assertEqual(expected_dict, check_create.to_dict())

def test_from_dict_recursive_currency(self):
def test_recursive_currency(self):
xrp = {"currency": "XRP"}
issued_currency = {
"currency": currency,
Expand All @@ -120,7 +150,7 @@ def test_from_dict_recursive_currency(self):
}
self.assertEqual(expected_dict, book_offers.to_dict())

def test_from_dict_recursive_transaction(self):
def test_recursive_transaction(self):
transaction = CheckCreate.from_dict(check_create_dict)
sign_dict = {"secret": secret, "transaction": transaction.to_dict()}
sign = Sign.from_dict(sign_dict)
Expand All @@ -136,7 +166,7 @@ def test_from_dict_recursive_transaction(self):
del expected_dict["transaction"]
self.assertEqual(expected_dict, sign.to_dict())

def test_from_dict_recursive_transaction_tx_json(self):
def test_recursive_transaction_tx_json(self):
transaction = CheckCreate.from_dict(check_create_dict)
sign_dict = {"secret": secret, "tx_json": transaction.to_dict()}
sign = Sign.from_dict(sign_dict)
Expand All @@ -151,7 +181,7 @@ def test_from_dict_recursive_transaction_tx_json(self):
}
self.assertEqual(expected_dict, sign.to_dict())

def test_from_dict_signer(self):
def test_signer(self):
dictionary = {
"account": "rpqBNcDpWaqZC2Rksayf8UyG66Fyv2JTQy",
"fee": "10",
Expand Down Expand Up @@ -182,7 +212,7 @@ def test_from_dict_signer(self):
actual = SignerListSet.from_dict(dictionary)
self.assertEqual(actual, expected)

def test_from_dict_trust_set(self):
def test_trust_set(self):
dictionary = {
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
"fee": "10",
Expand All @@ -206,7 +236,7 @@ def test_from_dict_trust_set(self):
actual = TrustSet.from_dict(dictionary)
self.assertEqual(actual, expected)

def test_from_dict_list_of_lists(self):
def test_list_of_lists(self):
path_step_dict = {"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr"}
path_find_dict = {
"subcommand": PathFindSubcommand.CREATE,
Expand All @@ -226,7 +256,7 @@ def test_from_dict_list_of_lists(self):
actual = PathFind.from_dict(path_find_dict)
self.assertEqual(actual, expected)

def test_from_dict_any(self):
def test_any(self):
account_channels_dict = {
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
"marker": "something",
Expand All @@ -235,7 +265,7 @@ def test_from_dict_any(self):
actual = AccountChannels.from_dict(account_channels_dict)
self.assertEqual(actual, expected)

def test_from_dict_bad_str(self):
def test_bad_str(self):
dictionary = {
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
"fee": 10, # this should be a str instead ("10")
Expand All @@ -250,7 +280,7 @@ def test_from_dict_bad_str(self):
with self.assertRaises(XRPLModelException):
TrustSet.from_dict(dictionary)

def test_from_dict_explicit_none(self):
def test_explicit_none(self):
dictionary = {
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
"fee": "10",
Expand All @@ -273,7 +303,7 @@ def test_from_dict_explicit_none(self):
actual = TrustSet.from_dict(dictionary)
self.assertEqual(actual, expected)

def test_from_dict_with_str_enum_value(self):
def test_with_str_enum_value(self):
dictionary = {
"method": "account_channels",
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
Expand All @@ -286,7 +316,7 @@ def test_from_dict_with_str_enum_value(self):
actual = AccountChannels.from_dict(dictionary)
self.assertEqual(actual, expected)

def test_from_dict_bad_list(self):
def test_bad_list(self):
dictionary = {
"account": "rpqBNcDpWaqZC2Rksayf8UyG66Fyv2JTQy",
"fee": "10",
Expand All @@ -303,7 +333,7 @@ def test_from_dict_bad_list(self):
with self.assertRaises(XRPLModelException):
SignerListSet.from_dict(dictionary)

def test_from_dict_multisign(self):
def test_multisign(self):
txn_sig1 = (
"F80E201FE295AA08678F8542D8FC18EA18D582A0BD19BE77B9A24479418ADBCF4CAD28E7BD"
"96137F88DE7736827C7AC6204FBA8DDADB7394E6D704CD1F4CD609"
Expand Down Expand Up @@ -375,7 +405,7 @@ def test_from_dict_multisign(self):
actual = Request.from_dict(request)
self.assertEqual(actual, expected)

def test_from_dict_submit(self):
def test_submit(self):
blob = "SOISUSF9SD0839W8U98J98SF"
id_val = "submit_786514"
request = {
Expand All @@ -388,6 +418,83 @@ def test_from_dict_submit(self):
actual = Request.from_dict(request)
self.assertEqual(actual, expected)

def test_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_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_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_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
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
Loading
Loading