Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #233: Add setup event #279

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Closes #233: Add setup event #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions benchmark/GnosisSafe.Creation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import "@nomiclabs/hardhat-ethers";
import { setupBenchmarkContracts } from "./utils/setup"

const contractSetup = setupBenchmarkContracts(undefined, true)
describe("GnosisSafe", async () => {
it("creation", async () => {
await contractSetup()
})
})
12 changes: 6 additions & 6 deletions benchmark/utils/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ export interface Contracts {
additions: any | undefined
}

const generateTarget = async (owners: Wallet[], threshold: number, guardAddress: string) => {
const generateTarget = async (owners: Wallet[], threshold: number, guardAddress: string, logGasUsage?: boolean) => {
const fallbackHandler = await getDefaultCallbackHandler()
const safe = await getSafeWithOwners(owners.map((owner) => owner.address), threshold, fallbackHandler.address)
const safe = await getSafeWithOwners(owners.map((owner) => owner.address), threshold, fallbackHandler.address, logGasUsage)
await executeContractCallWithSigners(safe, safe, "setGuard", [guardAddress], owners)
return safe
}
Expand All @@ -28,14 +28,14 @@ export const configs = [
{ name: "3 out of 5", signers: [user1, user2, user3, user4, user5], threshold: 3 },
]

const setupBenchmarkContracts = async (benchmarkFixture?: () => Promise<any>) => {
return await deployments.createFixture(async ({ deployments }) => {
export const setupBenchmarkContracts = (benchmarkFixture?: () => Promise<any>, logGasUsage?: boolean) => {
return deployments.createFixture(async ({ deployments }) => {
await deployments.fixture();
const guardFactory = await hre.ethers.getContractFactory("DelegateCallTransactionGuard");
const guard = await guardFactory.deploy(AddressZero)
const targets = []
for (const config of configs) {
targets.push(await generateTarget(config.signers, config.threshold, config.useGuard ? guard.address : AddressZero))
targets.push(await generateTarget(config.signers, config.threshold, config.useGuard ? guard.address : AddressZero, logGasUsage))
}
return {
targets,
Expand All @@ -54,7 +54,7 @@ export interface Benchmark {
export const benchmark = async (topic: string, benchmarks: Benchmark[]) => {
for (const benchmark of benchmarks) {
const { name, prepare, after, fixture } = benchmark
const contractSetup = await setupBenchmarkContracts(fixture)
const contractSetup = setupBenchmarkContracts(fixture)
describe(`${topic} - ${name}`, async () => {
it("with an EOA", async () => {
const contracts = await contractSetup()
Expand Down
8 changes: 8 additions & 0 deletions contracts/GnosisSafe.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ contract GnosisSafe
//);
bytes32 private constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;

event SafeSetup(
Uxio0 marked this conversation as resolved.
Show resolved Hide resolved
address indexed initiator,
address[] owners,
uint256 threshold,
address initializer,
address fallbackHandler
Copy link
Member

Choose a reason for hiding this comment

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

address paymentToken, uint256 payment, address payable paymentReceiver should be there until we remove them

Copy link
Member Author

Choose a reason for hiding this comment

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

I left them out as they do not have an impact on the internal state of the safe. And as they cost gas I would only include critical information.

);
event ApproveHash(
bytes32 indexed approvedHash,
address indexed owner
Expand Down Expand Up @@ -99,6 +106,7 @@ contract GnosisSafe
// baseGas = 0, gasPrice = 1 and gas = payment => amount = (payment + 0) * 1 = payment
handlePayment(payment, 0, 1, paymentToken, paymentReceiver);
}
emit SafeSetup(msg.sender, _owners, _threshold, to, fallbackHandler);
}

/// @dev Allows to execute a Safe transaction confirmed by required number of owners and then pays the account that submitted the transaction.
Expand Down
1 change: 0 additions & 1 deletion test/core/GnosisSafe.Execution.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ describe("GnosisSafe", async () => {
executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]).then((tx) => { executedTx = tx; return tx })
).to.emit(safe, "ExecutionSuccess")
const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx!!.hash)
console.log(receipt.logs)
const successEvent = safe.interface.decodeEventLog("ExecutionSuccess", receipt.logs[0].data, receipt.logs[0].topics)
expect(successEvent.txHash).to.be.eq(calculateSafeTransactionHash(safe, tx, await chainId()))
// Gas costs are around 3000, so even if we specified a safeTxGas from 100000 we should not use more
Expand Down
10 changes: 7 additions & 3 deletions test/core/GnosisSafe.Setup.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe("GnosisSafe", async () => {
}
})

describe("Setup", async () => {
describe("setup", async () => {
it('should not allow to call setup on singleton', async () => {
await deployments.fixture();
const singleton = await getSafeSingleton()
Expand All @@ -45,7 +45,9 @@ describe("GnosisSafe", async () => {

it('should set domain hash', async () => {
const { template } = await setupTests()
await template.setup([user1.address, user2.address, user3.address], 2, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero)
await expect(
template.setup([user1.address, user2.address, user3.address], 2, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero)
).to.emit(template, "SafeSetup").withArgs(user1.address, [user1.address, user2.address, user3.address], 2, AddressZero, AddressZero)
await expect(await template.domainSeparator()).to.be.eq(calculateSafeDomainSeparator(template, await chainId()))
await expect(await template.getOwners()).to.be.deep.eq([user1.address, user2.address, user3.address])
await expect(await template.getThreshold()).to.be.deep.eq(BigNumber.from(2))
Expand Down Expand Up @@ -129,7 +131,9 @@ describe("GnosisSafe", async () => {
}`
const testIntializer = await deployContract(user1, source);
const initData = testIntializer.interface.encodeFunctionData("init", ["0x42baddad"])
await template.setup([user1.address, user2.address, user3.address], 2, testIntializer.address, initData, AddressOne, AddressZero, 0, AddressZero)
await expect(
template.setup([user1.address, user2.address, user3.address], 2, testIntializer.address, initData, AddressOne, AddressZero, 0, AddressZero)
).to.emit(template, "SafeSetup").withArgs(user1.address, [user1.address, user2.address, user3.address], 2, testIntializer.address, AddressOne)
await expect(await template.domainSeparator()).to.be.eq(calculateSafeDomainSeparator(template, await chainId()))
await expect(await template.getOwners()).to.be.deep.eq([user1.address, user2.address, user3.address])
await expect(await template.getThreshold()).to.be.deep.eq(BigNumber.from(2))
Expand Down
4 changes: 2 additions & 2 deletions test/utils/execution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ export const buildSignatureBytes = (signatures: SafeSignature[]): string => {
return signatureBytes
}

export const logGas = async (message: string, tx: Promise<any>): Promise<any> => {
export const logGas = async (message: string, tx: Promise<any>, skip?: boolean): Promise<any> => {
return tx.then(async (result) => {
const receipt = await result.wait()
console.log(" Used", receipt.gasUsed.toNumber(), `gas for >${message}<`)
if (!skip) console.log(" Used", receipt.gasUsed.toNumber(), `gas for >${message}<`)
return result
})
}
Expand Down
9 changes: 7 additions & 2 deletions test/utils/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import hre, { deployments } from "hardhat"
import { Wallet, Contract } from "ethers"
import { AddressZero } from "@ethersproject/constants";
import solc from "solc"
import { logGas } from "./execution";

export const defaultCallbackHandlerDeployment = async () => {
return await deployments.get("DefaultCallbackHandler");
Expand Down Expand Up @@ -68,9 +69,13 @@ export const getSafeTemplate = async () => {
return Safe.attach(template);
}

export const getSafeWithOwners = async (owners: string[], threshold?: number, fallbackHandler?: string) => {
export const getSafeWithOwners = async (owners: string[], threshold?: number, fallbackHandler?: string, logGasUsage?: boolean) => {
const template = await getSafeTemplate()
await template.setup(owners, threshold || owners.length, AddressZero, "0x", fallbackHandler || AddressZero, AddressZero, 0, AddressZero)
await logGas(
`Setup Safe with ${owners.length} owner(s)${fallbackHandler && fallbackHandler !== AddressZero ? " and fallback handler" : ""}`,
template.setup(owners, threshold || owners.length, AddressZero, "0x", fallbackHandler || AddressZero, AddressZero, 0, AddressZero),
!logGasUsage
)
return template
}

Expand Down