Repository: XRPLF/xrpl-py

Issue:
Fix `LedgerObject.from_xrpl` to only accept CamelCase JSON keys
Fix `LedgerObject.from_xrpl` to only accept CamelCase JSON keys. Currently, it accepts both CamelCase and snake_case keys which is invalid functionality.

Found this bug in this PR that adds Ledger Objects models - https://github.com/XRPLF/xrpl-py/pull/663

Discussion:
@mvadari: I suspect all the `from_xrpl` methods have this problem.@JST5000: I'm confused as to why this change is required. This seems like it would be a breaking change without improving the experience for users.@mvadari: > I'm confused as to why this change is required. This seems like it would be a breaking change without improving the experience for users.

It's not how that function is supposed to be used. `snake_case` keys aren't the way that the XRPL represents objects and transactions, it's an xrpl-py-specific encoding. If you're using `from_xrpl` in that way, you should be using `from_dict`. 

IMO it would be an experience improvement for users, because we'd be doing some additional processing to ensure that the data you're putting into that function is actually from the XRPL and not from something else. It's also more confusing that `snake_case` keys are supported than if they're not.@ckeshava: Clarification: Why are we supporting `snake_case` in the `from_dict` function while proposing the opposite in the `from_xrpl` function?

The two functions (`from_xrpl` and `from_dict`) appear to be doing similar things (the latter invokes the former function after performing preprocessing steps about `TransactionType`).@mvadari: > Clarification: Why are we supporting `snake_case` in the `from_dict` function while proposing the opposite in the `from_xrpl` function?
> 
> The two functions (`from_xrpl` and `from_dict`) appear to be doing similar things (the latter invokes the former function after performing preprocessing steps about `TransactionType`).

That's exactly the difference between the two functions. `from_xrpl` converts from `CamelCase` to `snake_case`, then runs `from_dict`. `from_dict` converts the dictionary to a model object. That's why they should be doing different things.@ckeshava: Okay, I understand that.

By that reasoning, `Transaction.from_xrpl` should not accept non-`CamelCase` encoding of JSON inputs. Am I right?

I couldn't find equivalent functions in the Typescript (xrpl.js) codebase. Are we obligated to maintain consistency between the libraries?

I'd prefer to maintain adherence to the XRPL encoding formats. To that end, why are we supporting `snake_case` encoding at all?  I know that it would break backwards compatibility now, but I'd like to know the original reason for the support.@mvadari: > By that reasoning, `Transaction.from_xrpl` should not accept non-`CamelCase` encoding of JSON inputs. Am I right?

Yes, this issue should really be about all `from_xrpl` methods.

> I couldn't find equivalent functions in the Typescript (xrpl.js) codebase. Are we obligated to maintain consistency between the libraries?

There are language-specific differences between the libraries. If something works better in a certain way in JS vs. Python, or if the norms are different for that language, then it'll be different. Another example of a difference based on norms in a language are how we have class models for Python but just TypeScript types (instead of classes) for JS.

> I'd prefer to maintain adherence to the XRPL encoding formats. To that end, why are we supporting `snake_case` encoding at all? I know that it would break backwards compatibility now, but I'd like to know the original reason for the support.

It's more Pythonic to use `snake_case`.@ckeshava: ok thanks, that's helpful

Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6f19d8c..9a9aeda 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Support for the Price Oracles amendment (XLS-47).
 - Add `nfts_by_issuer` clio-only API definition
 - Included `ctid` field in the `tx` request.
+- `from_xrpl` method accepts input dictionary keys exclusively in the proper XRPL format.
 
 ### Fixed
 - Added support for `XChainModifyBridge` flag maps (fixing an issue with `NFTokenCreateOffer` flag names)
diff --git a/xrpl/models/base_model.py b/xrpl/models/base_model.py
index 87c790f..82c0299 100644
--- a/xrpl/models/base_model.py
+++ b/xrpl/models/base_model.py
@@ -15,6 +15,7 @@ from xrpl.models.exceptions import XRPLModelException
 from xrpl.models.required import REQUIRED
 from xrpl.models.types import XRPL_VALUE_TYPE
 
+_PASCAL_OR_CAMEL_CASE: Final[Pattern[str]] = re.compile("^[A-Za-z]+(?:[A-Za-z0-9]+)*$")
 # this regex splits words based on one of three cases:
 #
 # 1. 1-or-more non-capital chars at the beginning of the string. Handles cases
@@ -54,7 +55,15 @@ def _key_to_json(field: str) -> str:
         1. 'TransactionType' becomes 'transaction_type'
         2. 'value' remains 'value'
         3. 'URI' becomes 'uri'
+
+        This function accepts inputs in PascalCase or camelCase only
+
+    Raises:
+        XRPLModelException: If the input is invalid
     """
+    if not re.fullmatch(pattern=_PASCAL_OR_CAMEL_CASE, string=field):
+        raise XRPLModelException(f"Key {field} is not in the proper XRPL format.")
+
     # convert all special CamelCase substrings to capitalized strings
     for spec_str in ABBREVIATIONS.values():
         if spec_str in field:


Test patch:
diff --git a/tests/unit/models/test_base_model.py b/tests/unit/models/test_base_model.py
index 7cea1d9..4f94679 100644
--- a/tests/unit/models/test_base_model.py
+++ b/tests/unit/models/test_base_model.py
@@ -388,6 +388,49 @@ class TestFromDict(TestCase):
         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",
+                },
+            },
+            "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)
+
     def test_from_xrpl(self):
         dirname = os.path.dirname(__file__)
         full_filename = "x-codec-fixtures.json"