From 4bce1f3a91b735915447d9806bac9d7feb433c0b Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 17:57:03 +0200 Subject: [PATCH 1/9] Use non-initialSupply amount for tests We want to test the transactions with an amount that is different than totalSupply to check if the transfered values are exactly the amount user wan to transfer. --- test/token/ERC20WithPermit.test.js | 78 +++++++++++++++++------------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index cdc725c..c117cc7 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -12,16 +12,18 @@ describe("ERC20WithPermit", () => { const hardhatNetworkId = 31337 const initialSupply = to1e18(100) + const initialHolderBalance = to1e18(80) let owner let initialHolder + let secondHolder let recipient let anotherAccount let token beforeEach(async () => { - ;[owner, initialHolder, recipient, anotherAccount] = + ;[owner, initialHolder, secondHolder, recipient, anotherAccount] = await ethers.getSigners() const ERC20WithPermit = await ethers.getContractFactory("ERC20WithPermit") @@ -29,6 +31,9 @@ describe("ERC20WithPermit", () => { await token.deployed() await token.mint(initialHolder.address, initialSupply) + await token + .connect(initialHolder) + .transfer(secondHolder.address, initialSupply.sub(initialHolderBalance)) }) it("should have a name", async () => { @@ -96,7 +101,7 @@ describe("ERC20WithPermit", () => { context("when the requested account has some tokens", () => { it("should return the total amount of tokens", async () => { expect(await token.balanceOf(initialHolder.address)).to.equal( - initialSupply + initialHolderBalance ) }) }) @@ -105,7 +110,7 @@ describe("ERC20WithPermit", () => { describe("transfer", () => { context("when the recipient is not the zero address", () => { context("when the sender does not have enough balance", () => { - const amount = initialSupply.add(1) + const amount = initialHolderBalance.add(1) it("should revert", async () => { await expect( @@ -115,7 +120,7 @@ describe("ERC20WithPermit", () => { }) context("when the sender transfers all balance", () => { - const amount = initialSupply + const amount = initialHolderBalance it("should transfer the requested amount", async () => { await token.connect(initialHolder).transfer(recipient.address, amount) @@ -143,7 +148,7 @@ describe("ERC20WithPermit", () => { await token.connect(initialHolder).transfer(recipient.address, amount) expect(await token.balanceOf(initialHolder.address)).to.equal( - initialSupply + initialHolderBalance ) expect(await token.balanceOf(recipient.address)).to.equal(0) @@ -164,7 +169,9 @@ describe("ERC20WithPermit", () => { context("when the recipient is the zero address", () => { it("should revert", async () => { await expect( - token.connect(initialHolder).transfer(ZERO_ADDRESS, initialSupply) + token + .connect(initialHolder) + .transfer(ZERO_ADDRESS, initialHolderBalance) ).to.be.revertedWith("Transfer to the zero address") }) }) @@ -174,7 +181,8 @@ describe("ERC20WithPermit", () => { context("when the token owner is not the zero address", () => { context("when the recipient is not the zero address", () => { context("when the spender has enough approved balance", () => { - const allowance = initialSupply + const allowance = to1e18(90) + beforeEach(async function () { await token .connect(initialHolder) @@ -182,7 +190,7 @@ describe("ERC20WithPermit", () => { }) context("when the token owner has enough balance", () => { - const amount = initialSupply + const amount = initialHolderBalance it("should transfer the requested amount", async () => { await token @@ -204,7 +212,7 @@ describe("ERC20WithPermit", () => { initialHolder.address, anotherAccount.address ) - ).to.equal(0) + ).to.equal(allowance.sub(amount)) }) it("should emit a transfer event", async () => { @@ -233,7 +241,7 @@ describe("ERC20WithPermit", () => { }) context("when the token owner does not have enough balance", () => { - const amount = initialSupply + const amount = initialHolderBalance beforeEach(async () => { await token @@ -258,7 +266,7 @@ describe("ERC20WithPermit", () => { context( "when the spender does not have enough approved balance", () => { - const allowance = initialSupply.sub(1) + const allowance = initialHolderBalance.sub(1) beforeEach(async () => { await token @@ -267,7 +275,7 @@ describe("ERC20WithPermit", () => { }) context("when the token owner has enough balance", () => { - const amount = initialSupply + const amount = initialHolderBalance it("should revert", async () => { await expect( @@ -283,7 +291,7 @@ describe("ERC20WithPermit", () => { }) context("when the token owner does not have enough balance", () => { - const amount = initialSupply + const amount = initialHolderBalance beforeEach(async () => { await token @@ -305,13 +313,13 @@ describe("ERC20WithPermit", () => { }) context("when the token owner is the zero address", () => { - const allowance = initialSupply + const amount = initialHolderBalance it("should revert", async () => { await expect( token .connect(anotherAccount) - .transferFrom(ZERO_ADDRESS, recipient.address, allowance) + .transferFrom(ZERO_ADDRESS, recipient.address, amount) ).to.be.revertedWith("Transfer amount exceeds allowance") }) }) @@ -320,7 +328,7 @@ describe("ERC20WithPermit", () => { }) context("when the recipient is the zero address", () => { - const allowance = initialSupply + const allowance = initialHolderBalance beforeEach(async () => { await token @@ -339,6 +347,7 @@ describe("ERC20WithPermit", () => { context("when given the maximum allowance", () => { const allowance = MAX_UINT256 + const amount = to1e18(40) beforeEach(async () => { await token @@ -353,7 +362,7 @@ describe("ERC20WithPermit", () => { await token .connect(anotherAccount) - .transferFrom(initialHolder.address, recipient.address, to1e18(100)) + .transferFrom(initialHolder.address, recipient.address, amount) expect( await token.allowance(initialHolder.address, anotherAccount.address) @@ -366,7 +375,7 @@ describe("ERC20WithPermit", () => { describe("approve", () => { context("when the spender is not the zero address", () => { context("when the sender has enough balance", () => { - const allowance = initialSupply + const allowance = initialHolderBalance it("should emit an approval event", async () => { const tx = await token @@ -417,7 +426,7 @@ describe("ERC20WithPermit", () => { }) context("when the sender does not have enough balance", () => { - const allowance = initialSupply.add(1) + const allowance = initialHolderBalance.add(1) it("should emit an approval event", async () => { const tx = await token @@ -467,10 +476,10 @@ describe("ERC20WithPermit", () => { }) context("when the spender is the zero address", () => { - const allowance = initialSupply + const amount = initialHolderBalance it("should revert", async () => { await expect( - token.connect(initialHolder).approve(ZERO_ADDRESS, allowance) + token.connect(initialHolder).approve(ZERO_ADDRESS, amount) ).to.be.revertedWith("Approve to the zero address") }) }) @@ -518,7 +527,7 @@ describe("ERC20WithPermit", () => { describe("burn", () => { it("should reject burning more than balance", async () => { await expect( - token.connect(initialHolder).burn(initialSupply.add(1)) + token.connect(initialHolder).burn(initialHolderBalance.add(1)) ).to.be.revertedWith("Burn amount exceeds balance") }) @@ -535,7 +544,7 @@ describe("ERC20WithPermit", () => { }) it("should decrement owner's balance", async () => { - const expectedBalance = initialSupply.sub(amount) + const expectedBalance = initialHolderBalance.sub(amount) expect(await token.balanceOf(initialHolder.address)).to.equal( expectedBalance ) @@ -549,30 +558,30 @@ describe("ERC20WithPermit", () => { }) } - describeBurn("for entire balance", initialSupply) - describeBurn("for less amount than balance", initialSupply.sub(1)) + describeBurn("for entire balance", initialHolderBalance) + describeBurn("for less amount than balance", initialHolderBalance.sub(1)) }) describe("burnFrom", () => { it("should reject burning more than balance", async () => { await token .connect(initialHolder) - .approve(anotherAccount.address, initialSupply.add(1)) + .approve(anotherAccount.address, initialHolderBalance.add(1)) await expect( token .connect(anotherAccount) - .burnFrom(initialHolder.address, initialSupply.add(1)) + .burnFrom(initialHolder.address, initialHolderBalance.add(1)) ).to.be.revertedWith("Burn amount exceeds balance") }) it("should reject burning more than the allowance", async () => { await token .connect(initialHolder) - .approve(anotherAccount.address, initialSupply.sub(1)) + .approve(anotherAccount.address, initialHolderBalance.sub(1)) await expect( token .connect(anotherAccount) - .burnFrom(initialHolder.address, initialSupply) + .burnFrom(initialHolder.address, initialHolderBalance) ).to.be.revertedWith("Burn amount exceeds allowance") }) @@ -594,7 +603,7 @@ describe("ERC20WithPermit", () => { }) it("should decrement owner's balance", async () => { - const expectedBalance = initialSupply.sub(amount) + const expectedBalance = initialHolderBalance.sub(amount) expect(await token.balanceOf(initialHolder.address)).to.equal( expectedBalance ) @@ -617,8 +626,11 @@ describe("ERC20WithPermit", () => { }) } - describeBurnFrom("for entire balance", initialSupply) - describeBurnFrom("for less amount than balance", initialSupply.sub(1)) + describeBurnFrom("for entire balance", initialHolderBalance) + describeBurnFrom( + "for less amount than balance", + initialHolderBalance.sub(1) + ) context("when given the maximum allowance", () => { const allowance = MAX_UINT256 @@ -636,7 +648,7 @@ describe("ERC20WithPermit", () => { await token .connect(anotherAccount) - .burnFrom(initialHolder.address, initialSupply) + .burnFrom(initialHolder.address, initialHolderBalance) expect( await token.allowance(initialHolder.address, anotherAccount.address) From f635f52d0aa1e0081102c9e7f2fa8388a86b8bb5 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 17:59:04 +0200 Subject: [PATCH 2/9] Test different values of transfered amounts Additionally to testing the whole balance being transfered we want to test: - partial balance transfer to check that part of tokens remain on sender account - partial balance transfer in multiple transaction to check that balances are incremented/decremented --- test/token/ERC20WithPermit.test.js | 247 ++++++++++++++++++++++++----- 1 file changed, 210 insertions(+), 37 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index c117cc7..1514cb5 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -119,6 +119,55 @@ describe("ERC20WithPermit", () => { }) }) + context("when the sender transfers part of their tokens", () => { + const amount = to1e18(60) + + it("should transfer the requested amount", async () => { + await token.connect(initialHolder).transfer(recipient.address, amount) + + expect(await token.balanceOf(initialHolder.address)).to.equal( + initialHolderBalance.sub(amount) + ) + + expect(await token.balanceOf(recipient.address)).to.equal(amount) + }) + + it("should emit a transfer event", async () => { + const tx = await token + .connect(initialHolder) + .transfer(recipient.address, amount) + + await expect(tx) + .to.emit(token, "Transfer") + .withArgs(initialHolder.address, recipient.address, amount) + }) + }) + + context( + "when the sender transfers part of their tokens in two transactions", + () => { + const amount1 = to1e18(15) + const amount2 = to1e18(23) + + it("should transfer the requested amount", async () => { + await token + .connect(initialHolder) + .transfer(recipient.address, amount1) + await token + .connect(initialHolder) + .transfer(recipient.address, amount2) + + expect(await token.balanceOf(initialHolder.address)).to.equal( + initialHolderBalance.sub(amount1).sub(amount2) + ) + + expect(await token.balanceOf(recipient.address)).to.equal( + amount1.add(amount2) + ) + }) + } + ) + context("when the sender transfers all balance", () => { const amount = initialHolderBalance @@ -190,53 +239,177 @@ describe("ERC20WithPermit", () => { }) context("when the token owner has enough balance", () => { - const amount = initialHolderBalance + context("when the sender transfers part of their tokens", () => { + const amount = to1e18(60) - it("should transfer the requested amount", async () => { - await token - .connect(anotherAccount) - .transferFrom(initialHolder.address, recipient.address, amount) + it("should transfer the requested amount", async () => { + await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) - expect(await token.balanceOf(initialHolder.address)).to.equal(0) + expect(await token.balanceOf(initialHolder.address)).to.equal( + initialHolderBalance.sub(amount) + ) - expect(await token.balanceOf(recipient.address)).to.equal(amount) - }) + expect(await token.balanceOf(recipient.address)).to.equal( + amount + ) + }) - it("should decrease the spender allowance", async () => { - await token - .connect(anotherAccount) - .transferFrom(initialHolder.address, recipient.address, amount) + it("should decrease the spender allowance", async () => { + await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) - expect( - await token.allowance( - initialHolder.address, - anotherAccount.address - ) - ).to.equal(allowance.sub(amount)) - }) + expect( + await token.allowance( + initialHolder.address, + anotherAccount.address + ) + ).to.equal(allowance.sub(amount)) + }) - it("should emit a transfer event", async () => { - const tx = await token - .connect(anotherAccount) - .transferFrom(initialHolder.address, recipient.address, amount) + it("should emit a transfer event", async () => { + const tx = await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) + + await expect(tx) + .to.emit(token, "Transfer") + .withArgs(initialHolder.address, recipient.address, amount) + }) + + it("should emit an approval event", async () => { + const tx = await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) - await expect(tx) - .to.emit(token, "Transfer") - .withArgs(initialHolder.address, recipient.address, amount) + await expect(tx) + .to.emit(token, "Approval") + .withArgs( + initialHolder.address, + anotherAccount.address, + allowance.sub(amount) + ) + }) }) - it("should emit an approval event", async () => { - const tx = await token - .connect(anotherAccount) - .transferFrom(initialHolder.address, recipient.address, amount) - - await expect(tx) - .to.emit(token, "Approval") - .withArgs( - initialHolder.address, - anotherAccount.address, - allowance.sub(amount) + context( + "when the sender transfers part of their tokens in two transactions", + () => { + const amount1 = to1e18(15) + const amount2 = to1e18(23) + + it("should transfer the requested amount", async () => { + await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount1 + ) + await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount2 + ) + + expect(await token.balanceOf(initialHolder.address)).to.equal( + initialHolderBalance.sub(amount1).sub(amount2) + ) + + expect(await token.balanceOf(recipient.address)).to.equal( + amount1.add(amount2) + ) + }) + } + ) + + context("when the sender transfers all balance", () => { + const amount = initialHolderBalance + + it("should transfer the requested amount", async () => { + await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) + + expect(await token.balanceOf(initialHolder.address)).to.equal(0) + + expect(await token.balanceOf(recipient.address)).to.equal( + amount ) + }) + + it("should decrease the spender allowance", async () => { + await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) + + expect( + await token.allowance( + initialHolder.address, + anotherAccount.address + ) + ).to.equal(allowance.sub(amount)) + }) + + it("should emit a transfer event", async () => { + const tx = await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) + + await expect(tx) + .to.emit(token, "Transfer") + .withArgs(initialHolder.address, recipient.address, amount) + }) + + it("should emit an approval event", async () => { + const tx = await token + .connect(anotherAccount) + .transferFrom( + initialHolder.address, + recipient.address, + amount + ) + + await expect(tx) + .to.emit(token, "Approval") + .withArgs( + initialHolder.address, + anotherAccount.address, + allowance.sub(amount) + ) + }) }) }) From 94f5676b1efa11b44961eb68263b24bd68395d01 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 18:00:12 +0200 Subject: [PATCH 3/9] Check that totalSupply remains unchanged after transfers --- test/token/ERC20WithPermit.test.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index 1514cb5..7e42e6d 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -108,6 +108,10 @@ describe("ERC20WithPermit", () => { }) describe("transfer", () => { + afterEach("total supply remains unchanged", async () => { + expect(await token.totalSupply()).to.equal(initialSupply) + }) + context("when the recipient is not the zero address", () => { context("when the sender does not have enough balance", () => { const amount = initialHolderBalance.add(1) @@ -227,6 +231,10 @@ describe("ERC20WithPermit", () => { }) describe("transferFrom", () => { + afterEach("total supply remains unchanged", async () => { + expect(await token.totalSupply()).to.equal(initialSupply) + }) + context("when the token owner is not the zero address", () => { context("when the recipient is not the zero address", () => { context("when the spender has enough approved balance", () => { From 3166241b472cba9d1b461f45b13183a1fb65cd1a Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 18:00:39 +0200 Subject: [PATCH 4/9] Minor fixes to test names descriptions --- test/token/ERC20WithPermit.test.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index 7e42e6d..7318a34 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -668,10 +668,13 @@ describe("ERC20WithPermit", () => { describe("mint", () => { const amount = to1e18(50) - it("should reject a zero account", async () => { - await expect( - token.connect(owner).mint(ZERO_ADDRESS, amount) - ).to.be.revertedWith("Mint to the zero address") + + context("for a zero account", () => { + it("should reject a zero account", async () => { + await expect( + token.connect(owner).mint(ZERO_ADDRESS, amount) + ).to.be.revertedWith("Mint to the zero address") + }) }) context("when called not by the owner", () => { @@ -688,7 +691,7 @@ describe("ERC20WithPermit", () => { mintTx = await token.connect(owner).mint(anotherAccount.address, amount) }) - it("should incement totalSupply", async () => { + it("should increment totalSupply", async () => { const expectedSupply = initialSupply.add(amount) expect(await token.totalSupply()).to.equal(expectedSupply) }) From c854266624cc3c545127632e6748b163931d6faf Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 18:02:06 +0200 Subject: [PATCH 5/9] Use different account for permit submitter and spender Anyone can submit a permit, so we want to change the accounts to test it. --- test/token/ERC20WithPermit.test.js | 77 +++++++++++------------------- 1 file changed, 27 insertions(+), 50 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index 7318a34..d13e227 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -913,7 +913,7 @@ describe("ERC20WithPermit", () => { const deadline = yesterday const signature = await getApproval( permittingHolderBalance, - anotherAccount.address, + recipient.address, deadline ) @@ -922,7 +922,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, permittingHolderBalance, deadline, signature.v, @@ -963,7 +963,7 @@ describe("ERC20WithPermit", () => { const deadline = tomorrow const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -971,7 +971,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -981,11 +981,7 @@ describe("ERC20WithPermit", () => { await expect(tx) .to.emit(token, "Approval") - .withArgs( - permittingHolder.address, - anotherAccount.address, - allowance - ) + .withArgs(permittingHolder.address, recipient.address, allowance) }) context("when there was no approved amount before", () => { @@ -993,7 +989,7 @@ describe("ERC20WithPermit", () => { const deadline = tomorrow const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1001,7 +997,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -1010,10 +1006,7 @@ describe("ERC20WithPermit", () => { ) expect( - await token.allowance( - permittingHolder.address, - anotherAccount.address - ) + await token.allowance(permittingHolder.address, recipient.address) ).to.equal(allowance) }) }) @@ -1024,7 +1017,7 @@ describe("ERC20WithPermit", () => { const initialAllowance = allowance.sub(10) const signature = await getApproval( initialAllowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1032,7 +1025,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, initialAllowance, deadline, signature.v, @@ -1045,7 +1038,7 @@ describe("ERC20WithPermit", () => { const deadline = tomorrow const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1053,7 +1046,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -1062,10 +1055,7 @@ describe("ERC20WithPermit", () => { ) expect( - await token.allowance( - permittingHolder.address, - anotherAccount.address - ) + await token.allowance(permittingHolder.address, recipient.address) ).to.equal(allowance) }) }) @@ -1077,7 +1067,7 @@ describe("ERC20WithPermit", () => { const deadline = tomorrow const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1085,7 +1075,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -1095,11 +1085,7 @@ describe("ERC20WithPermit", () => { await expect(tx) .to.emit(token, "Approval") - .withArgs( - permittingHolder.address, - anotherAccount.address, - allowance - ) + .withArgs(permittingHolder.address, recipient.address, allowance) }) context("when there was no approved amount before", () => { @@ -1107,7 +1093,7 @@ describe("ERC20WithPermit", () => { const deadline = tomorrow const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1115,7 +1101,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -1124,10 +1110,7 @@ describe("ERC20WithPermit", () => { ) expect( - await token.allowance( - permittingHolder.address, - anotherAccount.address - ) + await token.allowance(permittingHolder.address, recipient.address) ).to.equal(allowance) }) }) @@ -1138,7 +1121,7 @@ describe("ERC20WithPermit", () => { const initialAllowance = allowance.sub(10) const signature = await getApproval( initialAllowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1146,7 +1129,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, initialAllowance, deadline, signature.v, @@ -1159,7 +1142,7 @@ describe("ERC20WithPermit", () => { const deadline = tomorrow const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1167,7 +1150,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -1176,10 +1159,7 @@ describe("ERC20WithPermit", () => { ) expect( - await token.allowance( - permittingHolder.address, - anotherAccount.address - ) + await token.allowance(permittingHolder.address, recipient.address) ).to.equal(allowance) }) }) @@ -1215,7 +1195,7 @@ describe("ERC20WithPermit", () => { it("should be accepted at any moment", async () => { const signature = await getApproval( allowance, - anotherAccount.address, + recipient.address, deadline ) @@ -1225,7 +1205,7 @@ describe("ERC20WithPermit", () => { .connect(anotherAccount) .permit( permittingHolder.address, - anotherAccount.address, + recipient.address, allowance, deadline, signature.v, @@ -1234,10 +1214,7 @@ describe("ERC20WithPermit", () => { ) expect( - await token.allowance( - permittingHolder.address, - anotherAccount.address - ) + await token.allowance(permittingHolder.address, recipient.address) ).to.equal(allowance) }) }) From 1a34a4086a31d1e14f39e6bc9034abb7d98f7a0b Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 18:03:05 +0200 Subject: [PATCH 6/9] Add more tests for invalid permit signatures --- test/token/ERC20WithPermit.test.js | 107 ++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 16 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index d13e227..1a20e9b 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -934,25 +934,100 @@ describe("ERC20WithPermit", () => { }) context("when permission has an invalid signature", () => { - it("should revert", async () => { - const deadline = tomorrow - const signature = await getApproval( - permittingHolderBalance, - anotherAccount.address, - deadline - ) + context("when owner doesn't match the permitting holder", () => { + it("should revert", async () => { + const deadline = tomorrow + const signature = await getApproval( + permittingHolderBalance, + recipient.address, + deadline + ) - await expect( - token.connect(anotherAccount).permit( - anotherAccount.address, // does not match the signature - anotherAccount.address, + await expect( + token.connect(anotherAccount).permit( + anotherAccount.address, // does not match the signature + recipient.address, + permittingHolderBalance, + deadline, + signature.v, + signature.r, + signature.s + ) + ).to.be.revertedWith("Invalid signature") + }) + }) + + context("when spender doesn't match the signature", () => { + it("should revert", async () => { + const deadline = tomorrow + const signature = await getApproval( permittingHolderBalance, - deadline, - signature.v, - signature.r, - signature.s + recipient.address, + deadline ) - ).to.be.revertedWith("Invalid signature") + + await expect( + token.connect(anotherAccount).permit( + permittingHolder.address, + anotherAccount.address, // does not match the signature + permittingHolderBalance, + deadline, + signature.v, + signature.r, + signature.s + ) + ).to.be.revertedWith("Invalid signature") + }) + }) + + context("when 's' value is malleable", () => { + it("should revert", async () => { + const deadline = tomorrow + const signature = await getApproval( + permittingHolderBalance, + recipient.address, + deadline + ) + + const malleableS = ethers.BigNumber.from( + "0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0" + ).add(1) + + await expect( + token.connect(anotherAccount).permit( + permittingHolder.address, + recipient.address, + permittingHolderBalance, + deadline, + signature.v, + signature.r, + malleableS // invalid 's' value + ) + ).to.be.revertedWith("Invalid signature 's' value") + }) + }) + + context("when 'v' value is invalid", () => { + it("should revert", async () => { + const deadline = tomorrow + const signature = await getApproval( + permittingHolderBalance, + recipient.address, + deadline + ) + + await expect( + token.connect(anotherAccount).permit( + permittingHolder.address, + recipient.address, + permittingHolderBalance, + deadline, + signature.v - 27, // invalid 'v' value + signature.r, + signature.s + ) + ).to.be.revertedWith("Invalid signature 'v' value") + }) }) }) From b3902153d3de8298d30e2b7dec06c3fa3b1c55ba Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 18:03:29 +0200 Subject: [PATCH 7/9] Test if permit increases the nonce --- test/token/ERC20WithPermit.test.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index 1a20e9b..6ab8e10 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -1059,6 +1059,35 @@ describe("ERC20WithPermit", () => { .withArgs(permittingHolder.address, recipient.address, allowance) }) + it("should increment the nonce for permitting holder", async () => { + const deadline = tomorrow + const signature = await getApproval( + allowance, + recipient.address, + deadline + ) + + const initialNonce = await token.nonces(permittingHolder.address) + + await token + .connect(anotherAccount) + .permit( + permittingHolder.address, + recipient.address, + allowance, + deadline, + signature.v, + signature.r, + signature.s + ) + + expect(await token.nonces(permittingHolder.address)).to.equal( + initialNonce.add(1) + ) + expect(await token.nonces(anotherAccount.address)).to.equal(0) + expect(await token.nonces(recipient.address)).to.equal(0) + }) + context("when there was no approved amount before", () => { it("should approve the requested amount", async () => { const deadline = tomorrow From 431fb388b9aee13869ebf69d6967d1e89b64cd19 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Jul 2021 18:07:11 +0200 Subject: [PATCH 8/9] Use test fixture to initialize tests Using the test fixture let us decrease test execution time from 14 seconds to 6 seconds. --- test/token/ERC20WithPermit.test.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index 6ab8e10..c13765b 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -7,6 +7,9 @@ const { ZERO_ADDRESS, } = require("../helpers/contract-test-helpers") +const { waffle } = require("hardhat") +const { loadFixture } = waffle + describe("ERC20WithPermit", () => { // default Hardhat's networks blockchain, see https://hardhat.org/config/ const hardhatNetworkId = 31337 @@ -22,18 +25,26 @@ describe("ERC20WithPermit", () => { let token - beforeEach(async () => { + before("load accounts", async () => { ;[owner, initialHolder, secondHolder, recipient, anotherAccount] = await ethers.getSigners() + }) + async function fixture() { const ERC20WithPermit = await ethers.getContractFactory("ERC20WithPermit") - token = await ERC20WithPermit.deploy("My Token", "MT") + const token = await ERC20WithPermit.deploy("My Token", "MT") await token.deployed() await token.mint(initialHolder.address, initialSupply) await token .connect(initialHolder) .transfer(secondHolder.address, initialSupply.sub(initialHolderBalance)) + + return token + } + + beforeEach(async () => { + token = await loadFixture(fixture) }) it("should have a name", async () => { From 0ef7ee3cc8758b9613e107625494da8b4d4ce51f Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Thu, 23 Sep 2021 17:01:18 +0200 Subject: [PATCH 9/9] Extract repeated tx code to beforeEach blocks We can define the transactions execution in beforeEach blocks, as we do in other tests. --- test/token/ERC20WithPermit.test.js | 44 ++++++++---------------------- 1 file changed, 12 insertions(+), 32 deletions(-) diff --git a/test/token/ERC20WithPermit.test.js b/test/token/ERC20WithPermit.test.js index 9add320..3ad890c 100644 --- a/test/token/ERC20WithPermit.test.js +++ b/test/token/ERC20WithPermit.test.js @@ -137,9 +137,15 @@ describe("ERC20WithPermit", () => { context("when the sender transfers part of their tokens", () => { const amount = to1e18(60) - it("should transfer the requested amount", async () => { - await token.connect(initialHolder).transfer(recipient.address, amount) + let tx + beforeEach(async () => { + tx = await token + .connect(initialHolder) + .transfer(recipient.address, amount) + }) + + it("should transfer the requested amount", async () => { expect(await token.balanceOf(initialHolder.address)).to.equal( initialHolderBalance.sub(amount) ) @@ -148,10 +154,6 @@ describe("ERC20WithPermit", () => { }) it("should emit a transfer event", async () => { - const tx = await token - .connect(initialHolder) - .transfer(recipient.address, amount) - await expect(tx) .to.emit(token, "Transfer") .withArgs(initialHolder.address, recipient.address, amount) @@ -365,15 +367,17 @@ describe("ERC20WithPermit", () => { context("when the sender transfers all balance", () => { const amount = initialHolderBalance - it("should transfer the requested amount", async () => { - await token + beforeEach(async () => { + tx = await token .connect(anotherAccount) .transferFrom( initialHolder.address, recipient.address, amount ) + }) + it("should transfer the requested amount", async () => { expect(await token.balanceOf(initialHolder.address)).to.equal(0) expect(await token.balanceOf(recipient.address)).to.equal( @@ -382,14 +386,6 @@ describe("ERC20WithPermit", () => { }) it("should decrease the spender allowance", async () => { - await token - .connect(anotherAccount) - .transferFrom( - initialHolder.address, - recipient.address, - amount - ) - expect( await token.allowance( initialHolder.address, @@ -399,28 +395,12 @@ describe("ERC20WithPermit", () => { }) it("should emit a transfer event", async () => { - const tx = await token - .connect(anotherAccount) - .transferFrom( - initialHolder.address, - recipient.address, - amount - ) - await expect(tx) .to.emit(token, "Transfer") .withArgs(initialHolder.address, recipient.address, amount) }) it("should emit an approval event", async () => { - const tx = await token - .connect(anotherAccount) - .transferFrom( - initialHolder.address, - recipient.address, - amount - ) - await expect(tx) .to.emit(token, "Approval") .withArgs(