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

Update manager to support multiple implementation contracts #105

Closed
wants to merge 7 commits into from

Conversation

neokry
Copy link
Collaborator

@neokry neokry commented Jul 18, 2023

Updates manager for upcoming V2 changes

  • manager now allows Builder DAO to register alternate implementations
  • Auction, Governor, Token, Treasury and Metadata contracts now take bytes data as initialization data to keep manager generic

Code Review

  • should we keep address parameters the same for core contracts or pass in something more generic ie a struct with all address a contract could need in an initialization step? (is this worth an increase in calldata?)
  • should the manager type system use an enum instead of public const uint8s?

@neokry neokry temporarily deployed to Test July 19, 2023 00:59 — with GitHub Actions Inactive
@neokry neokry temporarily deployed to Test July 19, 2023 05:18 — with GitHub Actions Inactive
@neokry neokry requested a review from iainnash July 31, 2023 06:48
uint256 _reservePrice
) external initializer {
/// @param _data The encoded auction settings
function initialize(address _token, address _founder, address _treasury, bytes calldata _data) external initializer {
Copy link
Contributor

Choose a reason for hiding this comment

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

typically i prefer structs in this situation as noted in the diff notes due to sometimes lower code size and easier initialization on the front-end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The _data parameter is a struct so clients will be using a struct to encode it still. the address params come from the manager contract. would a struct still be useful for the code size aspect?

@@ -82,9 +75,11 @@ contract Auction is IAuction, VersionedContract, UUPS, Ownable, ReentrancyGuard,
// Store DAO's ERC-721 token
token = Token(_token);

AuctionParams memory params = abi.decode(_data, (AuctionParams));
Copy link
Contributor

Choose a reason for hiding this comment

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

This also could be an inner struct – depends on how much you want to change things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same comments as the other one mentioning structs @iainnash:

To create a struct with the addresses and data parameters we would need to copy the _data param (from calldata) and deployed addresses (from storage) to memory which might be more expensive than re-encodinga and passing the calldata I think

what are your thoughts on this?

uint256 _quorumThresholdBps
) external initializer {
/// @param _data The encoded governor parameters
function initialize(address _treasury, address _token, bytes calldata _data) external initializer {
Copy link
Contributor

Choose a reason for hiding this comment

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

could pass in the generic struct here and _data as a part of that to prevent re-encoding and passing the calldata so many times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To create a struct with the addresses and data parameters we would need to copy the _data param (from calldata) and deployed addresses (from storage) to memory which might be more expensive than re-encodinga and passing the calldata I think

Copy link
Contributor

@iainnash iainnash left a comment

Choose a reason for hiding this comment

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

I want to take more time reviewing this but this is a first pass – would be helpful to not have all the linter changes here (But I think I have a tool to remove them) since this is a fairly large diff.

I'll think about a better way than the tagged array but that approach makes sense – more sense than nulling out a struct. another approach is a struct with an ID in it but that requires more iteration.

src/governance/governor/Governor.sol Show resolved Hide resolved
governorImpl = _governorImpl;
}
// Public constants for implementation types.
// Allows for adding new types later easily compared to a enum.
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe at this point enums under the hood are uint8 – this just makes it more clear when you add items.

auction = address(new ERC1967Proxy{ salt: salt }(auctionImpl, ""));
treasury = address(new ERC1967Proxy{ salt: salt }(treasuryImpl, ""));
governor = address(new ERC1967Proxy{ salt: salt }(governorImpl, ""));
metadata = address(new ERC1967Proxy{ salt: salt }(_implAddresses[IMPLEMENTATION_TYPE_METADATA], ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

salt isn't really needed but i don't think it hurts this – since our impl addresses now can change

src/manager/Manager.sol Outdated Show resolved Hide resolved
@neokry neokry temporarily deployed to Test August 7, 2023 02:31 — with GitHub Actions Inactive
@neokry neokry temporarily deployed to Test August 7, 2023 02:43 — with GitHub Actions Inactive
@neokry neokry temporarily deployed to Test August 7, 2023 02:54 — with GitHub Actions Inactive
@neokry
Copy link
Collaborator Author

neokry commented Aug 7, 2023

I want to take more time reviewing this but this is a first pass – would be helpful to not have all the linter changes here (But I think I have a tool to remove them) since this is a fairly large diff.

I'll think about a better way than the tagged array but that approach makes sense – more sense than nulling out a struct. another approach is a struct with an ID in it but that requires more iteration.

forgot to fix the linter changes. updated now to match the original formatting

@jgeary
Copy link
Contributor

jgeary commented Aug 16, 2023

@neokry havent dug deep into this yet but please run the storage generate script so isImplementation is included in the storage layout.

@neokry neokry temporarily deployed to Test August 17, 2023 03:53 — with GitHub Actions Inactive
uint256 implAddressesLength = _implAddresses.length;

// Ensure implementation parameters are correct length
if (implAddressesLength != IMPLEMENTATION_TYPE_COUNT || _implData.length != IMPLEMENTATION_TYPE_COUNT) revert INVALID_IMPLEMENTATION_PARAMS();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: { and } brackets.

@neokry neokry closed this Oct 20, 2023
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