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

split into constants,v2,v3 #585

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

split into constants,v2,v3 #585

wants to merge 9 commits into from

Conversation

mkflow27
Copy link
Contributor

@mkflow27 mkflow27 commented Feb 7, 2025

This pr closes https://github.com/orgs/balancer/projects/9/?pane=issue&itemId=94402497&issue=balancer%7Cb-sdk%7C563 The constants were moved either to new file (v2, or v3) or left in the old one

@brunoguerios
Copy link
Member

I like the idea of splitting the constants into 2 files so they are more organized, but if possible I'd like for constants themselves to also be differentiated with V2/V3 in their names.
I mean, reading the code is much easier to spot something like VAULT_V2 and VAULT_V3 than simply VAULT and having to look at the import to see it was imported from constantsV2 or constantsV3 file.
What do you think?

Copy link
Contributor Author

@mkflow27 mkflow27 left a comment

Choose a reason for hiding this comment

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

I like the idea of splitting the constants into 2 files so they are more organized, but if possible I'd like for constants themselves to also be differentiated with V2/V3 in their names.
I mean, reading the code is much easier to spot something like VAULT_V2 and VAULT_V3 than simply VAULT and having to look at the import to see it was imported from constantsV2 or constantsV3 file.
What do you think?

I think it is a good suggestion which will make the code more readable. I will implement it.
Edit: After looking through the code again, the only variable with a unclear naming would be vault -> Vault_v2. All other names can be tied to either general, v2 or v2 based on their name.

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.

2 participants