Skip to content

Commit

Permalink
Remove unnecessary awaits from sendTransactions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mmv08
mmv08 committed Aug 26, 2024
1 parent 35a4ef8 commit da63ade
Show file tree
Hide file tree
Showing 19 changed files with 139 additions and 116 deletions.
2 changes: 1 addition & 1 deletion test/accessors/SimulateTxAccessor.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe("SimulateTxAccessor", () => {
const [user1, user2] = signers;
const accessorAddress = await accessor.getAddress();
const safeAddress = await safe.getAddress();
await (await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") })).wait();
await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") });
const userBalance = await hre.ethers.provider.getBalance(user2.address);
const tx = await buildContractCall(interactor, "sendAndReturnBalance", [user2.address, ethers.parseEther("1")], 0, true);
const simulationData = accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.data, tx.operation]);
Expand Down
45 changes: 41 additions & 4 deletions test/core/Safe.Execution.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe("Safe", () => {
const [user1] = signers;
const safeAddress = await safe.getAddress();
// Fund refund
await (await user1.sendTransaction({ to: safeAddress, value: 10000000 })).wait();
await user1.sendTransaction({ to: safeAddress, value: 10000000 });
await expect(executeContractCallWithSigners(safe, reverter, "revert", [], [user1], false, { gasPrice: 1 })).to.emit(
safe,
"ExecutionFailure",
Expand Down Expand Up @@ -173,7 +173,7 @@ describe("Safe", () => {
const { safe, reverter, signers } = await setupTests();
const [user1] = signers;
const safeAddress = await safe.getAddress();
await (await user1.sendTransaction({ to: safeAddress, value: 10000000 })).wait();
await user1.sendTransaction({ to: safeAddress, value: 10000000 });
await expect(executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true, { gasPrice: 1 })).to.emit(
safe,
"ExecutionFailure",
Expand Down Expand Up @@ -207,7 +207,7 @@ describe("Safe", () => {
refundReceiver: user2.address,
});

await (await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") })).wait();
await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") });
const userBalance = await hre.ethers.provider.getBalance(user2.address);
expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.eq(ethers.parseEther("1"));

Expand All @@ -229,6 +229,43 @@ describe("Safe", () => {
expect(await hre.ethers.provider.getBalance(user2.address)).to.eq(userBalance + successEvent.payment);
});

it("should emit payment in failure event", async () => {
const { safe, storageSetter, signers } = await setupTests();
const [user1, user2] = signers;
const safeAddress = await safe.getAddress();
const storageSetterAddress = await storageSetter.getAddress();
const data = storageSetter.interface.encodeFunctionData("setStorage", ["0xbaddad"]);
const tx = buildSafeTransaction({
to: storageSetterAddress,
data,
nonce: await safe.nonce(),
operation: 0,
gasPrice: 1,
safeTxGas: 3000,
refundReceiver: user2.address,
});

await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") });
const userBalance = await hre.ethers.provider.getBalance(user2.address);
await expect(await hre.ethers.provider.getBalance(safeAddress)).to.eq(ethers.parseEther("1"));

const executedTx = await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]);
await expect(executedTx).to.emit(safe, "ExecutionFailure");
const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx!.hash);
const receiptLogs = receipt?.logs ?? [];
// There are additional ETH transfer events on zkSync related to transaction fees
const logIndex = receiptLogs.length - (hre.network.zksync ? 2 : 1);
const successEvent = safe.interface.decodeEventLog(
"ExecutionFailure",
receiptLogs[logIndex].data,
receiptLogs[logIndex].topics,
);
expect(successEvent.txHash).to.be.eq(calculateSafeTransactionHash(safeAddress, tx, await chainId()));
// FIXME: When running out of gas the gas used is slightly higher than the safeTxGas and the user has to overpay
expect(successEvent.payment).to.be.lte(10000n);
await expect(await hre.ethers.provider.getBalance(user2.address)).to.eq(userBalance + successEvent.payment);
});

it("should be possible to manually increase gas", async () => {
if (hre.network.zksync) {
// This test fails in zksync because of (allegedly) enormous gas cost differences
Expand Down Expand Up @@ -312,7 +349,7 @@ describe("Safe", () => {
refundReceiver: nativeTokenReceiverAddress,
});

await (await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") })).wait();
await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") });
await expect(await hre.ethers.provider.getBalance(safeAddress)).to.eq(ethers.parseEther("1"));

// await expect(await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 5500000 })).to.emit(
Expand Down
18 changes: 6 additions & 12 deletions test/core/Safe.FallbackManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@ describe("FallbackManager", () => {
).to.be.eq("0x" + "".padStart(64, "0"));

// Setup Safe
await (
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", handler.address, AddressZero, 0, AddressZero)
).wait();
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", handler.address, AddressZero, 0, AddressZero);

// Check fallback handler
await expect(
Expand All @@ -61,7 +59,7 @@ describe("FallbackManager", () => {
const [user1, user2] = signers;

// Setup Safe
await (await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero)).wait();
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero);

// Check fallback handler
await expect(
Expand Down Expand Up @@ -90,7 +88,7 @@ describe("FallbackManager", () => {
const [user1, user2] = signers;

// Setup Safe
await (await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero)).wait();
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero);

// Check event
await expect(executeContractCallWithSigners(safe, safe, "setFallbackHandler", [handler.address], [user1]))
Expand Down Expand Up @@ -125,9 +123,7 @@ describe("FallbackManager", () => {
const mirrorAddress = await mirror.getAddress();
const [user1, user2] = signers;
// Setup Safe
await (
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", mirrorAddress, AddressZero, 0, AddressZero)
).wait();
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", mirrorAddress, AddressZero, 0, AddressZero);

const tx = {
to: await safe.getAddress(),
Expand All @@ -150,9 +146,7 @@ describe("FallbackManager", () => {
const mirrorAddress = await mirror.getAddress();
const [user1, user2] = signers;
// Setup Safe
await (
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", mirrorAddress, AddressZero, 0, AddressZero)
).wait();
await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", mirrorAddress, AddressZero, 0, AddressZero);

const tx = {
to: await safe.getAddress(),
Expand Down Expand Up @@ -180,7 +174,7 @@ describe("FallbackManager", () => {
const { safe, signers } = await setupWithTemplate();
const [user1] = signers;
// Setup Safe
await (await safe.setup([user1.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero)).wait();
await safe.setup([user1.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero);

// The transaction execution function doesn't bubble up revert messages so we check for a generic transaction fail code GS013
await expect(
Expand Down
10 changes: 5 additions & 5 deletions test/core/Safe.GuardManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe("GuardManager", () => {
const validGuardMockAddress = await validGuardMock.getAddress();
const safe = await getSafe({ owners: [user1.address] });

await (await executeContractCallWithSigners(safe, safe, "setGuard", [validGuardMockAddress], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "setGuard", [validGuardMockAddress], [user1]);

// Check guard
await expect(await hre.ethers.provider.getStorage(await safe.getAddress(), GUARD_STORAGE_SLOT)).to.be.eq(
Expand Down Expand Up @@ -167,15 +167,15 @@ describe("GuardManager", () => {
signatureBytes,
safeMsgSender,
]);
await (await validGuardMock.givenCalldataRevertWithMessage(checkTxData, "Computer says Nah")).wait();
await validGuardMock.givenCalldataRevertWithMessage(checkTxData, "Computer says Nah");
const checkExecData = guardInterface.encodeFunctionData("checkAfterExecution", [
calculateSafeTransactionHash(safeAddress, safeTx, await chainId()),
true,
]);

await expect(executeTx(safe, safeTx, [signature])).to.be.revertedWith("Computer says Nah");

await (await validGuardMock.reset()).wait();
await validGuardMock.reset();

await expect(executeTx(safe, safeTx, [signature])).to.emit(safe, "ExecutionSuccess");

Expand Down Expand Up @@ -216,11 +216,11 @@ describe("GuardManager", () => {
calculateSafeTransactionHash(safeAddress, safeTx, await chainId()),
true,
]);
await (await validGuardMock.givenCalldataRevertWithMessage(checkExecData, "Computer says Nah")).wait();
await validGuardMock.givenCalldataRevertWithMessage(checkExecData, "Computer says Nah");

await expect(executeTx(safe, safeTx, [signature])).to.be.revertedWith("Computer says Nah");

await (await validGuardMock.reset()).wait();
await validGuardMock.reset();

await expect(executeTx(safe, safeTx, [signature])).to.emit(safe, "ExecutionSuccess");

Expand Down
28 changes: 14 additions & 14 deletions test/core/Safe.ModuleManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe("ModuleManager", () => {
signers: [user1, user2],
} = await setupTests();
// Use module for execution to see error
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await expect(executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).to.revertedWith("GS013");
});
Expand Down Expand Up @@ -158,8 +158,8 @@ describe("ModuleManager", () => {
safe,
signers: [user1, user2],
} = await setupTests();
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])).wait();
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1]);
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "disableModule", [user1.address, user2.address], [user1]),
).to.revertedWith("GS013");
Expand All @@ -170,9 +170,9 @@ describe("ModuleManager", () => {
safe,
signers: [user1, user2],
} = await setupTests();
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1]);
await expect(await safe.isModuleEnabled(user1.address)).to.be.true;
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);
await expect(await safe.isModuleEnabled(user2.address)).to.be.true;
await expect(await safe.getModulesPaginated(AddressOne, 10)).to.be.deep.equal([[user2.address, user1.address], AddressOne]);

Expand Down Expand Up @@ -219,7 +219,7 @@ describe("ModuleManager", () => {
} = await setupTests();
const mockAddress = await mock.getAddress();
const user2Safe = safe.connect(user2);
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await expect(user2Safe.execTransactionFromModule(mockAddress, 0, "0xbaddad", 0))
.to.emit(safe, "ExecutionFromModuleSuccess")
Expand All @@ -235,7 +235,7 @@ describe("ModuleManager", () => {
} = await setupTests();
const mockAddress = await mock.getAddress();
const user2Safe = safe.connect(user2);
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await mock.givenAnyRevert();
await expect(user2Safe.execTransactionFromModule(mockAddress, 0, "0xbaddad", 0))
Expand Down Expand Up @@ -347,7 +347,7 @@ describe("ModuleManager", () => {
} = await setupTests();
const mockAddress = await mock.getAddress();
const user2Safe = safe.connect(user2);
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await mock.givenAnyRevert();
await expect(user2Safe.execTransactionFromModuleReturnData(mockAddress, 0, "0xbaddad", 0))
Expand All @@ -363,7 +363,7 @@ describe("ModuleManager", () => {
} = await setupTests();
const mockAddress = await mock.getAddress();
const user2Safe = safe.connect(user2);
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await expect(user2Safe.execTransactionFromModuleReturnData(mockAddress, 0, "0xbaddad", 0))
.to.emit(safe, "ExecutionFromModuleSuccess")
Expand All @@ -379,9 +379,9 @@ describe("ModuleManager", () => {
} = await setupTests();
const mockAddress = await mock.getAddress();
const user2Safe = safe.connect(user2);
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await (await mock.givenCalldataReturn("0xbaddad", "0xdeaddeed")).wait();
await mock.givenCalldataReturn("0xbaddad", "0xdeaddeed");
await expect(await user2Safe.execTransactionFromModuleReturnData.staticCall(mockAddress, 0, "0xbaddad", 0)).to.be.deep.eq([
true,
"0xdeaddeed",
Expand All @@ -396,9 +396,9 @@ describe("ModuleManager", () => {
} = await setupTests();
const mockAddress = await mock.getAddress();
const user2Safe = safe.connect(user2);
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await (await mock.givenCalldataRevertWithMessage("0xbaddad", "Some random message")).wait();
await mock.givenCalldataRevertWithMessage("0xbaddad", "Some random message");
await expect(await user2Safe.execTransactionFromModuleReturnData.staticCall(mockAddress, 0, "0xbaddad", 0)).to.be.deep.eq([
false,
"0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000013536f6d652072616e646f6d206d65737361676500000000000000000000000000",
Expand Down Expand Up @@ -532,7 +532,7 @@ describe("ModuleManager", () => {
} = await setupTests();

await expect(safe.getModulesPaginated(AddressZero, 1)).to.be.reverted;
await (await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1]);
expect(await safe.getModulesPaginated(user1.address, 1)).to.be.deep.equal([[], AddressOne]);
await expect(safe.getModulesPaginated(user2.address, 1)).to.be.revertedWith("GS105");
});
Expand Down
6 changes: 3 additions & 3 deletions test/core/Safe.OwnerManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe("OwnerManager", () => {
safe,
signers: [user1, user2],
} = await setupTests();
await (await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);

await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])).to.revertedWith(
"GS013",
Expand Down Expand Up @@ -225,8 +225,8 @@ describe("OwnerManager", () => {
safe,
signers: [user1, user2, user3],
} = await setupTests();
await (await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])).wait();
await (await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user3.address, 2], [user1])).wait();
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user3.address, 2], [user1]);
await expect(await safe.getOwners()).to.be.deep.equal([user3.address, user2.address, user1.address]);
await expect(await safe.getThreshold()).to.be.deep.eq(2n);
await expect(await safe.isOwner(user1.address)).to.be.true;
Expand Down
Loading

0 comments on commit da63ade

Please sign in to comment.