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

Rename methods and error messages #251

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Rename methods and error messages #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12a50a4
Rename contracts in error messages
germartinez Aug 30, 2022
f18d0b2
Rename methods that return a Safe transaction that modifies the settings
germartinez Aug 30, 2022
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
38 changes: 19 additions & 19 deletions packages/safe-core-sdk/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ const safeSdk2 = await safeSdk.connect({ ethAdapter, safeAddress })

### getAddress

Returns the address of the current Safe Proxy contract.
Returns the address of the current SafeProxy contract.

```js
const address = safeSdk.getAddress()
Expand Down Expand Up @@ -651,12 +651,12 @@ const txHash = await safeSdk.getTransactionHash(safeTransaction)
const owners = await safeSdk.getOwnersWhoApprovedTx(txHash)
```

### getEnableModuleTx
### createEnableModuleTx
Copy link
Member

@iamacook iamacook Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should we format these? i.e.

Suggested change
### createEnableModuleTx
### `createEnableModuleTx`

Copy link
Member Author

@germartinez germartinez Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm personally not a fan of using `` in titles like these from the visuals perspective. I found ENS is doing it this way, but also not sure which one is better. I think I prefer to keep the method names as they are, all together.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's keep them as is.


Returns a Safe transaction ready to be signed that will enable a Safe module.

```js
const safeTransaction = await safeSdk.getEnableModuleTx(moduleAddress)
const safeTransaction = await safeSdk.createEnableModuleTx(moduleAddress)
const txResponse = await safeSdk.executeTransaction(safeTransaction)
await txResponse.transactionResponse?.wait()
```
Expand All @@ -672,15 +672,15 @@ const options: SafeTransactionOptionalProps = {
refundReceiver, // Optional
nonce // Optional
}
const safeTransaction = await safeSdk.getEnableModuleTx(moduleAddress, options)
const safeTransaction = await safeSdk.createEnableModuleTx(moduleAddress, options)
```

### getDisableModuleTx
### createDisableModuleTx

Returns a Safe transaction ready to be signed that will disable a Safe module.

```js
const safeTransaction = await safeSdk.getDisableModuleTx(moduleAddress)
const safeTransaction = await safeSdk.createDisableModuleTx(moduleAddress)
const txResponse = await safeSdk.executeTransaction(safeTransaction)
await txResponse.transactionResponse?.wait()
```
Expand All @@ -689,10 +689,10 @@ This method can optionally receive the `options` parameter:

```js
const options: SafeTransactionOptionalProps = { ... }
const safeTransaction = await safeSdk.getDisableModuleTx(moduleAddress, options)
const safeTransaction = await safeSdk.createDisableModuleTx(moduleAddress, options)
```

### getAddOwnerTx
### createAddOwnerTx

Returns the Safe transaction to add an owner and optionally change the threshold.

Expand All @@ -701,7 +701,7 @@ const params: AddOwnerTxParams = {
ownerAddress,
threshold // Optional. If `threshold` is not provided the current threshold will not change.
}
const safeTransaction = await safeSdk.getAddOwnerTx(params)
const safeTransaction = await safeSdk.createAddOwnerTx(params)
const txResponse = await safeSdk.executeTransaction(safeTransaction)
await txResponse.transactionResponse?.wait()
```
Expand All @@ -710,10 +710,10 @@ This method can optionally receive the `options` parameter:

```js
const options: SafeTransactionOptionalProps = { ... }
const safeTransaction = await safeSdk.getAddOwnerTx(params, options)
const safeTransaction = await safeSdk.createAddOwnerTx(params, options)
```

### getRemoveOwnerTx
### createRemoveOwnerTx

Returns the Safe transaction to remove an owner and optionally change the threshold.

Expand All @@ -722,7 +722,7 @@ const params: RemoveOwnerTxParams = {
ownerAddress,
newThreshold // Optional. If `newThreshold` is not provided, the current threshold will be decreased by one.
}
const safeTransaction = await safeSdk.getRemoveOwnerTx(params)
const safeTransaction = await safeSdk.createRemoveOwnerTx(params)
const txResponse = await safeSdk.executeTransaction(safeTransaction)
await txResponse.transactionResponse?.wait()
```
Expand All @@ -731,10 +731,10 @@ This method can optionally receive the `options` parameter:

```js
const options: SafeTransactionOptionalProps = { ... }
const safeTransaction = await safeSdk.getRemoveOwnerTx(params, options)
const safeTransaction = await safeSdk.createRemoveOwnerTx(params, options)
```

### getSwapOwnerTx
### createSwapOwnerTx

Returns the Safe transaction to replace an owner of the Safe with a new one.

Expand All @@ -743,7 +743,7 @@ const params: SwapOwnerTxParams = {
oldOwnerAddress,
newOwnerAddress
}
const safeTransaction = await safeSdk.getSwapOwnerTx(params)
const safeTransaction = await safeSdk.createSwapOwnerTx(params)
const txResponse = await safeSdk.executeTransaction(safeTransaction)
await txResponse.transactionResponse?.wait()
```
Expand All @@ -752,15 +752,15 @@ This method can optionally receive the `options` parameter:

```js
const options: SafeTransactionOptionalProps = { ... }
const safeTransaction = await safeSdk.getSwapOwnerTx(params, options)
const safeTransaction = await safeSdk.createSwapOwnerTx(params, options)
```

### getChangeThresholdTx
### createChangeThresholdTx

Returns the Safe transaction to change the threshold.

```js
const safeTransaction = await safeSdk.getChangeThresholdTx(newThreshold)
const safeTransaction = await safeSdk.createChangeThresholdTx(newThreshold)
const txResponse = await safeSdk.executeTransaction(safeTransaction)
await txResponse.transactionResponse?.wait()
```
Expand All @@ -769,7 +769,7 @@ This method can optionally receive the `options` parameter:

```js
const options: SafeTransactionOptionalProps = { ... }
const safeTransaction = await safeSdk.getChangeThresholdTx(newThreshold, options)
const safeTransaction = await safeSdk.createChangeThresholdTx(newThreshold, options)
```

### executeTransaction
Expand Down
24 changes: 13 additions & 11 deletions packages/safe-core-sdk/src/Safe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class Safe {
* Creates an instance of the Safe Core SDK.
* @param config - Ethers Safe configuration
* @returns The Safe Core SDK instance
* @throws "Safe Proxy contract is not deployed on the current network"
* @throws "SafeProxy contract is not deployed on the current network"
* @throws "MultiSend contract is not deployed on the current network"
* @throws "MultiSendCallOnly contract is not deployed on the current network"
*/
Expand All @@ -110,8 +110,9 @@ class Safe {
* Initializes the Safe Core SDK instance.
* @param config - Safe configuration
* @throws "Signer must be connected to a provider"
* @throws "Safe Proxy contract is not deployed on the current network"
* @throws "SafeProxy contract is not deployed on the current network"
* @throws "MultiSend contract is not deployed on the current network"
* @throws "MultiSendCallOnly contract is not deployed on the current network"
*/
private async init({
ethAdapter,
Expand All @@ -133,8 +134,9 @@ class Safe {
/**
* Returns a new instance of the Safe Core SDK.
* @param config - Connect Safe configuration
* @throws "Safe Proxy contract is not deployed on the current network"
* @throws "SafeProxy contract is not deployed on the current network"
* @throws "MultiSend contract is not deployed on the current network"
* @throws "MultiSendCallOnly contract is not deployed on the current network"
*/
async connect({
ethAdapter,
Expand All @@ -151,9 +153,9 @@ class Safe {
}

/**
* Returns the address of the current Safe Proxy contract.
* Returns the address of the current SafeProxy contract.
*
* @returns The address of the Safe Proxy contract
* @returns The address of the SafeProxy contract
*/
getAddress(): string {
return this.#contractManager.safeContract.getAddress()
Expand Down Expand Up @@ -482,7 +484,7 @@ class Safe {
* @throws "Invalid module address provided"
* @throws "Module provided is already enabled"
*/
async getEnableModuleTx(
async createEnableModuleTx(
moduleAddress: string,
options?: SafeTransactionOptionalProps
): Promise<SafeTransaction> {
Expand All @@ -505,7 +507,7 @@ class Safe {
* @throws "Invalid module address provided"
* @throws "Module provided is not enabled already"
*/
async getDisableModuleTx(
async createDisableModuleTx(
moduleAddress: string,
options?: SafeTransactionOptionalProps
): Promise<SafeTransaction> {
Expand All @@ -530,7 +532,7 @@ class Safe {
* @throws "Threshold needs to be greater than 0"
* @throws "Threshold cannot exceed owner count"
*/
async getAddOwnerTx(
async createAddOwnerTx(
{ ownerAddress, threshold }: AddOwnerTxParams,
options?: SafeTransactionOptionalProps
): Promise<SafeTransaction> {
Expand All @@ -555,7 +557,7 @@ class Safe {
* @throws "Threshold needs to be greater than 0"
* @throws "Threshold cannot exceed owner count"
*/
async getRemoveOwnerTx(
async createRemoveOwnerTx(
{ ownerAddress, threshold }: RemoveOwnerTxParams,
options?: SafeTransactionOptionalProps
): Promise<SafeTransaction> {
Expand All @@ -580,7 +582,7 @@ class Safe {
* @throws "New address provided is already an owner"
* @throws "Old address provided is not an owner"
*/
async getSwapOwnerTx(
async createSwapOwnerTx(
{ oldOwnerAddress, newOwnerAddress }: SwapOwnerTxParams,
options?: SafeTransactionOptionalProps
): Promise<SafeTransaction> {
Expand All @@ -603,7 +605,7 @@ class Safe {
* @throws "Threshold needs to be greater than 0"
* @throws "Threshold cannot exceed owner count"
*/
async getChangeThresholdTx(
async createChangeThresholdTx(
threshold: number,
options?: SafeTransactionOptionalProps
): Promise<SafeTransaction> {
Expand Down
6 changes: 3 additions & 3 deletions packages/safe-core-sdk/src/contracts/safeDeploymentContracts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export async function getSafeContract({
})
const isContractDeployed = await ethAdapter.isContractDeployed(gnosisSafeContract.getAddress())
if (!isContractDeployed) {
throw new Error('Safe Proxy contract is not deployed on the current network')
throw new Error('SafeProxy contract is not deployed on the current network')
}
return gnosisSafeContract
}
Expand All @@ -108,7 +108,7 @@ export async function getProxyFactoryContract({
safeProxyFactoryContract.getAddress()
)
if (!isContractDeployed) {
throw new Error('Safe Proxy Factory contract is not deployed on the current network')
throw new Error('SafeProxyFactory contract is not deployed on the current network')
}
return safeProxyFactoryContract
}
Expand All @@ -129,7 +129,7 @@ export async function getMultiSendContract({
})
const isContractDeployed = await ethAdapter.isContractDeployed(multiSendContract.getAddress())
if (!isContractDeployed) {
throw new Error('Multi Send contract is not deployed on the current network')
throw new Error('MultiSend contract is not deployed on the current network')
}
return multiSendContract
}
Expand Down
2 changes: 1 addition & 1 deletion packages/safe-core-sdk/src/safeFactory/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class SafeFactory {
})
const isContractDeployed = await this.#ethAdapter.isContractDeployed(safeAddress)
if (!isContractDeployed) {
throw new Error('Safe Proxy contract is not deployed on the current network')
throw new Error('SafeProxy contract is not deployed on the current network')
}
const safe = await Safe.create({
ethAdapter: this.#ethAdapter,
Expand Down
4 changes: 2 additions & 2 deletions packages/safe-core-sdk/src/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ export interface ContractNetworkConfig {
safeMasterCopyAddress: string
/** safeMasterCopyAbi - Abi of the Gnosis Safe Master Copy contract deployed on a specific network */
safeMasterCopyAbi?: AbiItem | AbiItem[]
/** safeProxyFactoryAddress - Address of the Gnosis Safe Proxy Factory contract deployed on a specific network */
/** safeProxyFactoryAddress - Address of the Gnosis SafeProxyFactory contract deployed on a specific network */
safeProxyFactoryAddress: string
/** safeProxyFactoryAbi - Abi of the Gnosis Safe Proxy Factory contract deployed on a specific network */
/** safeProxyFactoryAbi - Abi of the Gnosis SafeProxyFactory contract deployed on a specific network */
safeProxyFactoryAbi?: AbiItem | AbiItem[]
}

Expand Down
8 changes: 4 additions & 4 deletions packages/safe-core-sdk/tests/contractManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ describe('Safe contracts manager', () => {
.to.be.rejectedWith(
process.env.ETH_LIB === 'web3'
? 'You must provide the json interface of the contract when instantiating a contract object'
: 'Invalid Multi Send contract address'
: 'Invalid MultiSend contract address'
)
})

it('should fail if Safe Proxy contract is not deployed on the current network', async () => {
it('should fail if SafeProxy contract is not deployed on the current network', async () => {
const { accounts, contractNetworks } = await setupTests()
const [account1] = accounts
const ethAdapter = await getEthAdapter(account1.signer)
Expand All @@ -61,7 +61,7 @@ describe('Safe contracts manager', () => {
contractNetworks
})
)
.to.be.rejectedWith('Safe Proxy contract is not deployed on the current network')
.to.be.rejectedWith('SafeProxy contract is not deployed on the current network')
})

it('should fail if MultiSend contract is specified in contractNetworks but not deployed', async () => {
Expand All @@ -88,7 +88,7 @@ describe('Safe contracts manager', () => {
contractNetworks: customContractNetworks
})
)
.to.be.rejectedWith('Multi Send contract is not deployed on the current network')
.to.be.rejectedWith('MultiSend contract is not deployed on the current network')
})

it('should set the MultiSend contract available on the current network', async () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/safe-core-sdk/tests/ethAdapters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ describe('Safe contracts', () => {
})

describe('getMultiSendContract', async () => {
it('should return a Multi Send contract from safe-deployments', async () => {
it('should return a MultiSend contract from safe-deployments', async () => {
const { accounts } = await setupTests()
const [account1] = accounts
const ethAdapter = await getEthAdapter(account1.signer)
Expand All @@ -121,7 +121,7 @@ describe('Safe contracts', () => {
.to.be.eq('0xA238CBeb142c10Ef7Ad8442C6D1f9E89e07e7761')
})

it('should return a Multi Send contract from the custom addresses', async () => {
it('should return a MultiSend contract from the custom addresses', async () => {
const { accounts, contractNetworks, chainId } = await setupTests()
const [account1] = accounts
const ethAdapter = await getEthAdapter(account1.signer)
Expand Down Expand Up @@ -176,7 +176,7 @@ describe('Safe contracts', () => {
})

describe('getSafeProxyFactoryContract', async () => {
it('should return a Safe Proxy Factory contract from safe-deployments', async () => {
it('should return a SafeProxyFactory contract from safe-deployments', async () => {
const { accounts } = await setupTests()
const [account1] = accounts
const ethAdapter = await getEthAdapter(account1.signer)
Expand All @@ -193,7 +193,7 @@ describe('Safe contracts', () => {
.to.be.eq('0xa6B71E26C5e0845f74c812102Ca7114b6a896AB2')
})

it('should return a Safe Proxy Factory contract from the custom addresses', async () => {
it('should return a SafeProxyFactory contract from the custom addresses', async () => {
const { accounts, contractNetworks, chainId } = await setupTests()
const [account1] = accounts
const ethAdapter = await getEthAdapter(account1.signer)
Expand Down
Loading