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

Registry of Balancer standard contracts (v2) #1219

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

Conversation

EndymionJkb
Copy link
Collaborator

@EndymionJkb EndymionJkb commented Jan 9, 2025

Description

Based on #1206, this is designed to be a more straight-forward implementation that still covers all the use cases, but is less error-prone and without troublesome edge cases.

It maintains name => address, and address => info (i.e., state) mappings, but through guaranteeing uniqueness conceptually acts like a single name => address => info data structure, only split to enable querying by either name or address.

These entries are fixed once created (except that isActive can be set to false through deprecation), but it is possible to deregister (delete) a contract, which is the simplest mechanism to fix errors, without complex "replacement" logic.

The "alias" use case is enabled with another mapping, and a single function to add or update an alias. This separates concerns, and prevents possible corruption of the core index (e.g., setting an alias that is the same name as a real contract, which could break things if allowed on the main registry).

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Optimization: [ ] gas / [ ] bytecode
  • Documentation or wording changes
  • Other

Checklist:

  • The diff is legible and has no extraneous changes
  • Complex code has been commented, including external interfaces
  • Tests have 100% code coverage
  • The base branch is either main, or there's a description of how to merge

Issue Resolution

Copy link
Contributor

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

Thanks @EndymionJkb, looks good! I have a few comments / questions, but otherwise LGTM.

Bottom line, I'm trying to simplify the management of the mappings as much as possible (3 feels already too much to me, but I understand it's required to use it in the way it's intended to). That being said, the more we can split the source of truth from the aliases, the better.

pkg/vault/contracts/BalancerContractRegistry.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/BalancerContractRegistry.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

Sorry for being so annoying 😇 but I think there's some room to simplify the code.

pkg/vault/contracts/BalancerContractRegistry.sol Outdated Show resolved Hide resolved
@EndymionJkb EndymionJkb requested a review from jubeira January 10, 2025 18:03
Copy link
Contributor

@elshan-eth elshan-eth left a comment

Choose a reason for hiding this comment

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

Cool!

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