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.

Sign up for GitHub

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

Add __eq__ and __hash__ methods for LedgerAccount #51

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Add __eq__ and __hash__ methods for LedgerAccount #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions ledgereth/objects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,14 @@ def __init__(self, path, address):
def __repr__(self):
return f"<ledgereth.objects.LedgerAccount {self.address}>"

def __eq__(self, other):
if isinstance(other, LedgerAccount):
return self.path == other.path and self.address == other.address
Copy link
Owner

Choose a reason for hiding this comment

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

question: Why compare path here? The odds of an address collision with a different path are all but impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you when LedgerAccount is calculated from Ledger, but the class itself let you create different instances with the same address and different path, this instances shouldn't be equal between them.

Copy link
Owner

Choose a reason for hiding this comment

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

Have you run into this IRL? If so, under what circumstances?

Not asking for any changes, just trying to understand this so I don't introduce a regression unintentionally down the road.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't faced this scenario in real life. As far as I know, this could occur only if two ledgers are connected simultaneously, which isn't possible with the current implementation or someone manually initializes the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it, two ledgers shouldn't return the same address. I think the unique situation where this could happen is instantiating manually.

return False

def __hash__(self):
return hash((self.path, self.address))


class SerializableTransaction(Serializable):
"""An RLP Serializable transaction object"""
Expand Down
Loading