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

Added new package hyperverse-evm-mintpass #84

Open
wants to merge 1 commit into
base: feat/builderkit
Choose a base branch
from

Conversation

0xtp
Copy link

@0xtp 0xtp commented Apr 19, 2022

Added new package hyperverse-evm-mintpass. This package includes MintPass, MintPassFactory, CloneFactory contracts, hardhat test scripts, etc.

related #83

Copy link
Contributor

@gelicamarie gelicamarie left a comment

Choose a reason for hiding this comment

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

are you able to switch all the requires to use the new Solidity custom errors? Thank you!

Here are some references:
https://blog.soliditylang.org/2021/04/21/custom-errors/


contractOwner = msg.sender;

metadata = ModuleMetadata(
'Token',
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be changed to MintPass

Suggested change
'Token',
'MintPass',

*/

contract MintPassFactory is CloneFactory {
/*@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ S T A T E @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@*/
Copy link
Contributor

Choose a reason for hiding this comment

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

lets add a tenant counter to keep track of how many tenants we have

_;
}

modifier isAllowedToCreateInstance(address _tenant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename this to isAuthorized

Suggested change
modifier isAllowedToCreateInstance(address _tenant) {
modifier isAuthorized(address _tenant) {

address private hyperverseAdmin = 0xD847C7408c48b6b6720CCa75eB30a93acbF5163D;

/*@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ M O D I F I E R S @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@*/
modifier isOwner(address _tenant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this modifier here, but let's add a hasAnInstance modifier and use it in createInstance

}

/*@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ E V E N T S @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@*/

Copy link
Contributor

Choose a reason for hiding this comment

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

lets add a TenantCreated Event

Suggested change
event TenantCreated(address _tenant, address _proxy);

and emit it on createInstance


/*@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ F U N C T I O N S @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@*/

function createInstance(string memory name, string memory symbol) external {
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
function createInstance(string memory name, string memory symbol) external {
function createInstance(address _tenant, string memory _name, string memory _symbol) external {

lets keep the convention of using _ for function params

/*@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ F U N C T I O N S @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@*/

function createInstance(string memory name, string memory symbol) external {
address tenant = msg.sender;
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't set tenant as the msg.sender since we are allowing a hyperverseAdmin to createInstance for others

string public _symbol;

uint256 public tokenCounter;
bool public initialized;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can get rid of this initialized variable if we just use a modifier to check

reference:

modifier canInitialize(address _tenant) {

import "@openzeppelin/contracts/access/Ownable.sol";

contract MintPass is ERC1155, Ownable, IHyperverseModule {
string public _name;
Copy link
Contributor

Choose a reason for hiding this comment

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

since _name and _symbol are public state variables let's remove the _

return tokenURI[_id];
}

function name() public view returns (string memory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can get rid of the name and symbol getters again since name and symbol are public state vars since Solidity creates those getter for us

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.

2 participants