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 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
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": account,
"amount": value,
"destination": destination,
"flags": "1234", # should be an int
}
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", # this is invalid
"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