Add balance changes parser #383
Conversation
|
Let's start with |
|
Tests failed because of: |
xrpl/utils/txn_parser/utils/nodes.py
Outdated
| node = ( | ||
| created_node | ||
| if created_node is not None | ||
| else modified_node | ||
| if modified_node is not None | ||
| else deleted_node |
There was a problem hiding this comment.
I think this could be cleaner using if/else
| node = ( | |
| created_node | |
| if created_node is not None | |
| else modified_node | |
| if modified_node is not None | |
| else deleted_node | |
| if (created_node is not None): | |
| node = created_node | |
| elif (modified_node is not None): | |
| node = modified_node | |
| else: | |
| node = deleted_node |
There was a problem hiding this comment.
Actually, it's probably easier to just set node directly instead of setting created_node/modified_node/deleted_node as intermediary values.
Ex. node = cast(ModifiedNode, affected_node)["ModifiedNode"]
There was a problem hiding this comment.
I would have done so, but unfortunately mypy raises: 'Incompatible types in assignment (expression has type "ModifiedNodeFields", variable has type "CreatedNodeFields")'
There was a problem hiding this comment.
Your suggestion is also not valid. Same error.
There was a problem hiding this comment.
Ah sorry about that. Since node's type isn't inferrable from it's first declaration we have to specify the type explicitly. I tried locally and this version works:
if diff_type == "CreatedNode":
node: Union[CreatedNodeFields, ModifiedNodeFields, DeletedNodeFields] = cast(
CreatedNode, affected_node
)["CreatedNode"]
elif diff_type == "ModifiedNode":
node = cast(ModifiedNode, affected_node)["ModifiedNode"]
else:
node = cast(DeletedNode, affected_node)["DeletedNode"]
(Note that this replaces the diff_type if statements AND the node = ... statement)
There was a problem hiding this comment.
I don't have a preference either way. I think the TypedDicts make sense, and that using classes would also make sense. So I'm leaning towards what's easier.
The tests passed once re-ran. I copied the error down and will keep an eye out for the root cause. (Seems to be an intermittent error though, not something related to your changes) |
| class Fields(TypedDict): | ||
| """Model for possible fields.""" | ||
|
|
||
| Account: Optional[str] |
There was a problem hiding this comment.
If all of these are also optional fields, then maybe we should do the same thing as with NormalizedNodes. Might make it easier to type the code as well.
There was a problem hiding this comment.
Is there any reason you're using TypedDict instead of objects here (for everything except the final return, which I agree should be a dictionary)? It might be easier to type everything, and the constructors wouldn't even have to change.
There was a problem hiding this comment.
So you mean making CreatedNode, ModifiedNode and DeletedNode a dataclass but keep TransactionMetadata as a TypedDict?
There was a problem hiding this comment.
I think that since there's no added functions tied to the data, it makes sense to keep them as TypedDicts.
There was a problem hiding this comment.
I'm mainly suggesting that we use helper classes because it makes it easier to type optional fields. They can even use the same constructor.
There was a problem hiding this comment.
When making CreatedNode, ModifiedNode and DeletedNode dataclasses I would have to make an extra function that converts the Nodes into theses dataclasses, otherwise mypy throws errors because in _normalize_node I am passing dicts instead of CreatedNode, ... objects. Honestly I don't really think it is easier, but maybe I misunderstood your suggestion.
High Level Overview of Change
The #342 PR will be splitted in four parts. For each exported method (
parse_balance_changes,parse_final_balances,parse_previous_balancesandparse_order_book_changes) there will now be a separate PR to make it easier to review. This PR adds the functionality to parse the balance changes from a transaction's metadata.Context of Change
#298
Type of Change
Test Plan
I wrote a test to ensure the parser is working well.
XRPLTxnFieldsException