Skip to content

Commit

Permalink
test: use custom errors, fix the tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@adjisb
adjisb committed Apr 26, 2024
1 parent 372e4a0 commit a52e7b5
Show file tree
Hide file tree
Showing 12 changed files with 426 additions and 252 deletions.
18 changes: 9 additions & 9 deletions packages/land/test/common/Config.behavior.ts
View file
Expand Up @@ -7,9 +7,9 @@ export function landConfig(setupLand, Contract: string) {
describe('roles', function () {
it('Only admin can set landMinter', async function () {
const {LandContract, deployer} = await loadFixture(setupLand);
await expect(LandContract.setMinter(deployer, true)).to.be.revertedWith(
'only admin allowed',
);
await expect(
LandContract.setMinter(deployer, true),
).to.be.revertedWithCustomError(LandContract, 'OnlyAdmin');
});

it('should enable a landMinter', async function () {
Expand All @@ -27,9 +27,9 @@ export function landConfig(setupLand, Contract: string) {

it('should not set royaltyManager if caller is not admin', async function () {
const {LandAsOther, other} = await loadFixture(setupLand);
await expect(LandAsOther.setRoyaltyManager(other)).to.be.revertedWith(
'only admin allowed',
);
await expect(
LandAsOther.setRoyaltyManager(other),
).to.be.revertedWithCustomError(LandAsOther, 'OnlyAdmin');
});

it('should set royaltyManager', async function () {
Expand All @@ -44,9 +44,9 @@ export function landConfig(setupLand, Contract: string) {

it('should not set owner if caller is not admin', async function () {
const {LandAsOther, other} = await loadFixture(setupLand);
await expect(LandAsOther.transferOwnership(other)).to.be.revertedWith(
'only admin allowed',
);
await expect(
LandAsOther.transferOwnership(other),
).to.be.revertedWithCustomError(LandAsOther, 'OnlyAdmin');
});

it('should set owner', async function () {
Expand Down
126 changes: 87 additions & 39 deletions packages/land/test/common/ERC721.behavior.ts
View file
Expand Up @@ -153,13 +153,16 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
});

it('should revert burning tokens by unauthorized operator', async function () {
const {LandAsMinter, other, LandAsOther1} =
const {LandAsMinter, other, other1, LandAsOther1} =
await loadFixture(setupLand);

await LandAsMinter.mintQuad(other, 1, 0, 0, '0x');
await expect(LandAsOther1.burnFrom(other, 0)).to.be.revertedWith(
'UNAUTHORIZED_BURN',
);
await expect(LandAsOther1.burnFrom(other, 0))
.to.be.revertedWithCustomError(
LandAsOther1,
'ERC721InsufficientApproval',
)
.withArgs(other1, 0);
});
});

Expand All @@ -168,22 +171,27 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
const {LandAsOwner, other, tokenIds} = await loadFixture(setupLand);
await expect(
LandAsOwner.batchTransferFrom(ZeroAddress, other, tokenIds, '0x'),
).to.be.revertedWith('NOT_FROM_ZEROADDRESS');
).to.be.revertedWithCustomError(LandAsOwner, 'InvalidAddress');
});

it('should revert batchTransfer to zero address', async function () {
const {LandAsOwner, landOwner, tokenIds} = await loadFixture(setupLand);
await expect(
LandAsOwner.batchTransferFrom(landOwner, ZeroAddress, tokenIds, '0x'),
).to.be.revertedWith('NOT_TO_ZEROADDRESS');
).to.be.revertedWithCustomError(LandAsOwner, 'InvalidAddress');
});

it('should revert batchTransfer from unauthorized sender', async function () {
const {LandAsOther, landOwner, other, tokenIds} =
await loadFixture(setupLand);
await expect(
LandAsOther.batchTransferFrom(landOwner, other, tokenIds, '0x'),
).to.be.revertedWith('NOT_AUTHORIZED');
)
.to.be.revertedWithCustomError(
LandAsOther,
'ERC721InsufficientApproval',
)
.withArgs(other, tokenIds[0]);
});

it('should batch transfer tokens from authorized sender', async function () {
Expand Down Expand Up @@ -241,7 +249,9 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
[tokenIds[1], tokenIds[1], tokenIds[0]],
'0x',
),
).to.be.revertedWith('BATCHTRANSFERFROM_NOT_OWNER');
)
.to.be.revertedWithCustomError(LandAsOwner, 'ERC721InvalidOwner')
.withArgs(landOwner);
});

it('batch transfer works', async function () {
Expand Down Expand Up @@ -279,7 +289,10 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
[tokenIds[0]],
'0x',
),
).to.be.revertedWith('Batch Receive not allowed');
).to.be.revertedWithCustomError(
TestERC721TokenReceiver,
'BatchReceiveNotAllowed',
);
});

it('batch transferring to a contract that do not accept erc721 token should fail', async function () {
Expand All @@ -293,7 +306,10 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
[tokenIds[0]],
'0x',
),
).to.be.revertedWith('Batch Receive not allowed');
).to.be.revertedWithCustomError(
TestERC721TokenReceiver,
'BatchReceiveNotAllowed',
);
});

it('batch transferring to a contract that do not return the correct onERC721Received bytes should fail', async function () {
Expand All @@ -307,7 +323,12 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
[tokenIds[0]],
'0x',
),
).to.be.revertedWith('ERC721_BATCH_RECEIVED_REJECTED');
)
.to.be.revertedWithCustomError(
LandAsOwner,
'ERC721InvalidBatchReceiver',
)
.withArgs(TestERC721TokenReceiver);
});

it('batch transferring to a contract that do not implemented mandatory receiver should not fail', async function () {
Expand Down Expand Up @@ -359,7 +380,10 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
TestERC721TokenReceiver,
tokenIds[0],
),
).to.be.revertedWith('Receive not allowed');
).to.be.revertedWithCustomError(
TestERC721TokenReceiver,
'ReceiveNotAllowed',
);
});

it('transferring to a contract that do not return the correct onERC721Received bytes should fail', async function () {
Expand All @@ -372,7 +396,9 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
TestERC721TokenReceiver,
tokenIds[0],
),
).to.be.revertedWith('ERC721_TRANSFER_REJECTED');
)
.to.be.revertedWithCustomError(LandAsOwner, 'ERC721InvalidReceiver')
.withArgs(TestERC721TokenReceiver);
});

it('transferring to a contract that do not implemented mandatory receiver should not fail', async function () {
Expand Down Expand Up @@ -410,7 +436,9 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
[tokenIds[0], tokenIds[1], tokenIds[0]],
'0x',
),
).to.be.revertedWith('BATCHTRANSFERFROM_NOT_OWNER');
)
.to.be.revertedWithCustomError(LandAsOwner, 'ERC721InvalidOwner')
.withArgs(landOwner);
});

it('safe batch transfer works', async function () {
Expand Down Expand Up @@ -491,16 +519,19 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
it('transferring from without approval should fails', async function () {
const {LandAsOther, landOwner, other, tokenIds} =
await loadFixture(setupLand);
await expect(
LandAsOther.transferFrom(landOwner, other, tokenIds[0]),
).to.be.revertedWith('UNAUTHORIZED_TRANSFER');
await expect(LandAsOther.transferFrom(landOwner, other, tokenIds[0]))
.to.be.revertedWithCustomError(
LandAsOther,
'ERC721InsufficientApproval',
)
.withArgs(other, tokenIds[0]);
});

it('transferring to zero address should fails', async function () {
const {LandAsOwner, landOwner, tokenIds} = await loadFixture(setupLand);
await expect(
LandAsOwner.transferFrom(landOwner, ZeroAddress, tokenIds[0]),
).to.be.revertedWith('NOT_TO_ZEROADDRESS');
).to.be.revertedWithCustomError(LandAsOwner, 'InvalidAddress');
});

it('transferring to a contract that accepts erc721 token should not fail', async function () {
Expand Down Expand Up @@ -563,12 +594,12 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
});

it('should revert approveFor from unauthorized sender', async function () {
const {LandContract, LandAsMinter, other1, other} =
const {LandContract, LandAsMinter, other1, other, deployer} =
await loadFixture(setupLand);
await LandAsMinter.mintQuad(other, 1, 0, 0, '0x');
await expect(
LandContract.approveFor(other, other1, 0),
).to.be.revertedWith('UNAUTHORIZED_APPROVAL');
await expect(LandContract.approveFor(other, other1, 0))
.to.be.revertedWithCustomError(LandContract, 'ERC721InvalidApprover')
.withArgs(deployer);
});

it('should approveFor when sender is superOperator', async function () {
Expand Down Expand Up @@ -612,10 +643,11 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
});

it('should revert setApprovalForAllFor from unauthorized sender', async function () {
const {LandAsOther, other1, deployer} = await loadFixture(setupLand);
await expect(
LandAsOther.setApprovalForAllFor(deployer, other1, true),
).to.be.revertedWith('UNAUTHORIZED_APPROVE_FOR_ALL');
const {LandAsOther, other, other1, deployer} =
await loadFixture(setupLand);
await expect(LandAsOther.setApprovalForAllFor(deployer, other1, true))
.to.be.revertedWithCustomError(LandAsOther, 'ERC721InvalidApprover')
.withArgs(other);
});

it('should setApprovalForAllFor from authorized sender', async function () {
Expand All @@ -635,9 +667,9 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
const {LandAsOther1, LandAsAdmin, other, other1} =
await loadFixture(setupLand);
await LandAsAdmin.setSuperOperator(other1, true);
await expect(
LandAsOther1.setApprovalForAllFor(other, other1, true),
).to.be.revertedWith('INVALID_APPROVAL_CHANGE');
await expect(LandAsOther1.setApprovalForAllFor(other, other1, true))
.to.be.revertedWithCustomError(LandAsAdmin, 'ERC721InvalidOperator')
.withArgs(other1);
});
});

Expand Down Expand Up @@ -683,7 +715,7 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
await loadFixture(setupLand);
await expect(
safeTransferFrom(LandAsOwner, landOwner, ZeroAddress, tokenIds[0]),
).to.be.revertedWith('NOT_TO_ZEROADDRESS');
).to.be.revertedWithCustomError(LandAsOwner, 'InvalidAddress');
},
);

Expand All @@ -704,7 +736,12 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
await loadFixture(setupLand);
await expect(
safeTransferFrom(LandAsOther, landOwner, other, tokenIds[0]),
).to.be.revertedWith('UNAUTHORIZED_TRANSFER');
)
.to.be.revertedWithCustomError(
LandAsOther,
'ERC721InsufficientApproval',
)
.withArgs(other, tokenIds[0]);
},
);

Expand All @@ -722,7 +759,10 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
TestERC721TokenReceiver,
tokenIds[0],
),
).to.be.revertedWith('Receive not allowed');
).to.be.revertedWithCustomError(
TestERC721TokenReceiver,
'ReceiveNotAllowed',
);
},
);

Expand All @@ -740,7 +780,9 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
TestERC721TokenReceiver,
tokenIds[0],
),
).to.be.revertedWith('ERC721_TRANSFER_REJECTED');
)
.to.be.revertedWithCustomError(LandAsOwner, 'ERC721InvalidReceiver')
.withArgs(TestERC721TokenReceiver);
},
);

Expand Down Expand Up @@ -889,9 +931,12 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
await loadFixture(setupLand);
await LandAsOwner.approve(other1, tokenIds[0]);
await LandAsOther1.transferFrom(landOwner, other, tokenIds[0]);
await expect(
LandAsOther1.transferFrom(other, landOwner, tokenIds[0]),
).to.be.revertedWith('UNAUTHORIZED_TRANSFER');
await expect(LandAsOther1.transferFrom(other, landOwner, tokenIds[0]))
.to.be.revertedWithCustomError(
LandAsOther1,
'ERC721InsufficientApproval',
)
.withArgs(other1, tokenIds[0]);
});

it('approval by operator works', async function () {
Expand Down Expand Up @@ -964,9 +1009,12 @@ export function shouldCheckForERC721(setupLand, Contract: string) {
await loadFixture(setupLand);
await LandAsOwner.setApprovalForAll(other1, true);
await LandAsOwner.transferFrom(landOwner, other, tokenIds[0]);
await expect(
LandAsOther1.transferFrom(other, other1, tokenIds[0]),
).to.be.revertedWith('UNAUTHORIZED_TRANSFER');
await expect(LandAsOther1.transferFrom(other, other1, tokenIds[0]))
.to.be.revertedWithCustomError(
LandAsOwner,
'ERC721InsufficientApproval',
)
.withArgs(other1, tokenIds[0]);
});

it('approval for all set before will work on a transfered NFT', async function () {
Expand Down
30 changes: 15 additions & 15 deletions packages/land/test/common/LandGetter.behavior.ts
View file
Expand Up @@ -55,33 +55,33 @@ export function shouldCheckLandGetter(setupLand, Contract: string) {
it('should revert when fetching owner of given quad id with wrong size', async function () {
const {LandContract} = await loadFixture(setupLand);
const id = getId(9, 0, 0);
await expect(LandContract.ownerOf(id)).to.be.revertedWith(
'Invalid token id',
);
await expect(LandContract.ownerOf(id))
.to.be.revertedWithCustomError(LandContract, 'ERC721NonexistentToken')
.withArgs(id);
});

it('should revert when fetching owner of given quad id with invalid token', async function () {
const {LandContract} = await loadFixture(setupLand);
const id = getId(3, 2, 2);
await expect(LandContract.ownerOf(id)).to.be.revertedWith(
'Invalid token id',
);
await expect(LandContract.ownerOf(id))
.to.be.revertedWithCustomError(LandContract, 'ERC721NonexistentToken')
.withArgs(id);
});

it('should revert when fetching owner of given quad id with invalid token by(x)', async function () {
const {LandContract} = await loadFixture(setupLand);
const id = getId(3, 2, 0);
await expect(LandContract.ownerOf(id)).to.be.revertedWith(
'Invalid token id',
);
await expect(LandContract.ownerOf(id))
.to.be.revertedWithCustomError(LandContract, 'ERC721NonexistentToken')
.withArgs(id);
});

it('should revert when fetching owner of given quad id with invalid token(y)', async function () {
const {LandAsMinter} = await loadFixture(setupLand);
const id = getId(3, 0, 2);
await expect(LandAsMinter.ownerOf(id)).to.be.revertedWith(
'Invalid token id',
);
await expect(LandAsMinter.ownerOf(id))
.to.be.revertedWithCustomError(LandAsMinter, 'ERC721NonexistentToken')
.withArgs(id);
});

it('should return owner of given quad id', async function () {
Expand Down Expand Up @@ -152,9 +152,9 @@ export function shouldCheckLandGetter(setupLand, Contract: string) {
const {LandContract} = await loadFixture(setupLand);

const tokenId = 2 + 2 * GRID_SIZE;
await expect(LandContract.tokenURI(tokenId)).to.be.revertedWith(
'Id does not exist',
);
await expect(LandContract.tokenURI(tokenId))
.to.be.revertedWithCustomError(LandContract, 'ERC721NonexistentToken')
.withArgs(tokenId);
});
});
}

1 comment on commit a52e7b5

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage for this commit

95.58%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
packages/land/contracts
   Land.sol94.37%92.11%100%94.74%119, 22, 25–26
   LandMetadataRegistry.sol100%100%100%100%
   PolygonLand.sol94.74%92.50%100%95.24%23, 26–27, 97
packages/land/contracts/common
   ERC721BaseToken.sol87.50%93.55%70.59%89.06%101, 108, 167, 167, 167, 35, 351–352, 352, 352–353, 355, 357, 44, 52, 61, 69, 78
   LandBaseToken.sol97.37%95.38%97.06%98.43%214, 214, 214, 214, 214, 474, 589, 591–593
   OperatorFiltererUpgradeable.sol91.11%88.89%100%91.30%21–22, 39–40
   WithAdmin.sol93.10%90%100%93.33%36–37
   WithMetadataRegistry.sol100%100%100%100%
   WithOwner.sol100%100%100%100%
   WithRoyalties.sol100%100%100%100%
   WithSuperOperators.sol100%100%100%100%
packages/land/contracts/interfaces
   IERC721MandatoryTokenReceiver.sol100%100%100%100%
   IErrors.sol100%100%100%100%
   ILandMetadataRegistry.sol100%100%100%100%
   ILandToken.sol100%100%100%100%
   IOperatorFilterRegistry.sol100%100%100%100%
packages/land/contracts/mainnet
   LandBase.sol100%100%100%100%
   LandStorageMixin.sol100%100%100%100%
packages/land/contracts/polygon
   ERC2771Handler.sol57.14%50%66.67%54.55%17–18, 51, 51, 51–52, 54
   PolygonLandBase.sol95%100%95%95%31
   PolygonLandStorageMixin.sol100%100%100%100%
packages/land/contracts/registry
   LandMetadataBase.sol100%100%100%100%

Please sign in to comment.