Modify transfer in DomainToken #119
Conversation
…n-standard transfer to transfer token only
WIP: Modify transfer in DomainToken
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| @@ -34,6 +34,8 @@ contract ZNSAccessController is AccessControl, ZNSRoles, IZNSAccessController { | |||
| _setRoleAdmin(GOVERNOR_ROLE, GOVERNOR_ROLE); | |||
| // all of the admins control registrar | |||
| _setRoleAdmin(REGISTRAR_ROLE, ADMIN_ROLE); | |||
| // all of the admins control domain token | |||
| _setRoleAdmin(DOMAIN_TOKEN_ROLE, ADMIN_ROLE); | |||
There was a problem hiding this comment.
hmm.... this will require us to upgrade every single contract in ZNS for this change to work? is it possible to do this in a less invasive way?
| /** | ||
| * @notice This role is here specifically for the ZNSDomainToken.sol contract | ||
| */ | ||
| bytes32 public constant DOMAIN_TOKEN_ROLE = keccak256("DOMAIN_TOKEN_ROLE"); |
There was a problem hiding this comment.
can't we just use Registrar role for this? this way we don't need to upgrade all contracts just for this string...
thoughts?
There was a problem hiding this comment.
actually... we may have to upgrade all contracts anyway since they all have custom errors now...
There was a problem hiding this comment.
It's sort of a decision on the "proper" way this should be. It would definitely be the same effect to just assign the DomainToken contract the REGISTRAR_ROLE but it isn't a registrar, so it felt improper. If we have to upgrade regardless it might make sense to leave it. Let me know what you think
contracts/token/IZNSDomainToken.sol
Outdated
| @@ -55,11 +58,15 @@ interface IZNSDomainToken is IERC2981, IERC721 { | |||
|
|
|||
| function setDefaultRoyalty(address receiver, uint96 royaltyFraction) external; | |||
|
|
|||
| function transferTokenFrom(address from, address to, uint256 tokenId) external; | |||
There was a problem hiding this comment.
maybe make a more clear name?...
idk... "changeTokenOwner()` or something... "transferTokenFrom" is kind of a good name, but may be a bit confusing?
There was a problem hiding this comment.
I felt making it as close to the regular transferFrom was best, so transferTokenFrom made sense because it was transferring just the token, but maybe setTokenOwner or updateTokenOwner to match verbiage from the registry would be better
| address from, | ||
| address to, | ||
| uint256 tokenId | ||
| ) public override(ERC721Upgradeable, IERC721) { |
There was a problem hiding this comment.
hm... what about all other transfer methods? there used to be 3 at least. those will not do what we want.
There was a problem hiding this comment.
Messaged you about this, but yes because the other two public safeTransferFrom both route through this function it's fine to just override this one.
…into feat/modify-transfers
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## zns-zchain-final #119 +/- ##
=================================================
Coverage 99.81% 99.82%
=================================================
Files 12 12
Lines 547 560 +13
Branches 122 123 +1
=================================================
+ Hits 546 559 +13
Misses 1 1 |
JamesEarle
changed the title
…st for use of safeTransferFrom checks
Modify the original
transferFrommethod to also include updating the registry with the owner update as well, effectively tying the split ownership together in all standard transfers such as sales on OpenSeaIntroduce a new non-standard transfer
transferTokenFromthat behaves similarly to the originaltransferFromwhere only the token is transferred.