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 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
31 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
55172f2
Merge branch 'main' into model-type-validation
mvadari Oct 25, 2024
28963b4
Merge branch 'main' into model-type-validation
mvadari Nov 4, 2024
95df2fc
Merge branch 'main' into model-type-validation
mvadari Nov 7, 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 @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- Support for the DeliverMax field in Payment transactions
- Support for the `feature` RPC
- Improved validation for models to also check param types
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Move entry to "Unreleased" section and enhance description.

The changelog entry for model validation should be moved to the "Unreleased" section since this is a new feature that hasn't been released yet. Additionally, consider expanding the description to provide more context about the types of validation being added.

Apply this diff to move and enhance the entry:

-### Added
-- Improved validation for models to also check param types

## [[Unreleased]]

### Added
+ - Added type validation for model parameters to catch type mismatches early and improve error reporting
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Improved validation for models to also check param types
### Added
## [[Unreleased]]
### Added
- Added type validation for model parameters to catch type mismatches early and improve error reporting


### Fixed
- Allow empty strings for the purpose of removing fields in DIDSet transaction
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
213 changes: 162 additions & 51 deletions tests/unit/models/test_base_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
SignerListSet,
TrustSet,
TrustSetFlag,
XChainAddAccountCreateAttestation,
XChainClaim,
)
from xrpl.models.transactions.transaction import Transaction
Expand Down Expand Up @@ -83,15 +84,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 @@ -102,7 +132,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 @@ -122,7 +152,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 @@ -139,7 +169,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 @@ -155,7 +185,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 @@ -186,7 +216,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 @@ -210,7 +240,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 @@ -230,7 +260,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 @@ -239,7 +269,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 @@ -254,7 +284,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 @@ -277,7 +307,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 @@ -290,7 +320,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 @@ -307,7 +337,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 @@ -379,7 +409,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 @@ -392,49 +422,83 @@ def test_from_dict_submit(self):
actual = Request.from_dict(request)
self.assertEqual(actual, expected)

# Note: BaseModel.from_xrpl and its overridden methods accept only camelCase or
# PascalCase inputs (i.e. snake_case is not accepted)
def test_request_input_from_xrpl_accepts_camel_case(self):
request = {
"method": "submit",
"tx_json": {
"Account": "rnD6t3JF9RTG4VgNLoc4i44bsQLgJUSi6h",
"transaction_type": "TrustSet",
"Fee": "10",
"Sequence": 17896798,
"Flags": 131072,
"signing_pub_key": "",
"limit_amount": {
"currency": "USD",
"issuer": "rH5gvkKxGHrFAMAACeu9CB3FMu7pQ9jfZm",
"value": "10",
},
def test_nonexistent_field(self):
tx = {
"account": "rH6ZiHU1PGamME2LvVTxrgvfjQpppWKGmr",
"bad_field": "random",
"flags": 131072,
"limit_amount": {
"currency": "USD",
"issuer": "raoV5dkC66XvGWjSzUhCUuuGM3YFTitMxT",
"value": "100",
},
"fail_hard": False,
}
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",
mvadari marked this conversation as resolved.
Show resolved Hide resolved
"signature": "3044022053B26DAAC9C886192C95",
"other_chain_source": issuer,
"amount": amount_dict,
"attestation_reward_account": issuer,
"attestation_signer_account": issuer,
"was_locking_chain_send": 2, # supposed to be 0 or 1
"xchain_account_create_count": 12,
"destination": issuer,
"signature_reward": "200",
}
with self.assertRaises(XRPLModelException):
Request.from_xrpl(request)
XChainAddAccountCreateAttestation.from_dict(tx)

def test_transaction_input_from_xrpl_accepts_only_camel_case(self):
# verify that Transaction.from_xrpl method does not accept snake_case JSON keys
tx_snake_case_keys = {
"Account": "rnoGkgSpt6AX1nQxZ2qVGx7Fgw6JEcoQas",
"transaction_type": "TrustSet",
"Fee": "10",
"Sequence": 17892983,
"Flags": 131072,
"signing_pub_key": "",
"limit_amount": {
"currency": "USD",
"issuer": "rBPvTKisx7UCGLDtiUZ6mDssXNREuVuL8Y",
"value": "10",
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))

with self.assertRaises(XRPLModelException):
Transaction.from_xrpl(tx_snake_case_keys)

class TestFromXrpl(TestCase):
def test_from_xrpl(self):
dirname = os.path.dirname(__file__)
full_filename = "x-codec-fixtures.json"
Expand All @@ -444,14 +508,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 @@ -749,3 +817,46 @@ def test_to_from_xrpl_xchain(self):
)
self.assertEqual(tx_obj.to_xrpl(), tx_json)
self.assertEqual(Transaction.from_xrpl(tx_json), tx_obj)

def test_request_input_from_xrpl_accepts_camel_case(self):
# Note: BaseModel.from_xrpl and its overridden methods accept only camelCase or
# PascalCase inputs (i.e. snake_case is not accepted)
request = {
"method": "submit",
"tx_json": {
"Account": "rnD6t3JF9RTG4VgNLoc4i44bsQLgJUSi6h",
"transaction_type": "TrustSet",
"Fee": "10",
"Sequence": 17896798,
"Flags": 131072,
"signing_pub_key": "",
"limit_amount": {
"currency": "USD",
"issuer": "rH5gvkKxGHrFAMAACeu9CB3FMu7pQ9jfZm",
"value": "10",
},
},
"fail_hard": False,
}

with self.assertRaises(XRPLModelException):
Request.from_xrpl(request)

def test_transaction_input_from_xrpl_accepts_only_camel_case(self):
# verify that Transaction.from_xrpl method does not accept snake_case JSON keys
tx_snake_case_keys = {
"Account": "rnoGkgSpt6AX1nQxZ2qVGx7Fgw6JEcoQas",
"transaction_type": "TrustSet",
"Fee": "10",
"Sequence": 17892983,
"Flags": 131072,
"signing_pub_key": "",
"limit_amount": {
"currency": "USD",
"issuer": "rBPvTKisx7UCGLDtiUZ6mDssXNREuVuL8Y",
"value": "10",
},
}

with self.assertRaises(XRPLModelException):
Transaction.from_xrpl(tx_snake_case_keys)
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