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

Clone vouchers using create2 #703

Merged
merged 29 commits into from
Jul 7, 2023
Merged

Clone vouchers using create2 #703

merged 29 commits into from
Jul 7, 2023

Conversation

zajck
Copy link
Member

@zajck zajck commented Jul 4, 2023

Closes #702

Based on #592 which should be merged first.

Changes:

  • use create2 instead of create in cloneBosonVoucher
  • BeaconClientProxy is deployed from the protocol.
  • seller creator is stored and used when computing salt for additional collections

@zajck zajck self-assigned this Jul 4, 2023
@zajck zajck added enhancement New feature or request v2.3.0 labels Jul 4, 2023
@coveralls
Copy link

coveralls commented Jul 5, 2023

Coverage Status

Changes unknown when pulling 1ec9c3c on voucher-create2 into ** on main**.

@zajck zajck requested review from mischat and anajuliabit July 6, 2023 18:44
mischat
mischat previously approved these changes Jul 6, 2023
Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

mischat
mischat previously approved these changes Jul 7, 2023
Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

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

I am happy with this, i have a question which is merely informational for me

contracts/protocol/bases/SellerBase.sol Show resolved Hide resolved
contracts/protocol/bases/SellerBase.sol Show resolved Hide resolved
Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

Looks good. I added some suggestions, but nothing that has an impact on contracts.

scripts/util/deploy-protocol-clients.js Show resolved Hide resolved
@@ -743,6 +837,73 @@ describe("ProtocolInitializationHandler", async function () {
).to.be.revertedWith(RevertReasons.VALUE_ZERO_NOT_ALLOWED);
});

it("sellerIds and sellerCreators length mismatch", async function () {
// set invalid minResolutionPeriod
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// set invalid minResolutionPeriod

});

it("invalid seller id ", async function () {
// set invalid minResolutionPeriod
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// set invalid minResolutionPeriod

test/protocol/ProtocolInitializationHandlerTest.js Outdated Show resolved Hide resolved
Comment on lines +267 to +271
function getCloneByteCodeHash(beaconProxyAddress) {
return keccak256(
`0x3d602d80600a3d3981f3363d3d373d3d3d363d73${beaconProxyAddress.slice(2)}5af43d82803e903d91602b57fd5bf3`
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

where did you get this value? would be nice to have some comments here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it from our contracts, but otherwise this is EIP 1167 style minimal clone.

We use the code from this example.

OZ uses the same: https://blog.openzeppelin.com/deep-dive-into-the-minimal-proxy-contract

anajuliabit
anajuliabit previously approved these changes Jul 7, 2023
Co-authored-by: Ana Julia Bittencourt  <anajuliabit@gmail.com>
@mischat mischat merged commit c6d0e08 into main Jul 7, 2023
@mischat mischat deleted the voucher-create2 branch July 7, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use create2 instead of create in cloneBosonVoucher
4 participants