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(protocol): Add setter to IAddressManager of AddressResolver #13799

Merged
merged 4 commits into from
May 23, 2023

Conversation

adaki2004
Copy link
Contributor

Adding a setter to IAddressManager.

Because contracts connected to AddressManager (be it static or not) via an interface, a setter is necessary.

See issue : #13798

Also I feel there is a need for the setter, because both @dantaik and @Brechtpd indicated, that each contract (i.e. ProofVerifier, TaikoL1, Bridge) shall have it's own AddressResolver (= IAddressManager interface) - tho currently the TaikoL1, Bridge, TaikoToken sharing the same AddressManager (as we can see within the tests and also within the deployment script).

There could be situations (as described in discord convo) which implies security issues for not having a setter OR we indeed need separate instances for those contracts. For an example like: TaikoL1 needs BridgeV2, although TaikoNFTBridge still needs BridgeV1, therefore TaikoL1 and TaikoNFTBridge cannot share the same.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request adds a setter to the IAddressManager.

  • The commit message is clear and concise.
  • The code changes are well-formatted and easy to read.
  • The code is properly indented.
  • The function setIAddressManager is a valid setter for the IAddressManager contract address.
  • The function has proper input validation to check if the address is a valid one.
  • There is an event emitted when the address changes.
  • It would be good to add an extra check to make sure that the new address manager is actually an instance of the IAddressManager contract.

@adaki2004 adaki2004 linked an issue May 22, 2023 that may be closed by this pull request
Copy link
Contributor

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

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

I think this makes sense in general.

packages/protocol/contracts/common/AddressResolver.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/EssentialContract.sol Outdated Show resolved Hide resolved
adaki2004 and others added 2 commits May 23, 2023 07:41
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
@adaki2004 adaki2004 requested a review from Brechtpd May 23, 2023 05:42
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, the changes look good. Here are a few points to consider:

  • In patch 1/4, it looks like the event IAddressManagerChanged is missing the I prefix.
  • In patch 3/4, the function setIAddressManager should be renamed to setAddressManager to match the event name in patch 1/4.
  • In patch 4/4, there is a typo in the comment for the newAddressManager parameter. It should be "Sets a new AddressManager's address" instead of "Sets a new address manager contract address".

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this pull request looks good. Here are a few comments on the code changes:

  • In Patch 1/4, the event IAddressManagerChanged was added. It should be named AddressManagerChanged to be consistent with other naming conventions in the codebase.
  • In Patch 3/4, the event name was changed from IAddressManagerChanged to AddressManagerChanged.
  • In Patch 4/4, the function name was changed from setIAddressManager to setAddressManager. This is consistent with the change in Patch 3/4.

@dantaik

This comment was marked as resolved.

@adaki2004
Copy link
Contributor Author

Hmmm. I'm not sure I agree. The idea is that each contract that needs address lookup is an IAddressResover itself. These address lookup contracts do not expose any function to set addresses as this is done by the admin interacting directly with the underlying/shared AddressManager.

@dantaik this function is not setting the lookup addresses, it is setting the address of the AddressManager. Currently each and every contract which inherits from EssentialContract has no chance to update their AddressManager contract in case for example it needs to (for whatever reason, be it security or 2 contracts needs to have separate AddressManager).

@adaki2004
Copy link
Contributor Author

In the case two contracts require the same string name to be resolved to different addresses, then they just need to use two different AddressManager, and updating addresses are also done by these two AddressManager.

Exactly this is what it solves. Currently you cannot do anything to separate it (after the 2 contracts deployed) - only by deploying a new proxy as well OR upgrading with a proxy to have this setter which i'm adding here.

@adaki2004 adaki2004 changed the title feat(protocol): Add setter to IAddressManager feat(protocol): Add setter to IAddressManager of AddressResolver May 23, 2023
@dantaik dantaik enabled auto-merge May 23, 2023 07:08
@dantaik dantaik added this pull request to the merge queue May 23, 2023
Merged via the queue into main with commit 34de89c May 23, 2023
@dantaik dantaik deleted the setter_to_essential_contracts_address_manager_interface branch May 23, 2023 07:08
@github-actions github-actions bot mentioned this pull request May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AddressRresolver's IAddressManager setter
4 participants