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.

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

Set safeTxGas=0 when safeVersion>=1.3.0 #121

Merged
merged 6 commits into from
Dec 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion packages/safe-core-sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
"@types/chai-as-promised": "^7.1.4",
"@types/mocha": "^9.0.0",
"@types/node": "^16.11.9",
"@types/semver": "^7.3.9",
"@types/yargs": "^16.0.1",
"@typescript-eslint/eslint-plugin": "^5.4.0",
"@typescript-eslint/parser": "^5.4.0",
Expand Down Expand Up @@ -92,6 +93,7 @@
"dependencies": {
"@gnosis.pm/safe-core-sdk-types": "^0.1.1",
"@gnosis.pm/safe-deployments": "^1.4.0",
"ethereumjs-util": "^7.1.3"
"ethereumjs-util": "^7.1.3",
"semver": "^7.3.5"
}
}
9 changes: 9 additions & 0 deletions packages/safe-core-sdk/src/Safe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@ class Safe {
return this.#contractManager.safeContract.getAddress()
}

/**
* Returns the ContractManager
*
* @returns The current ContractManager
* */
getContractManager(): ContractManager {
return this.#contractManager
}

/**
* Returns the current EthAdapter.
*
Expand Down
6 changes: 5 additions & 1 deletion packages/safe-core-sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { SafeVersion } from './contracts/config'
import EthAdapter, { EthAdapterTransaction } from './ethereumLibs/EthAdapter'
import EthersAdapter, { EthersAdapterConfig } from './ethereumLibs/EthersAdapter'
import Web3Adapter, { Web3AdapterConfig } from './ethereumLibs/Web3Adapter'
import ContractManager from './managers/contractManager'
import Safe, {
AddOwnerTxParams,
ConnectSafeConfig,
Expand All @@ -21,9 +22,11 @@ import {
TransactionOptions,
TransactionResult
} from './utils/transactions/types'
import { standardizeSafeTransactionData } from './utils/transactions/utils'

export default Safe
export {
ContractManager,
SafeVersion,
SafeFactory,
SafeFactoryConfig,
Expand All @@ -44,5 +47,6 @@ export {
AddOwnerTxParams,
RemoveOwnerTxParams,
SwapOwnerTxParams,
EthSignSignature
EthSignSignature,
standardizeSafeTransactionData
}
25 changes: 25 additions & 0 deletions packages/safe-core-sdk/src/utils/safeVersions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import semverSatisfies from 'semver/functions/satisfies'

export enum FEATURES {
SAFE_TX_GAS_OPTIONAL
}

const FEATURES_BY_VERSION: Record<string, string> = {
[FEATURES.SAFE_TX_GAS_OPTIONAL]: '>=1.3.0'
}

const isEnabledByVersion = (feature: FEATURES, version: string): boolean => {
if (!(feature in FEATURES_BY_VERSION)) {
return true
}
return semverSatisfies(version, FEATURES_BY_VERSION[feature])
}

export const enabledFeatures = (version: string): FEATURES[] => {
const features = Object.values(FEATURES) as FEATURES[]
return features.filter((feature) => isEnabledByVersion(feature, version))
}

export const hasFeature = (name: FEATURES, version: string): boolean => {
return enabledFeatures(version).includes(name)
}
27 changes: 17 additions & 10 deletions packages/safe-core-sdk/src/utils/transactions/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
import GnosisSafeContract from '../../contracts/GnosisSafe/GnosisSafeContract'
import EthAdapter from '../../ethereumLibs/EthAdapter'
import { ZERO_ADDRESS } from '../constants'
import { FEATURES, hasFeature } from '../safeVersions'
import { estimateTxGas } from './gas'

export function standardizeMetaTransactionData(
Expand Down Expand Up @@ -37,16 +38,22 @@ export async function standardizeSafeTransactionData(
refundReceiver: tx.refundReceiver || ZERO_ADDRESS,
nonce: tx.nonce ?? (await safeContract.getNonce())
}
const safeTxGas =
tx.safeTxGas ??
(await estimateTxGas(
safeContract,
ethAdapter,
standardizedTxs.to,
standardizedTxs.value,
standardizedTxs.data,
standardizedTxs.operation
))
let safeTxGas: number
const safeVersion = await safeContract.getVersion()
if (hasFeature(FEATURES.SAFE_TX_GAS_OPTIONAL, safeVersion)) {
safeTxGas = 0
} else {
safeTxGas =
tx.safeTxGas ??
(await estimateTxGas(
safeContract,
ethAdapter,
standardizedTxs.to,
standardizedTxs.value,
standardizedTxs.data,
standardizedTxs.operation
))
}
return {
...standardizedTxs,
safeTxGas
Expand Down
96 changes: 92 additions & 4 deletions packages/safe-core-sdk/tests/createTransaction.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import { MetaTransactionData, SafeTransactionDataPartial } from '@gnosis.pm/safe
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import { deployments, waffle } from 'hardhat'
import Safe, { ContractNetworksConfig, SafeTransactionOptionalProps } from '../src'
import { safeVersionDeployed } from '../hardhat/deploy/deploy-contracts'
import Safe, {
ContractNetworksConfig,
SafeTransactionOptionalProps,
standardizeSafeTransactionData
} from '../src'
import { itif } from './utils/helpers'
import {
getERC20Mintable,
getFactory,
Expand Down Expand Up @@ -36,6 +42,88 @@ describe('Transactions creation', () => {
}
})

describe('standardizeSafeTransactionData', async () => {
itif(safeVersionDeployed >= '1.3.0')(
'should return a transaction with safeTxGas=0 if safeVersion>=1.3.0',
async () => {
const { accounts, contractNetworks } = await setupTests()
const [account1, account2] = accounts
const safe = await getSafeWithOwners([account1.address])
const ethAdapter = await getEthAdapter(account1.signer)
const safeSdk = await Safe.create({
ethAdapter,
safeAddress: safe.address,
contractNetworks
})
const txDataPartial: SafeTransactionDataPartial = {
to: account2.address,
value: '0',
data: '0x'
}
const safeTxData = await standardizeSafeTransactionData(
safeSdk.getContractManager().safeContract,
ethAdapter,
txDataPartial
)
chai.expect(safeTxData.safeTxGas).to.be.eq(0)
}
)

itif(safeVersionDeployed < '1.3.0')(
'should return a transaction with estimated safeTxGas if safeVersion<1.3.0',
async () => {
const { accounts, contractNetworks } = await setupTests()
const [account1, account2] = accounts
const safe = await getSafeWithOwners([account1.address])
const ethAdapter = await getEthAdapter(account1.signer)
const safeSdk = await Safe.create({
ethAdapter,
safeAddress: safe.address,
contractNetworks
})
const txDataPartial: SafeTransactionDataPartial = {
to: account2.address,
value: '0',
data: '0x'
}
const safeTxData = await standardizeSafeTransactionData(
safeSdk.getContractManager().safeContract,
ethAdapter,
txDataPartial
)
chai.expect(safeTxData.safeTxGas).to.be.gt(0)
}
)

itif(safeVersionDeployed < '1.3.0')(
'should return a transaction with defined safeTxGas if safeVersion<1.3.0',
async () => {
const { accounts, contractNetworks } = await setupTests()
const [account1, account2] = accounts
const safe = await getSafeWithOwners([account1.address])
const ethAdapter = await getEthAdapter(account1.signer)
const safeSdk = await Safe.create({
ethAdapter,
safeAddress: safe.address,
contractNetworks
})
const safeTxGas = 111
const txDataPartial: SafeTransactionDataPartial = {
to: account2.address,
value: '0',
data: '0x',
safeTxGas
}
const safeTxData = await standardizeSafeTransactionData(
safeSdk.getContractManager().safeContract,
ethAdapter,
txDataPartial
)
chai.expect(safeTxData.safeTxGas).to.be.eq(safeTxGas)
}
)
})

describe('createTransaction', async () => {
it('should create a single transaction', async () => {
const { accounts, contractNetworks } = await setupTests()
Expand Down Expand Up @@ -67,7 +155,7 @@ describe('Transactions creation', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should create a single transaction when passing a transaction array with length=1', async () => {
Expand Down Expand Up @@ -127,7 +215,7 @@ describe('Transactions creation', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should fail when creating a MultiSend transaction passing a transaction array with length=0', async () => {
Expand Down Expand Up @@ -221,7 +309,7 @@ describe('Transactions creation', () => {
chai.expect(multiSendTx.data.gasToken).to.be.eq('0x333')
chai.expect(multiSendTx.data.refundReceiver).to.be.eq('0x444')
chai.expect(multiSendTx.data.nonce).to.be.eq(555)
chai.expect(multiSendTx.data.safeTxGas).to.be.eq(666)
chai.expect(multiSendTx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})
})
})
5 changes: 3 additions & 2 deletions packages/safe-core-sdk/tests/moduleManager.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import { deployments, waffle } from 'hardhat'
import { safeVersionDeployed } from '../hardhat/deploy/deploy-contracts'
import Safe, { ContractNetworksConfig, SafeTransactionOptionalProps } from '../src'
import { SENTINEL_ADDRESS, ZERO_ADDRESS } from '../src/utils/constants'
import {
Expand Down Expand Up @@ -153,7 +154,7 @@ describe('Safe modules manager', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should enable a Safe module', async () => {
Expand Down Expand Up @@ -259,7 +260,7 @@ describe('Safe modules manager', () => {
chai.expect(tx2.data.gasToken).to.be.eq('0x333')
chai.expect(tx2.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx2.data.nonce).to.be.eq(555)
chai.expect(tx2.data.safeTxGas).to.be.eq(666)
chai.expect(tx2.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should disable Safe modules', async () => {
Expand Down
7 changes: 4 additions & 3 deletions packages/safe-core-sdk/tests/ownerManager.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import { deployments, waffle } from 'hardhat'
import { safeVersionDeployed } from '../hardhat/deploy/deploy-contracts'
import Safe, { ContractNetworksConfig, SafeTransactionOptionalProps } from '../src'
import { SENTINEL_ADDRESS, ZERO_ADDRESS } from '../src/utils/constants'
import {
Expand Down Expand Up @@ -198,7 +199,7 @@ describe('Safe owners manager', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should add an owner and keep the same threshold', async () => {
Expand Down Expand Up @@ -362,7 +363,7 @@ describe('Safe owners manager', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should remove the first owner of a Safe and decrease the threshold', async () => {
Expand Down Expand Up @@ -631,7 +632,7 @@ describe('Safe owners manager', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should replace the first owner of a Safe', async () => {
Expand Down
3 changes: 2 additions & 1 deletion packages/safe-core-sdk/tests/threshold.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import { deployments, waffle } from 'hardhat'
import { safeVersionDeployed } from '../hardhat/deploy/deploy-contracts'
import Safe, { ContractNetworksConfig, SafeTransactionOptionalProps } from '../src'
import {
getFactory,
Expand Down Expand Up @@ -106,7 +107,7 @@ describe('Safe Threshold', () => {
chai.expect(tx.data.gasToken).to.be.eq('0x333')
chai.expect(tx.data.refundReceiver).to.be.eq('0x444')
chai.expect(tx.data.nonce).to.be.eq(555)
chai.expect(tx.data.safeTxGas).to.be.eq(666)
chai.expect(tx.data.safeTxGas).to.be.eq(safeVersionDeployed >= '1.3.0' ? 0 : 666)
})

it('should change the threshold', async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/safe-service-client/tests/endpoint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ describe('Endpoint tests', () => {
data: '0x',
value: '123456789',
operation: 1,
safeTxGas: 123,
safeTxGas: 0,
baseGas: 0,
gasPrice: 0,
gasToken: '0x0000000000000000000000000000000000000000',
Copy link
Member

Choose a reason for hiding this comment

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

Do you use the a ZERO_ADDRESS often? Might be worthwhile creating a constant for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is only used twice in this test file

Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3816,6 +3816,11 @@
dependencies:
"@types/node" "*"

"@types/semver@^7.3.9":
version "7.3.9"
resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.3.9.tgz#152c6c20a7688c30b967ec1841d31ace569863fc"
integrity sha512-L/TMpyURfBkf+o/526Zb6kd/tchUP3iBDEPjqjb+U2MAJhVRxxrmr2fwpe08E7QsV7YLcpq0tUaQ9O9x97ZIxQ==

"@types/serve-static@*":
version "1.13.10"
resolved "https://registry.yarnpkg.com/@types/serve-static/-/serve-static-1.13.10.tgz#f5e0ce8797d2d7cc5ebeda48a52c96c4fa47a8d9"
Expand Down