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

Jump to bottom

feat: Lookup unique address on all chains #448

Merged
merged 11 commits into from
Oct 23, 2024
Merged

feat: Lookup unique address on all chains #448

merged 11 commits into from
Oct 23, 2024

Conversation

Tritium-VLK
Copy link
Member

Found myself writing a script to do this and thought it made the most sense in bal_addresses.

@Tritium-VLK
Copy link
Member Author

Tritium-VLK commented Sep 22, 2024 •
edited
Loading

I wasn't quite sure where to put this. It could also be a function in the AddrBook class, that can run without init. This is a bit funny in one sense, but groups the address related function under the AddrBook class. Feel free to just move it if you think something else is better or suggest so and I can.

@Tritium-VLK Tritium-VLK self-assigned this Oct 14, 2024
jalbrekt85
jalbrekt85 reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@jalbrekt85 jalbrekt85 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

feel like having this function be a static method of AddrBook would be better. might be more intuitive to find if its organized inside the AddrBook with the rest of the methods

@Tritium-VLK
Copy link
Member Author

feel like having this function be a static method of AddrBook would be better. might be more intuitive to find if its organized inside the AddrBook with the rest of the methods

Care to propose a commit for how you think it best fits? I agree it would maybe fit better as a method in the class, but the class is kind of cast for the most part by chain.

gosuto-inzasheru
gosuto-inzasheru reviewed Oct 23, 2024
View reviewed changes
Copy link
Member

@gosuto-inzasheru gosuto-inzasheru left a comment

Choose a reason for hiding this comment

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

i agree moving it into the AddrBook itself would be cleaner.

yes AddrBook is chain aware, but it could just be reinit to a new chain in the for loop

bal_addresses/addresses.py Outdated Show resolved Hide resolved
@jalbrekt85
Copy link
Contributor

feel like having this function be a static method of AddrBook would be better. might be more intuitive to find if its organized inside the AddrBook with the rest of the methods

Care to propose a commit for how you think it best fits? I agree it would maybe fit better as a method in the class, but the class is kind of cast for the most part by chain.

moved in cc2284e.
you can access static methods without initializing the class. can just do: AddrBook.get_address_all_chains()

@Tritium-VLK Tritium-VLK merged commit a657b83 into main Oct 23, 2024
3 checks passed
@Tritium-VLK Tritium-VLK deleted the lookup_unique_address_on_all_chains branch October 23, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jalbrekt85 jalbrekt85 jalbrekt85 left review comments

@gosuto-inzasheru gosuto-inzasheru gosuto-inzasheru approved these changes

Assignees

@Tritium-VLK Tritium-VLK

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tritium-VLK @jalbrekt85 @gosuto-inzasheru