WIP: Create the first version of Billboard contracts #119
Conversation
src/Billboard/IBillboardRegistry.sol
Outdated
| string description; | ||
| string location; | ||
| string contentURI; | ||
| string redirectLink; |
src/Billboard/Billboard.sol
Outdated
|
|
||
| /// @inheritdoc IBillboard | ||
| function setIsOpened(bool value_) external isAdmin(msg.sender) { | ||
| if (address(registry) == address(0)) { |
There was a problem hiding this comment.
L61-L66 can use isValidAddress instead?
There was a problem hiding this comment.
or we don't need L61-L66 actually since the transaction will be failed if registry or auction are zero addresses.
There was a problem hiding this comment.
You're right tx will be failed. Removed L61-L66.
src/Billboard/BillboardRegistry.sol
Outdated
| modifier isValidBoard(uint256 tokenId_) { | ||
| uint256 latestId = tokenIds.current(); | ||
|
|
||
| if (tokenId_ < 1 || tokenId_ > latestId) { |
There was a problem hiding this comment.
If there is L49, then this is no needed?
There was a problem hiding this comment.
Remove the first validation since ownership checker is added
| assertEq(operatorAddress, registry.operator()); | ||
|
|
||
| operator.upgradeAuction(address(auction)); | ||
| operator.upgradeRegistry(address(registry)); |
There was a problem hiding this comment.
assetEq to check operator.auction and operator.registry?
There was a problem hiding this comment.
Added assertion for auction and registry upgradability
src/Billboard/BillboardAuction.sol
Outdated
| } | ||
|
|
||
| modifier isFromOperator() { | ||
| if (operator == address(0)) { |
There was a problem hiding this comment.
Zero address checkers (L62-L67) seem unnecessary.
src/Billboard/BillboardRegistry.sol
Outdated
| } | ||
|
|
||
| modifier isFromOperator() { | ||
| if (operator == address(0)) { |
There was a problem hiding this comment.
Zero address checkers (L83-L88) seem unnecessary.
| assertEq("", board.description); | ||
| assertEq("", board.location); | ||
| assertEq("", board.contentURI); | ||
| assertEq("", board.redirectLink); |
There was a problem hiding this comment.
test mint more than one token and check sequential token id?
There was a problem hiding this comment.
Added one more minting test case.
src/Billboard/IBillboardRegistry.sol
Outdated
| ////////////////////////////// | ||
|
|
||
| struct Board { | ||
| address owner; |
There was a problem hiding this comment.
i was thinking that maybe owner is not a good name since it's easily confused with token.ownerOf.
There was a problem hiding this comment.
Rename owner to creator (better than minter).
|
forked to #120 |
Create the basic version of Billboard contracts: