[v1.5] SubdomainToken contract#136
[v1.5] SubdomainToken contract#136MichaelKorchagin wants to merge 8 commits intorc/zchain-native-mainfrom
Conversation
[v1.5] SubdomainToken contract
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #136 +/- ##
===============================================
- Coverage 99.80% 94.26% -5.55%
===============================================
Files 11 13 +2
Lines 512 610 +98
Branches 127 129 +2
===============================================
+ Hits 511 575 +64
- Misses 1 35 +34 🚀 New features to boost your workflow:
|
… get token URI. Overrided #transferFrom (WIP). Added domainToken #revoke function.
…oken with setRoyalty function.
There was a problem hiding this comment.
If files show like this, where the entire file is changed, it is usually because of the line endings for the file changing and it makes it really hard to review what actually changed. Please always use LF not CRLF to be consistent with the repo
| _setRegistry(registry_); | ||
| } | ||
|
|
||
| function register(address to, uint256 tokenId, string memory _tokenURI) |
There was a problem hiding this comment.
To remain consistent with the other functions in contracts we define in the repository, when line breaks are necessary put them within parameter lists instead.
function myFunc(
address var1,
address var2,
uint256 var3
) external {
...
}
| _setTokenURI(tokenId, _tokenURI); | ||
| } | ||
|
|
||
| function revoke(uint256 tokenId) |
There was a problem hiding this comment.
for functions without long signatures like this, it isn't necessary to break the signature into multiple lines.
function revoke(uint256 tokenID) external {
...
}
| } | ||
|
|
||
| // TODO: add access control, when it's ready | ||
| // TODO: do we check the passed string? |
There was a problem hiding this comment.
No we don't check this on-chain
| function revoke(uint256 tokenId) | ||
| external | ||
| { | ||
| _burn(tokenId); |
There was a problem hiding this comment.
Be sure access control is on this function as well, when it is ready to be implemented.
| super.transferFrom(from, to, tokenId); | ||
|
|
||
| // TODO: make a fucntion or update existing one in the registry | ||
| // registry.updateDomainOwner(bytes32(abi.encodePacked(tokenId)), to); |
There was a problem hiding this comment.
Should this be uncommented?
| emit TokenURISet(tokenId, _tokenURI); | ||
| } | ||
|
|
||
| function transferFrom( |
There was a problem hiding this comment.
also be sure to include AC here when ready
|
|
||
| string private baseURI; | ||
|
|
||
| event TokenURISet(uint256 indexed tokenId, string tokenURI); |
There was a problem hiding this comment.
Put events in the interface IZNSSubdomainToken
There was a problem hiding this comment.
After addressing other things I've pointed out, add natspec comments
| EIP712, | ||
| ERC721Votes, | ||
| ERC2981, | ||
| ARegistryWired { |
There was a problem hiding this comment.
why doesn't this just inherit the ZNSDomainToken contract? It seems to have almost the same implementation. is there a reason you chose to make this separate?
… visibilities and overrides.
…rage variables to it.
3 participants
No description provided.