refactor: improve Request.from_dict to have fewer exceptions to the rule

CHANGELOG.md

@@ -27,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - Allow empty strings for the purpose of removing fields in DIDSet transaction
+- Added support for `amm_info` to `Request.from_dict`
+- Improved erroring for `amm_info`
 
 ### Removed
 - Remove deprecated `full`, `accounts`, and `type` parameters from ledger request model

tests/unit/models/requests/test_amm_info.py

@@ -1,11 +1,13 @@
 from unittest import TestCase
 
 from xrpl.models.currencies import XRP, IssuedCurrency
+from xrpl.models.exceptions import XRPLModelException
 from xrpl.models.requests import AMMInfo
 from xrpl.models.requests.request import _DEFAULT_API_VERSION
 
 _ASSET = XRP()
 _ASSET_2 = IssuedCurrency(currency="USD", issuer="rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj")
+_ACCOUNT = _ASSET_2.issuer
 
 
 class TestAMMInfo(TestCase):
@@ -23,3 +25,29 @@ def test_asset_asset2(self):
             asset2=_ASSET_2,
         )
         self.assertTrue(request.is_valid())
+
+    def test_amount(self):
+        request = AMMInfo(
+            amm_account=_ACCOUNT,
+        )
+        self.assertTrue(request.is_valid())
+
+    def test_all_three(self):
+        with self.assertRaises(XRPLModelException):
+            AMMInfo(
+                amm_account=_ACCOUNT,
+                asset=_ASSET,
+                asset2=_ASSET_2,
+            )
+
+    def test_only_asset(self):
+        with self.assertRaises(XRPLModelException):
+            AMMInfo(
+                asset=_ASSET,
+            )
+
+    def test_only_one_asset2(self):
+        with self.assertRaises(XRPLModelException):
+            AMMInfo(
+                asset2=_ASSET_2,
+            )

tests/unit/models/requests/test_requests.py

@@ -1,18 +1,101 @@
 from unittest import TestCase
 
-from xrpl.models.requests import Fee, GenericRequest
+from xrpl.models.exceptions import XRPLModelException
+from xrpl.models.requests import Fee, GenericRequest, Request
 from xrpl.models.requests.request import _DEFAULT_API_VERSION
 
 
 class TestRequest(TestCase):
     def test_to_dict_includes_method_as_string(self):
-        tx = Fee()
-        value = tx.to_dict()["method"]
+        req = Fee()
+        value = req.to_dict()["method"]
         self.assertEqual(type(value), str)
 
     def test_generic_request_to_dict_sets_command_as_method(self):
         command = "validator_list_sites"
-        tx = GenericRequest(command=command).to_dict()
-        self.assertDictEqual(
-            tx, {"method": command, "api_version": _DEFAULT_API_VERSION}
-        )
+        req = GenericRequest(command=command).to_dict()
+        expected = {**req, "api_version": _DEFAULT_API_VERSION}
+        self.assertDictEqual(req, expected)
+
+    def test_from_dict(self):
+        req = {"method": "account_tx", "account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj"}
+        obj = Request.from_dict(req)
+        self.assertEqual(obj.__class__.__name__, "AccountTx")
+        expected = {
+            **req,
+            "binary": False,
+            "forward": False,
+            "api_version": _DEFAULT_API_VERSION,
+        }
+        self.assertDictEqual(obj.to_dict(), expected)
+
+    def test_from_dict_no_method(self):
+        req = {"account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj"}
+        with self.assertRaises(XRPLModelException):
+            Request.from_dict(req)
+
+    def test_from_dict_wrong_method(self):
+        req = {"method": "account_tx"}
+        with self.assertRaises(XRPLModelException):
+            Fee.from_dict(req)
+
+    def test_from_dict_noripple_check(self):
+        req = {
+            "method": "noripple_check",
+            "account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj",
+            "role": "user",
+        }
+        obj = Request.from_dict(req)
+        self.assertEqual(obj.__class__.__name__, "NoRippleCheck")
+        expected = {
+            **req,
+            "transactions": False,
+            "limit": 300,
+            "api_version": _DEFAULT_API_VERSION,
+        }
+        self.assertDictEqual(obj.to_dict(), expected)
+
+    def test_from_dict_account_nfts(self):
+        req = {
+            "method": "account_nfts",
+            "account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj",
+        }
+        obj = Request.from_dict(req)
+        expected = {**req, "api_version": _DEFAULT_API_VERSION}
+        self.assertEqual(obj.__class__.__name__, "AccountNFTs")
+        self.assertDictEqual(obj.to_dict(), expected)
+
+    def test_from_dict_amm_info(self):
+        req = {
+            "method": "amm_info",
+            "amm_account": "rN6zcSynkRnf8zcgTVrRL8K7r4ovE7J4Zj",
+        }
+        obj = Request.from_dict(req)
+        expected = {**req, "api_version": _DEFAULT_API_VERSION}
+        self.assertEqual(obj.__class__.__name__, "AMMInfo")
+        self.assertDictEqual(obj.to_dict(), expected)
+
+    def test_from_dict_nft_history(self):
+        req = {
+            "method": "nft_history",
+            "nft_id": "00000000",
+        }
+        obj = Request.from_dict(req)
+        expected = {
+            **req,
+            "binary": False,
+            "forward": False,
+            "api_version": _DEFAULT_API_VERSION,
+        }
+        self.assertEqual(obj.__class__.__name__, "NFTHistory")
+        self.assertDictEqual(obj.to_dict(), expected)
+
+    def test_from_dict_generic_request(self):
+        req = {
+            "method": "tx_history",
+            "start": 0,
+        }
+        obj = Request.from_dict(req)
+        expected = {**req, "api_version": _DEFAULT_API_VERSION}
+        self.assertEqual(obj.__class__.__name__, "GenericRequest")
+        self.assertDictEqual(obj.to_dict(), expected)

xrpl/models/base_model.py

@@ -34,13 +34,16 @@
     f"(?:{_CAMEL_CASE_LEADING_LOWER}|{_CAMEL_CASE_ABBREVIATION}|{_CAMEL_CASE_TYPICAL})"
 )
 # This is used to make exceptions when converting dictionary keys to xrpl JSON
-# keys. We snake case keys, but some keys are abbreviations.
+# keys. xrpl-py uses snake case keys, but some keys are abbreviations.
 ABBREVIATIONS: Final[Dict[str, str]] = {
     "amm": "AMM",
     "did": "DID",
     "id": "ID",
     "lp": "LP",
+    "nft": "NFT",
     "nftoken": "NFToken",
+    "nfts": "NFTs",
+    "noripple": "NoRipple",
     "unl": "UNL",
     "uri": "URI",
     "xchain": "XChain",

xrpl/models/requests/amm_info.py

@@ -1,8 +1,11 @@
 """This request gets information about an Automated Market Maker (AMM) instance."""
+
 from __future__ import annotations
 
 from dataclasses import dataclass, field
-from typing import Optional
+from typing import Dict, Optional
+
+from typing_extensions import Self
 
 from xrpl.models.currencies import Currency
 from xrpl.models.requests.request import Request, RequestMethod
@@ -33,3 +36,11 @@ class AMMInfo(Request):
     """
 
     method: RequestMethod = field(default=RequestMethod.AMM_INFO, init=False)
+
+    def _get_errors(self: Self) -> Dict[str, str]:
+        errors = super()._get_errors()
+        if (self.asset is None) != (self.asset2 is None):
+            errors["assets"] = "Must have both `asset` and `asset2` fields."
+        if (self.asset is None) == (self.amm_account is None):
+            errors["params"] = "Must not have both `asset` and `amm_account` fields."
+        return errors

xrpl/models/requests/request.py

@@ -12,7 +12,7 @@
 from typing_extensions import Final, Self
 
 import xrpl.models.requests  # bare import to get around circular dependency
-from xrpl.models.base_model import BaseModel
+from xrpl.models.base_model import ABBREVIATIONS, BaseModel
 from xrpl.models.exceptions import XRPLModelException
 from xrpl.models.required import REQUIRED
 from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init
@@ -131,6 +131,8 @@ def from_dict(cls: Type[Self], value: Dict[str, Any]) -> Self:
         Raises:
             XRPLModelException: If the dictionary provided is invalid.
         """
+        # TODO: add support for "command" parameter and proper JSON RPC format
+        # This is already done in `GenericRequest`, for reference
         if cls.__name__ == "Request":
             if "method" not in value:
                 raise XRPLModelException("Request does not include method.")
@@ -168,22 +170,12 @@ def get_method(cls: Type[Self], method: str) -> Type[Request]:
             The request class with the given name. If the request doesn't exist, then
             it will return a `GenericRequest`.
         """
-        # special case for NoRippleCheck and NFT methods
-        if method == RequestMethod.NO_RIPPLE_CHECK:
-            return xrpl.models.requests.NoRippleCheck
-        if method == RequestMethod.ACCOUNT_NFTS:
-            return xrpl.models.requests.AccountNFTs
-        if method == RequestMethod.NFT_BUY_OFFERS:
-            return xrpl.models.requests.NFTBuyOffers
-        if method == RequestMethod.NFT_SELL_OFFERS:
-            return xrpl.models.requests.NFTSellOffers
-        if method == RequestMethod.NFT_INFO:
-            return xrpl.models.requests.NFTInfo
-        if method == RequestMethod.NFT_HISTORY:
-            return xrpl.models.requests.NFTHistory
-        if method == RequestMethod.NFTS_BY_ISSUER:
-            return xrpl.models.requests.NFTsByIssuer
-        parsed_name = "".join([word.capitalize() for word in method.split("_")])
+        parsed_name = "".join(
+            [
+                ABBREVIATIONS[word] if word in ABBREVIATIONS else word.capitalize()
+                for word in method.split("_")
+            ]
+        )
         if parsed_name in xrpl.models.requests.__all__:
             return cast(Type[Request], getattr(xrpl.models.requests, parsed_name))
         return xrpl.models.requests.GenericRequest