Skip to content

Commit

Permalink
feat(billboard): use "require" instead of custom errors
Browse files Browse the repository at this point in the history
  • Loading branch information
robertu7 committed Nov 23, 2023
1 parent 7e28b4b commit ff9409a
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 148 deletions.
89 changes: 45 additions & 44 deletions .gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -10,53 +10,54 @@ ACLManagerTest:testRenounceRole() (gas: 27841)
ACLManagerTest:testRoles() (gas: 15393)
ACLManagerTest:testTransferRole() (gas: 21528)
BillboardTest:testAddToWhitelist() (gas: 37205)
BillboardTest:testApproveAndTransfer() (gas: 162759)
BillboardTest:testApproveAndTransfer() (gas: 162735)
BillboardTest:testCalculateTax() (gas: 29439)
BillboardTest:testCannnotWithdrawTaxIfSmallAmount(uint8) (runs: 256, μ: 420531, ~: 434362)
BillboardTest:testCannnotWithdrawTaxIfZero() (gas: 386565)
BillboardTest:testCannotAddToWhitelistByAttacker() (gas: 11794)
BillboardTest:testCannotApproveByAttacker() (gas: 130412)
BillboardTest:testCannotClearAuctionIfAuctionNotEnded() (gas: 604617)
BillboardTest:testCannotClearAuctionOnNewBoard() (gas: 136255)
BillboardTest:testCannotMintBoardByAttacker() (gas: 13993)
BillboardTest:testCannotPlaceBidByAttacker() (gas: 142428)
BillboardTest:testCannotPlaceBidTwice(uint96) (runs: 256, μ: 650981, ~: 657212)
BillboardTest:testCannotRemoveToWhitelistByAttacker() (gas: 11861)
BillboardTest:testCannotSafeTransferByAttacker() (gas: 128214)
BillboardTest:testCannotSetBoardProprtiesByAttacker() (gas: 159548)
BillboardTest:testCannotSetIsOpenedByAttacker() (gas: 11751)
BillboardTest:testCannotSetTaxRateByAttacker() (gas: 11763)
BillboardTest:testCannotTransferByOperator() (gas: 132890)
BillboardTest:testCannotTransferToZeroAddress() (gas: 128377)
BillboardTest:testCannotUpgradeRegistryByAttacker() (gas: 11885)
BillboardTest:testCannotWithBidTwice(uint96) (runs: 256, μ: 934696, ~: 934696)
BillboardTest:testCannotWithdrawBidIfAuctionNotCleared(uint96) (runs: 256, μ: 771287, ~: 771287)
BillboardTest:testCannotWithdrawBidIfAuctionNotEnded(uint96) (runs: 256, μ: 646346, ~: 646346)
BillboardTest:testCannotWithdrawBidIfNotFound() (gas: 419293)
BillboardTest:testCannotWithdrawBidIfWon(uint96) (runs: 256, μ: 741144, ~: 741144)
BillboardTest:testCannotWithdrawTaxByAttacker() (gas: 21710)
BillboardTest:testClearAuctionIfAuctionEnded() (gas: 649643)
BillboardTest:testClearAuctionsIfAuctionEnded() (gas: 1206344)
BillboardTest:testGetBids(uint8,uint8,uint8) (runs: 256, μ: 2790498, ~: 1448206)
BillboardTest:testGetTokenURI() (gas: 155305)
BillboardTest:testMintBoard() (gas: 225991)
BillboardTest:testMintBoardByWhitelist() (gas: 157825)
BillboardTest:testMintBoardIfOpened() (gas: 130863)
BillboardTest:testPlaceBidByWhitelist() (gas: 470072)
BillboardTest:testPlaceBidIfAuctionEnded() (gas: 937111)
BillboardTest:testPlaceBidOnNewBoard(uint96) (runs: 256, μ: 519355, ~: 529681)
BillboardTest:testPlaceBidWithHigherPrice(uint96) (runs: 256, μ: 766973, ~: 774047)
BillboardTest:testPlaceBidWithSamePrices(uint96) (runs: 256, μ: 764899, ~: 776575)
BillboardTest:testPlaceBidZeroPrice() (gas: 357780)
BillboardTest:testRemoveToWhitelist() (gas: 24939)
BillboardTest:testSafeTransferByOperator() (gas: 141290)
BillboardTest:testSetBoardProperties() (gas: 305548)
BillboardTest:testSetBoardPropertiesAfterTransfer() (gas: 338157)
BillboardTest:testCannnotWithdrawTaxIfSmallAmount(uint8) (runs: 256, μ: 419677, ~: 433681)
BillboardTest:testCannnotWithdrawTaxIfZero() (gas: 385862)
BillboardTest:testCannotAddToWhitelistByAttacker() (gas: 11137)
BillboardTest:testCannotApproveByAttacker() (gas: 130388)
BillboardTest:testCannotClearAuctionIfAuctionNotEnded() (gas: 604134)
BillboardTest:testCannotClearAuctionOnNewBoard() (gas: 136029)
BillboardTest:testCannotMintBoardByAttacker() (gas: 13332)
BillboardTest:testCannotPlaceBidByAttacker() (gas: 141747)
BillboardTest:testCannotPlaceBidTwice(uint96) (runs: 256, μ: 650687, ~: 656918)
BillboardTest:testCannotRemoveToWhitelistByAttacker() (gas: 11204)
BillboardTest:testCannotSafeTransferByAttacker() (gas: 127555)
BillboardTest:testCannotSetBoardProprtiesByAttacker() (gas: 156298)
BillboardTest:testCannotSetIsOpenedByAttacker() (gas: 11094)
BillboardTest:testCannotSetTaxRateByAttacker() (gas: 11106)
BillboardTest:testCannotTransferByOperator() (gas: 132888)
BillboardTest:testCannotTransferToZeroAddress() (gas: 128375)
BillboardTest:testCannotUpgradeRegistryByAttacker() (gas: 11228)
BillboardTest:testCannotWithBidTwice(uint96) (runs: 256, μ: 934036, ~: 934036)
BillboardTest:testCannotWithdrawBidIfAuctionNotCleared(uint96) (runs: 256, μ: 770628, ~: 770628)
BillboardTest:testCannotWithdrawBidIfAuctionNotEnded(uint96) (runs: 256, μ: 645425, ~: 645425)
BillboardTest:testCannotWithdrawBidIfNotFound() (gas: 419086)
BillboardTest:testCannotWithdrawBidIfWon(uint96) (runs: 256, μ: 740484, ~: 740484)
BillboardTest:testCannotWithdrawTaxByAttacker() (gas: 21053)
BillboardTest:testClearAuctionIfAuctionEnded() (gas: 649619)
BillboardTest:testClearAuctionsIfAuctionEnded() (gas: 1206340)
BillboardTest:testGetBids(uint8,uint8,uint8) (runs: 256, μ: 2871675, ~: 1453869)
BillboardTest:testGetTokenURI() (gas: 155303)
BillboardTest:testMintBoard() (gas: 225987)
BillboardTest:testMintBoardByWhitelist() (gas: 157167)
BillboardTest:testMintBoardIfOpened() (gas: 130861)
BillboardTest:testPlaceBidByWhitelist() (gas: 470026)
BillboardTest:testPlaceBidIfAuctionEnded() (gas: 937087)
BillboardTest:testPlaceBidOnNewBoard(uint96) (runs: 256, μ: 519419, ~: 529745)
BillboardTest:testPlaceBidWithHigherPrice(uint96) (runs: 256, μ: 766815, ~: 774045)
BillboardTest:testPlaceBidWithSamePrices(uint96) (runs: 256, μ: 764942, ~: 776618)
BillboardTest:testPlaceBidZeroPrice() (gas: 357801)
BillboardTest:testRemoveToWhitelist() (gas: 24957)
BillboardTest:testSafeTransferByOperator() (gas: 141354)
BillboardTest:testSetBoardProperties() (gas: 305524)
BillboardTest:testSetBoardPropertiesAfterTransfer() (gas: 334252)
BillboardTest:testSetIsOpened() (gas: 15978)
BillboardTest:testSetTaxRate() (gas: 27263)
BillboardTest:testUpgradeRegistry() (gas: 2662891)
BillboardTest:testWithdrawBid(uint96) (runs: 256, μ: 935334, ~: 935334)
BillboardTest:testWithdrawTax(uint96) (runs: 256, μ: 509585, ~: 509585)
BillboardTest:testSomethin() (gas: 1698254)
BillboardTest:testUpgradeRegistry() (gas: 2725063)
BillboardTest:testWithdrawBid(uint96) (runs: 256, μ: 935332, ~: 935332)
BillboardTest:testWithdrawTax(uint96) (runs: 256, μ: 509539, ~: 509539)
CurationTest:testCannotCurateERC20CurateZeroAmount() (gas: 12194)
CurationTest:testCannotCurateERC20EmptyURI() (gas: 15797)
CurationTest:testCannotCurateERC20IfNotApproval() (gas: 21624)
Expand Down
58 changes: 21 additions & 37 deletions src/Billboard/Billboard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,31 +32,23 @@ contract Billboard is IBillboard {
//////////////////////////////

modifier isFromAdmin() {
if (msg.sender != admin) {
revert Unauthorized("admin");
}
require(msg.sender == admin, "Admin");
_;
}

modifier isFromWhitelist() {
if (!whitelist[msg.sender]) {
revert Unauthorized("whitelist");
}
require(whitelist[msg.sender], "Whitelist");
_;
}

modifier isFromBoardCreator(uint256 tokenId_) {
(address _boardCreator, , , , , ) = registry.boards(tokenId_);
if (_boardCreator != msg.sender) {
revert Unauthorized("creator");
}
require(_boardCreator == msg.sender, "Creator");
_;
}

modifier isFromBoardTenant(uint256 tokenId_) {
if (msg.sender != registry.ownerOf(tokenId_)) {
revert Unauthorized("tenant");
}
require(msg.sender == registry.ownerOf(tokenId_), "Tenant");
_;
}

Expand Down Expand Up @@ -94,11 +86,9 @@ contract Billboard is IBillboard {

/// @inheritdoc IBillboard
function mintBoard(address to_) external returns (uint256 tokenId) {
if (isOpened || whitelist[msg.sender]) {
tokenId = registry.mintBoard(to_);
} else {
revert Unauthorized("whitelist");
}
require(isOpened || whitelist[msg.sender], "Whitelist");

tokenId = registry.mintBoard(to_);
}

/// @inheritdoc IBillboard
Expand Down Expand Up @@ -147,16 +137,16 @@ contract Billboard is IBillboard {
function clearAuction(uint256 tokenId_) public {
// revert if board not found
(address _boardCreator, , , , , ) = registry.boards(tokenId_);
if (_boardCreator == address(0)) revert BoardNotFound();
require(_boardCreator != address(0), "Board not found");

// revert if it's a new board
uint256 _nextAuctionId = registry.nextBoardAuctionId(tokenId_);
if (_nextAuctionId == 0) revert AuctionNotFound();
require(_nextAuctionId != 0, "Auction not found");

IBillboardRegistry.Auction memory _nextAuction = registry.getAuction(tokenId_, _nextAuctionId);

// revert if auction is still running
if (block.timestamp < _nextAuction.endAt) revert AuctionNotEnded();
require(block.timestamp >= _nextAuction.endAt, "Auction not ended");

// reclaim ownership to board creator if no auction
address _prevOwner = registry.ownerOf(tokenId_);
Expand Down Expand Up @@ -211,7 +201,7 @@ contract Billboard is IBillboard {
/// @inheritdoc IBillboard
function placeBid(uint256 tokenId_, uint256 amount_) external payable isFromWhitelist {
(address _boardCreator, , , , , ) = registry.boards(tokenId_);
if (_boardCreator == address(0)) revert BoardNotFound();
require(_boardCreator != address(0), "Board not found");

uint256 _nextAuctionId = registry.nextBoardAuctionId(tokenId_);
IBillboardRegistry.Auction memory _nextAuction = registry.getAuction(tokenId_, _nextAuctionId);
Expand All @@ -236,9 +226,7 @@ contract Billboard is IBillboard {
// if next auction is not ended,
// push new bid to next auction
else {
if (registry.getBid(tokenId_, _nextAuctionId, msg.sender).placedAt != 0) {
revert BidAlreadyPlaced();
}
require(registry.getBid(tokenId_, _nextAuctionId, msg.sender).placedAt == 0, "Bid already placed");

uint256 _tax = calculateTax(amount_);
registry.newBid(tokenId_, _nextAuctionId, msg.sender, amount_, _tax);
Expand All @@ -261,17 +249,13 @@ contract Billboard is IBillboard {
function _lockBidPriceAndTax(uint256 amount_) private {
// transfer bid price and tax to the registry
(bool _success, ) = address(registry).call{value: amount_}("");
if (!_success) {
revert TransferFailed();
}
require(_success, "Transfer failed");

// refund if overpaid
uint256 _overpaid = msg.value - amount_;
if (_overpaid > 0) {
(bool _refundSuccess, ) = msg.sender.call{value: _overpaid}("");
if (!_refundSuccess) {
revert TransferFailed();
}
require(_refundSuccess, "Transfer failed");
}
}

Expand Down Expand Up @@ -338,7 +322,7 @@ contract Billboard is IBillboard {

uint256 amount = _taxAccumulated - _taxWithdrawn;

if (amount <= 0) revert WithdrawFailed("zero amount");
require(amount > 0, "Zero amount");

// transfer tax to the owner
registry.transferAmount(msg.sender, amount);
Expand All @@ -354,18 +338,18 @@ contract Billboard is IBillboard {
function withdrawBid(uint256 tokenId_, uint256 auctionId_) external {
// revert if auction is still running
IBillboardRegistry.Auction memory _auction = registry.getAuction(tokenId_, auctionId_);
if (block.timestamp < _auction.endAt) revert AuctionNotEnded();
require(block.timestamp >= _auction.endAt, "Auction not ended");

// revert if auction is not cleared
if (_auction.leaseEndAt == 0) revert WithdrawFailed("auction not cleared");
require(_auction.leaseEndAt != 0, "Auction not cleared");

IBillboardRegistry.Bid memory _bid = registry.getBid(tokenId_, auctionId_, msg.sender);
uint256 amount = _bid.price + _bid.tax;

if (_bid.placedAt == 0) revert BidNotFound();
if (_bid.isWithdrawn) revert WithdrawFailed("withdrawn");
if (_bid.isWon) revert WithdrawFailed("won");
if (amount <= 0) revert WithdrawFailed("zero amount");
require(_bid.placedAt != 0, "Bid not found");
require(!_bid.isWithdrawn, "Bid already withdrawn");
require(!_bid.isWon, "Bid already won");
require(amount > 0, "Zero amount");

// transfer bid price and tax back to the bidder
registry.transferAmount(msg.sender, amount);
Expand Down
8 changes: 2 additions & 6 deletions src/Billboard/BillboardRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ contract BillboardRegistry is IBillboardRegistry, ERC721 {
//////////////////////////////

modifier isFromOperator() {
if (msg.sender != operator) {
revert Unauthorized("operator");
}
require(msg.sender == operator, "Operator");
_;
}

Expand Down Expand Up @@ -224,9 +222,7 @@ contract BillboardRegistry is IBillboardRegistry, ERC721 {
/// @inheritdoc IBillboardRegistry
function transferAmount(address to_, uint256 amount_) external isFromOperator {
(bool _success, ) = to_.call{value: amount_}("");
if (!_success) {
revert TransferFailed();
}
require(_success, "transfer failed");
}

//////////////////////////////
Expand Down
20 changes: 0 additions & 20 deletions src/Billboard/IBillboard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,6 @@ pragma solidity ^0.8.20;
import "./IBillboardRegistry.sol";

interface IBillboard {
//////////////////////////////
/// Error
//////////////////////////////

error Unauthorized(string reason_);

error BoardNotFound();

error AuctionNotFound();

error AuctionNotEnded();

error BidNotFound();

error BidAlreadyPlaced();

error WithdrawFailed(string reason_);

error TransferFailed();

//////////////////////////////
/// Upgradability
//////////////////////////////
Expand Down
8 changes: 0 additions & 8 deletions src/Billboard/IBillboardRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,6 @@ pragma solidity ^0.8.20;
import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

interface IBillboardRegistry is IERC721 {
//////////////////////////////
/// Error
//////////////////////////////

error Unauthorized(string type_);

error TransferFailed();

//////////////////////////////
/// Event
//////////////////////////////
Expand Down
Loading

0 comments on commit ff9409a

Please sign in to comment.