-
Notifications
You must be signed in to change notification settings - Fork 42
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
[REG-1430] RegistrarCustody contract #368
base: main
Are you sure you want to change the base?
Conversation
0c9418e
to
ac62b75
Compare
ac62b75
to
457aece
Compare
_disableInitializers(); | ||
} | ||
|
||
function initialize(address _unsRegistry, address _mintingManager) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
_disableInitializers(); | ||
} | ||
|
||
function initialize(address _unsRegistry, address _mintingManager) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
function _msgData() internal view override(ContextUpgradeable, ERC2771RegistryContext) returns (bytes calldata) { | ||
return ERC2771RegistryContext._msgData(); | ||
} |
Check warning
Code scanning / Slither
Dead-code Warning
457aece
to
821990b
Compare
821990b
to
6b6a567
Compare
Contracts size report
|
function safeTransfer(address to, uint256 tokenId) external onlyMinter nonReentrant { | ||
_protectTokenOperation(tokenId); | ||
|
||
unsRegistry.setOwner(to, tokenId); | ||
|
||
delete virtualOwners[tokenId]; | ||
} |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
External calls:
- unsRegistry.setOwner(to,tokenId)
State variables written after the call(s):
- delete virtualOwners[tokenId]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Love the simplicity of RegistrarCustody contract. I know it wasn't easy to keep it simple. Left a few comments.
|
||
virtualOwners[tokenId] = virtualOwner; | ||
|
||
mintingManager.issueExpirableWithRecords(address(this), labels, keys, values, expiry, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to grant RegistrarCustody minter role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
|
||
unsRegistry.setOwner(to, tokenId); | ||
|
||
delete virtualOwners[tokenId]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to delete virtual owner first and call external contract later to avoid reentrancy. I know we have nonReentrant guard there, so this is a minor issue, just a nitpick
|
||
_protectTokenOperation(tokenId); | ||
|
||
virtualOwners[tokenId] = virtualOwner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should smart contract do if it has virtual owner already. I'm not sure there - but we may want to check this case.
} | ||
|
||
function safeTransfer(address to, uint256 tokenId) external onlyMinter nonReentrant { | ||
_protectTokenOperation(tokenId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do if virtualOwner doesn't exist anymore there? May be good to check this and fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it's needed, we can allow claiming from the RegistrarCustody regardless
address signer = hash.recover(signature); | ||
|
||
if (isMinter(signer)) { | ||
return 0x1626ba7e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add comment with explanation or move it to the constant. Like // bytes4(keccak256("isValidSignature(bytes32,bytes)"))
} | ||
|
||
function _msgSender() internal view override(ContextUpgradeable, ERC2771RegistryContext) returns (address) { | ||
return ERC2771RegistryContext._msgSender(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one! It has much more clarity comparing to super._msgSender()
PR Checklist
1. Contracts versioning
patch
version of the contracts is increased if changes have been made to theUNSRegistry
,MintingManager
,ProxyReader
,ENSCustody
contracts.minor
version of the contracts is increased if breaking changes have been made to theUNSRegistry
,MintingManager
,ProxyReader
,ENSCustody
contracts. It includes changes of interfaces.2. Contracts licensing
3. Coverage
4. Configs versioning
uns-config.json
is increased if changes have been made to the config.ens-config.json
is increased if changes have been made to the config.resolver-keys.json
is increased if changes have been made to the config.ens-resolver-keys.json
is increased if changes have been made to the config.5. Package versioning
patch
version of package is increased if valuable changes have been made to the package. It includes contracts update, configs update, etc.major.minor
version of package is synced with version ofUNSRegistry
contract.CHANGELOG
is updated with short description for the new version.6. Code review
resolver-keys.json
code review is required from DevTools teamens-resolver-keys.json
code review is required from DevTools team