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

Module Core Refactor #145

Merged
merged 20 commits into from
Sep 4, 2024
Merged

Module Core Refactor #145

merged 20 commits into from
Sep 4, 2024

Conversation

GWSzeto
Copy link
Contributor

@GWSzeto GWSzeto commented Aug 31, 2024

No description provided.

bytes memory signature
) external payable {
address signer = _hashTypedData(
keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC1155, to, tokenId, value, keccak256(bytes(baseURI)), data))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keccack256 the data part here

keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC1155, to, tokenId, value, keccak256(bytes(baseURI)), data))
).recover(signature);

if (!OwnableRoles(address(this)).hasAllRoles(signer, Role._MINTER_ROLE)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove the OwnableRoles(address(this)) part as it's already the core contract

bytes memory signature
) external payable {
address signer = _hashTypedData(
keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC1155, to, tokenId, value, keccak256(bytes(baseURI)), data))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keccack256(data)

keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC721, to, quantity, keccak256(bytes(baseURI)), data))
).recover(signature);

if (!OwnableRoles(address(this)).hasAllRoles(signer, Role._MINTER_ROLE)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove OwnableRoles(address(this))

bytes memory signature
) external payable {
address signer = _hashTypedData(
keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC721, to, quantity, keccak256(bytes(baseURI)), data))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keccack256(Data)

bytes memory signature
) external payable {
address signer = _hashTypedData(
keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC721, to, quantity, keccak256(bytes(baseURI)), data))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keccack256(data)

keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC721, to, quantity, keccak256(bytes(baseURI)), data))
).recover(signature);

if (!OwnableRoles(address(this)).hasAllRoles(signer, Role._MINTER_ROLE)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove

_batchMetadataStorage().tokenIdRangeEnd.push(rangeEndNonInclusive);
_batchMetadataStorage().baseURIOfTokenIdRange[rangeEndNonInclusive] = _baseURI;

emit NewMetadataBatch(rangeStart, rangeEndNonInclusive, _baseURI);
Copy link
Member

Choose a reason for hiding this comment

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

remove this, "BatchMetadataUpdate" is part of the spec

keccak256(abi.encode(TYPEHASH_SIGNATURE_MINT_ERC1155, to, tokenId, value, keccak256(bytes(baseURI)), data))
).recover(signature);

if (!OwnableRoles(address(this)).hasAllRoles(signer, Role._MINTER_ROLE)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to module

_batchMetadataStorage().baseURIOfTokenIdRange[rangeEndNonInclusive] = _baseURI;

emit NewMetadataBatch(rangeStart, rangeEndNonInclusive, _baseURI);
emit BatchMetadataUpdate(rangeStart, rangeEndNonInclusive - 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look into the spec around this event UpdateMetadata spec

contract ClaimableERC1155 is Module, EIP712, BeforeMintCallbackERC1155, IInstallationCallback {
contract ClaimableERC1155 is
Module,
EIP712,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this

* @param data ABI encoded data to pass to the beforeMint hook.
*/
function mint(address to, uint256 tokenId, uint256 value, bytes memory data) external payable {
function mint(address to, uint256 tokenId, uint256 value, string calldata baseURI, bytes memory data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename to either amount or quantity, just make the naming consistent

@@ -86,11 +93,8 @@ contract ClaimableERC1155 is Module, EIP712, BeforeMintCallbackERC1155, IInstall
* @param uid A unique identifier for the minting request.
*/
struct ClaimRequestERC1155 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ClaimSignatureParams

@GWSzeto GWSzeto changed the title Erc 1155 refactor Module Core Refactor Sep 4, 2024
@@ -1,7 +1,11 @@
// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.20;

import "forge-std/console.sol";
Copy link
Member

Choose a reason for hiding this comment

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

remove

_mint(to, tokenId, amount, "");
}

receive() external payable {}
Copy link
Member

Choose a reason for hiding this comment

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

remove

)
).recover(signature);
console.log("signer");
console.logAddress(signer);
Copy link
Member

Choose a reason for hiding this comment

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

cleanup

TYPEHASH_SIGNATURE_MINT_ERC1155, to, tokenId, amount, keccak256(bytes(baseURI)), keccak256(data)
)
)
).recover(signature);
Copy link
Member

Choose a reason for hiding this comment

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

you're not checking that the signer has minter role here?

// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.20;

contract BeforeMintWithSignatureCallbackERC1155 {
Copy link
Member

Choose a reason for hiding this comment

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

kinda weird that these are not abstract/interfaces

@GWSzeto GWSzeto merged commit 76655aa into main Sep 4, 2024
5 checks passed
@GWSzeto GWSzeto deleted the ERC-1155-refactor branch September 4, 2024 02:34
@kumaryash90 kumaryash90 restored the ERC-1155-refactor branch September 4, 2024 16:03
@joaquim-verges joaquim-verges deleted the ERC-1155-refactor branch September 4, 2024 22:17
GWSzeto added a commit that referenced this pull request Sep 10, 2024
* separated out metadata functionality from mintable module

* built in signature mint into the core erc721 contract

* implemented in Mintable

* Implemented updateMetadata

* simplified parameters and structs

* all tests pass

* updated ERC721 initializable to match ERC721Core

* addressed the PR issues

* updated 1155 versions to now match 721 implementations

* completed all the tests

* Implemented parity in the ERC1155Initializable contract

* unified naming from quantity and value to amount

* slapped on a keccak256

* move OwnableRoles check on the signature

* removed double events being emitted

* tests pass

* updated ERC20 core

* implemented Claimable and Mintable on the ERC20 side

* tests pass

* updated based on PR feedback
GWSzeto added a commit that referenced this pull request Sep 11, 2024
* [L-4] Wrong function selector returned for the transfer validation function (#150)

* [L-4] Wrong function selector returned for the transfer validation function

* [Q-3] Move interface identifier for ERC165 to Core

* [Q-5] Royalty modules should inherit ICreatorToken interface

* [Q-6] Nitpicks

* removes duplicate supportsInterface (#156)

* renamed to nextTokenIdToMint in BatchMetadata (#141)

* fix: expectRevert on low-level call (#136)

* update batchMetadata logic to make each batchUris independent (#144)

* update batchMetadata logic to make each batchUris independent

* format

* Module Core Refactor (#145)

* separated out metadata functionality from mintable module

* built in signature mint into the core erc721 contract

* implemented in Mintable

* Implemented updateMetadata

* simplified parameters and structs

* all tests pass

* updated ERC721 initializable to match ERC721Core

* addressed the PR issues

* updated 1155 versions to now match 721 implementations

* completed all the tests

* Implemented parity in the ERC1155Initializable contract

* unified naming from quantity and value to amount

* slapped on a keccak256

* move OwnableRoles check on the signature

* removed double events being emitted

* tests pass

* updated ERC20 core

* implemented Claimable and Mintable on the ERC20 side

* tests pass

* updated based on PR feedback

* Fix getSupportedCallbackFunctions ub ERC721CoreInitializable (#149)

* implmented delayed functionality into batchMetadata (#148)

* implmented delayed functionality into batchMetadata

* created tests for BatchMetadata

* updated ERC1155 tests and updated from batchStartId to batchRange

* Implement tokenIdERC1155 module to handle tokenId management (#147)

* initial commit

* created tests for tokenIdERC1155

* updated to be optional

* updated naming and tests

* Transfer validator has roles (#143)

* created hasRole function in the roylaty module

* created tests

* Optimzed callback execution (#135)

* gas benchmark

* optimize execute callback function

* fix typo

* optimize execute callback view function

* optimize callback mode loop

* Implement Max per wallet (#151)

* implemented maxMintPerWallet

* tests pass

* maxMintPerWalletExceeded tests pass

* introduced base contracts for core and initilizable to inherit

* rename commit

* rename commit

* renamed from core to coreInitializable for the ERC1155 (#152)

* updated to now use 1e18 divided (#153)

* Remove double initializer in ERC721CoreInitializable (#154)

---------

Co-authored-by: Pranav Garg <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>
GWSzeto added a commit that referenced this pull request Sep 12, 2024
* renamed to nextTokenIdToMint in BatchMetadata (#141)

* fix: expectRevert on low-level call (#136)

* update batchMetadata logic to make each batchUris independent (#144)

* update batchMetadata logic to make each batchUris independent

* format

* Module Core Refactor (#145)

* separated out metadata functionality from mintable module

* built in signature mint into the core erc721 contract

* implemented in Mintable

* Implemented updateMetadata

* simplified parameters and structs

* all tests pass

* updated ERC721 initializable to match ERC721Core

* addressed the PR issues

* updated 1155 versions to now match 721 implementations

* completed all the tests

* Implemented parity in the ERC1155Initializable contract

* unified naming from quantity and value to amount

* slapped on a keccak256

* move OwnableRoles check on the signature

* removed double events being emitted

* tests pass

* updated ERC20 core

* implemented Claimable and Mintable on the ERC20 side

* tests pass

* updated based on PR feedback

* Fix getSupportedCallbackFunctions ub ERC721CoreInitializable (#149)

* implmented delayed functionality into batchMetadata (#148)

* implmented delayed functionality into batchMetadata

* created tests for BatchMetadata

* updated ERC1155 tests and updated from batchStartId to batchRange

* Implement tokenIdERC1155 module to handle tokenId management (#147)

* initial commit

* created tests for tokenIdERC1155

* updated to be optional

* updated naming and tests

* Transfer validator has roles (#143)

* created hasRole function in the roylaty module

* created tests

* Optimzed callback execution (#135)

* gas benchmark

* optimize execute callback function

* fix typo

* optimize execute callback view function

* optimize callback mode loop

* Implement Max per wallet (#151)

* implemented maxMintPerWallet

* tests pass

* maxMintPerWalletExceeded tests pass

* introduced base contracts for core and initilizable to inherit

* rename commit

* rename commit

* renamed from core to coreInitializable for the ERC1155 (#152)

* updated to now use 1e18 divided (#153)

* Remove double initializer in ERC721CoreInitializable (#154)

* rebase off of main (#157)

* [L-4] Wrong function selector returned for the transfer validation function (#150)

* [L-4] Wrong function selector returned for the transfer validation function

* [Q-3] Move interface identifier for ERC165 to Core

* [Q-5] Royalty modules should inherit ICreatorToken interface

* [Q-6] Nitpicks

* removes duplicate supportsInterface (#156)

* renamed to nextTokenIdToMint in BatchMetadata (#141)

* fix: expectRevert on low-level call (#136)

* update batchMetadata logic to make each batchUris independent (#144)

* update batchMetadata logic to make each batchUris independent

* format

* Module Core Refactor (#145)

* separated out metadata functionality from mintable module

* built in signature mint into the core erc721 contract

* implemented in Mintable

* Implemented updateMetadata

* simplified parameters and structs

* all tests pass

* updated ERC721 initializable to match ERC721Core

* addressed the PR issues

* updated 1155 versions to now match 721 implementations

* completed all the tests

* Implemented parity in the ERC1155Initializable contract

* unified naming from quantity and value to amount

* slapped on a keccak256

* move OwnableRoles check on the signature

* removed double events being emitted

* tests pass

* updated ERC20 core

* implemented Claimable and Mintable on the ERC20 side

* tests pass

* updated based on PR feedback

* Fix getSupportedCallbackFunctions ub ERC721CoreInitializable (#149)

* implmented delayed functionality into batchMetadata (#148)

* implmented delayed functionality into batchMetadata

* created tests for BatchMetadata

* updated ERC1155 tests and updated from batchStartId to batchRange

* Implement tokenIdERC1155 module to handle tokenId management (#147)

* initial commit

* created tests for tokenIdERC1155

* updated to be optional

* updated naming and tests

* Transfer validator has roles (#143)

* created hasRole function in the roylaty module

* created tests

* Optimzed callback execution (#135)

* gas benchmark

* optimize execute callback function

* fix typo

* optimize execute callback view function

* optimize callback mode loop

* Implement Max per wallet (#151)

* implemented maxMintPerWallet

* tests pass

* maxMintPerWalletExceeded tests pass

* introduced base contracts for core and initilizable to inherit

* rename commit

* rename commit

* renamed from core to coreInitializable for the ERC1155 (#152)

* updated to now use 1e18 divided (#153)

* Remove double initializer in ERC721CoreInitializable (#154)

---------

Co-authored-by: Pranav Garg <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>

* Duplicate constant (#161)

* [L-4] Wrong function selector returned for the transfer validation function (#150)

* [L-4] Wrong function selector returned for the transfer validation function

* [Q-3] Move interface identifier for ERC165 to Core

* [Q-5] Royalty modules should inherit ICreatorToken interface

* [Q-6] Nitpicks

* removes duplicate supportsInterface (#156)

* Has roles (#158)

* created hasRole function in the roylaty module

* created tests

* removed duplicate constant in royaltyERC721

* removed .vscode

* case sensitivity issue

---------

Co-authored-by: Pranav Garg <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>
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