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.

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

feat: remove bal_addresses deps #70

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

gosuto-inzasheru
Copy link
Collaborator

@jalbrekt85 can you try and finalise this pr by remove the bal_addresses dep from safe_tx_builder and conftest?

that will then remove the weird circular dependency between bal_tools and bal_addresses which is blocking #69

@jalbrekt85
Copy link
Collaborator

jalbrekt85 commented Jan 10, 2025

@jalbrekt85 can you try and finalise this pr by remove the bal_addresses dep from safe_tx_builder and conftest?

that will then remove the weird circular dependency between bal_tools and bal_addresses which is blocking #69

this will require removing resolve_address from SafeTxBuilder which imo that functionality is not really needed, but i think will introduce breaking changes to scripts that use this

looks like its being used in two places afaik:

  • fee_allocator v2 generate_bribe_payload. now mitigated in this commit here
  • in this multisig-ops script: this looks like just a one off script so i don't think it is a problem

@jalbrekt85
Copy link
Collaborator

rest of bal addresses refs removed in 0101010 and 1010101

@jalbrekt85
Copy link
Collaborator

btw had to add sonic to exempt_chains to get tests to pass. i guess we can remove eventually

Copy link
Collaborator Author

@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.

amazing! that fix in protocol_fee_allocator_v2 is minimal and clean. it correctly (imo) moves the dependency to that repo instead of this one

Copy link

@Xeonus Xeonus left a comment

Choose a reason for hiding this comment

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

  • correct removal of bal_addresses dependency in requirements.txt
  • simple code linting in some components
  • removal of resolve_address method calling address book
  • fetching chain-specific data via raw json instead of address book dependency
  • tests simplified

@gosuto-inzasheru gosuto-inzasheru merged commit 2b38bfb into main Jan 13, 2025
6 checks passed
@gosuto-inzasheru gosuto-inzasheru deleted the fix/remove-bal_addresses-dependency branch January 13, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants