Handle write contract functions #19
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
README.md
Outdated
| const tx = await safeSdk.buildContractCall({ | ||
| method: SafeSettings.ENABLE_MODULE, | ||
| moduleAddress | ||
| }) | ||
| ``` | ||
|
|
||
| #### Transaction to disable a Safe module. | ||
|
|
||
| ```js | ||
| const tx = await safeSdk.buildContractCall({ | ||
| method: SafeSettings.DISABLE_MODULE, | ||
| moduleAddress | ||
| }) |
There was a problem hiding this comment.
I'm curious why you chose the approach of having one generic method to perform safe contract calls.
IMO It feels too "raw" or low-level. I think it's handier to have a method responsible for the call (enableModule, disableModule, etc). This buildContractCall could be used in the internal realization. You can find an example here: https://github.com/gnosis/safe-apps-sdk/blob/master/packages/safe-apps-sdk/src/eth/index.ts#L84 Probably, not the best one 🙈 I also found that in web3.js repo, they also use that approach to build methods for rpc calls
There was a problem hiding this comment.
This is a good comment :)
I had it implemented that way before refactoring to just just have one generic method. You can check here the commit and the methods I had previously:
35c0aa1#diff-9d1172cb852904ea345f63e4470d48e4b305c14b39b6ef97202c28c356e58676L25
So it is this:
getEnableModuleTx(moduleAddress: string): Promise<SafeTransaction>
getDisableModuleTx(moduleAddress: string): Promise<SafeTransaction>
getAddOwnerTx(ownerAddress: string, threshold?: number): Promise<SafeTransaction>
getRemoveOwnerTx(ownerAddress: string, threshold?: number): Promise<SafeTransaction>
getSwapOwnerTx(oldOwnerAddress: string, newOwnerAddress: string): Promise<SafeTransaction>
getChangeThresholdTx(threshold: number): Promise<SafeTransaction>vs. this:
buildContractCall(params: ContractCallParams): Promise<SafeTransaction>I don't really have a strong opinion on this. I unified them because I thought the logic/concept (not the implementation) was almost the same: to get the encoding of a contract call data. So why not using just one function passing the contract method name as an argument?
Does this look too low-level? It may be.
What do you think @rmeissner? Do you also agree that is easier for the devs to have one independent method for each contract call?
There was a problem hiding this comment.
Hmm I also don't have a strong opinion. Ethers allows you to do both approaches, right? Not sure what is best. Probably the one where autocomplete works the best :P
src/managers/types.ts
Outdated
| @@ -0,0 +1,49 @@ | |||
| export enum SafeSettings { | |||
There was a problem hiding this comment.
I can see an import in index.ts for this file:
import { ContractCallParams, SafeSettings } from './managers/types'
I couldn't find any other ones, so seems not all types are used.
There was a problem hiding this comment.
I forgot to remove them. They are not used with the Specific methods for each contract call.
| to: this.getAddress(), | ||
| value: '0', | ||
| data: await this.#moduleManager.encodeEnableModuleData(moduleAddress), | ||
| nonce: (await this.#contract.nonce()).toNumber() |
There was a problem hiding this comment.
nitpick: I can see at some time in the future a feature request for a way to set a custom nonce. No need to address it now but better to keep it in mind when designing the project.
There was a problem hiding this comment.
This was implemented in the other PR with some other improvements. (Not pushed yet) :)
src/managers/moduleManager.ts
Outdated
| if (!isValidAddress || moduleAddress === zeroAddress || moduleAddress === SENTINEL_MODULES) { | ||
| throw new Error('Invalid module address provided') | ||
| } |
There was a problem hiding this comment.
Could be possibly extracted to a function for reusability and readability (as it will have a meaningful name)
Check #9