Skip to content

Add authorizedMint and whitelist Reservoir Relay EOA#146

Merged
nothing0012 merged 3 commits into
me-foundation:mainfrom
channing-magiceden:main
Aug 22, 2024
Merged

Add authorizedMint and whitelist Reservoir Relay EOA#146
nothing0012 merged 3 commits into
me-foundation:mainfrom
channing-magiceden:main

Conversation

@channing-magiceden
Copy link
Copy Markdown
Contributor

  • Add authorizedMint in 1155 and 721
  • Whitelisted Reservoir Relay EOA by default

@channing-magiceden channing-magiceden marked this pull request as ready for review August 21, 2024 09:51
@tenthirtyone
Copy link
Copy Markdown
Contributor

If that EOA is compromised or the relationship with the EOA changes I'm not seeing any protections that can ever remove it. I think the _authorizedMinters mapping needs add/remove methods and a process for managing access to those functions.

What is the motivation for setting the EoA as a constant if it can be passed as a constructor param?

@channing-magiceden
Copy link
Copy Markdown
Contributor Author

If that EOA is compromised or the relationship with the EOA changes I'm not seeing any protections that can ever remove it. I think the _authorizedMinters mapping needs add/remove methods and a process for managing access to those functions.

What is the motivation for setting the EoA as a constant if it can be passed as a constructor param?

Good suggestion. My initial intention is to make Reservior Relay fixed so it cannot be modified. But after some thoughts I think it's better to make the list mutable (by owner only).

@tenthirtyone
Copy link
Copy Markdown
Contributor

Should work.

In the future what do you think about something like AuthorizedMinter.sol and inherit/extend but it's not necessary in this PR, I think.

Copy link
Copy Markdown
Contributor

@tenthirtyone tenthirtyone left a comment

Choose a reason for hiding this comment

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

See comments about AuthorizedMinter.sol in the future. There could probably be some zero address checks but it should be benign/caught by the underlying standard.

@channing-magiceden
Copy link
Copy Markdown
Contributor Author

Should work.

In the future what do you think about something like AuthorizedMinter.sol and inherit/extend but it's not necessary in this PR, I think.

I think we could switch to use OZ's Access Control / Role to consolidate owner and authorizer.

@nothing0012
Copy link
Copy Markdown
Contributor

worth refactoring later, but yeah, lgtm for now, that we delay the introduction of another Roles dependency

@nothing0012 nothing0012 merged commit 7d71165 into me-foundation:main Aug 22, 2024
tenthirtyone pushed a commit to tenthirtyone/magicdrop that referenced this pull request Aug 27, 2024
* Add authorizedMint and whitelist Reservoir Relay EOA

* Add ability to change authorizedMinter list

* pump version
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