Add Ledger Objects #663
Add Ledger Objects #663
Conversation
There was a problem hiding this comment.
Hi Omar and @LimpidCrypto , nice work! thanks for putting all the ledger objects together.
I used this file as the source of truth (https://github.com/XRPLF/rippled/blob/develop/src/ripple/protocol/impl/LedgerFormats.cpp), so I found a few differences in the python implementation.
Let me know my suggestions make sense
Thoughts on adding prefix/postfix to the ledger object class names? |
| provided or the type mismatches the constructor type. | ||
| """ | ||
| if cls.__name__ == "LedgerObject": | ||
| if "ledger_entry_type" not in value: |
There was a problem hiding this comment.
Why are you not verifying this condition in the upper level else (last branch)?
Shouldn't we throw an exception here: if cls.__name__ != "LedgerObject" and cls.__name__ not in ["FinalFields", "PreviousFields", "NewFields"] and "ledger_entry_type" not in value ?
There was a problem hiding this comment.
As a follow-up question: I notice that 21 unit tests in test_ledger_object.py do not have a LedgerObjectEntry in their expected json. Isn't that an issue? Shouldn't all necessarily LedgerObjects contain this field?
| @@ -54,6 +56,10 @@ def interface_to_flag_list( | |||
| f[:-4]: _flag_enum_to_dict(getattr(transactions.pseudo_transactions, f)) | |||
| for f in pseudo_tx_flag_enums | |||
| }, | |||
| **{ | |||
There was a problem hiding this comment.
interface_to_flag_list appears to be used in conjunction with Transaction class. Why are we including LedgerObject flags in this list?
| "TransferRate": 1004999999, | ||
| "index": "13F1A95D7AAB7108D5CE7EEAF504B2894B8C674E6D68499076441C4837282BF8", | ||
| } | ||
| actual = LedgerObject.from_xrpl(account_root_json) |
There was a problem hiding this comment.
Can the tests also check to_xrpl, since that's just adding one line? Just to make sure that everything's correct.
| wallet_size: Optional[int] = None | ||
| """ | ||
| Unused. (The code supports this field but there is no way to set it.) | ||
| """ |
There was a problem hiding this comment.
Should this field be included?
| API method. (Note, even though this is specified as "optional" in the code, every | ||
| ledger entry should have one unless it's legacy data from very early in the XRP | ||
| Ledger's history.) |
There was a problem hiding this comment.
Should this just be specified as required here?
| API method. (Note, even though this is specified as "optional" in the code, every | ||
| ledger entry should have one unless it's legacy data from very early in the XRP | ||
| Ledger's history.) |
| elif cls.__name__ in ["FinalFields", "PreviousFields", "NewFields"]: | ||
| if "ledger_entry_type" not in value: | ||
| raise XRPLModelException( | ||
| "Ledger Object does not include ledger_entry_type." | ||
| ) | ||
| correct_type = cls.get_md_ledger_object_type(value["ledger_entry_type"]) | ||
| del value["ledger_entry_type"] | ||
| return correct_type.from_dict(value) # type: ignore |
There was a problem hiding this comment.
Why is this part included? FinalFields, PreviousFields and NewFields don't inherit from LedgerObject, and therefore this elif will never be hit.
| ) | ||
|
|
||
| @classmethod | ||
| def get_md_ledger_object_type( |
There was a problem hiding this comment.
I don't understand what the objective/purpose of this function is
| f"{ledger_object_type} is not a valid Ledger Object type." | ||
| ) | ||
|
|
||
| def __getitem__(self: LedgerObject, field_name: str) -> Any: |
There was a problem hiding this comment.
Is there a reason why this function exists for this base class and not for transactions or requests? It seems unnecessary.
|
@khancode this PR should also add support for the |
Update: this PR is deprioritized and I'm no longer working on it. Feel free to pick it up or anyone else. |
High Level Overview of Change
This PR adds (some) Ledger Object models:
You might want to look over the fields if they are still correct as it's been a while since I wrote them.
TODO
MD...models to metadata PR (see Add Metadata #664)Context of Change
@JST5000 and I had a great chat this week. He told me one thing you might prioritize as a coming feature would be response models. I remembered I once had a PR adding Ledger Object and Metadata models and was told to split it up into two PRs, but somehow I forgot about it. This PR adds the Ledger objects. I could imagine there are some objects missing or are a bit deprecated but I still wanted to leave the progress made here for you to use.
Type of Change
Did you update CHANGELOG.md?
Test Plan
I wrote tests trying to de-/serialize them from/to json.