Safe module tutorial #670
Safe module tutorial #670
Conversation
Branch preview✅ Deployed successfully in branch deployment: |
|
Overall readability score: 31.4 (🟢 +0.08)
View detailed metrics🟢 - Shows an increase in readability
Averages:
View metric targets
|
pages/advanced/smart-account-modules/smart-account-modules-tutorial.mdx
Outdated
Show resolved
Hide resolved
pages/advanced/smart-account-modules/smart-account-modules-tutorial.mdx
Outdated
Show resolved
Hide resolved
pages/advanced/smart-account-modules/smart-account-modules-tutorial.mdx
Outdated
Show resolved
Hide resolved
|
|
||
| Before progressing with the tutorial, please make sure you have the following: | ||
|
|
||
| - Downloaded and installed [Node.js](https://nodejs.org/en/download/package-manager) and [pnpm](https://pnpm.io/installation). |
There was a problem hiding this comment.
here it mentions pnpm but then throughout the tutorial npm is used
|
|
||
| ```json | ||
| ... | ||
| "overrides": { |
There was a problem hiding this comment.
I think you can include a more specific overide:
"overrides": {
"@safe-global/safe-contracts": {
"ethers": "^6.13.5"
}
}
|
|
||
| ### Initialize hardhat project | ||
|
|
||
| Select create TypeScript project. |
There was a problem hiding this comment.
should be after calling the init command right?
There was a problem hiding this comment.
The other options to choose should be namen, too.
(If it is possible to start with an empty hardhat.config, because it gets overwritten in the next step, this option should be chosen because it is easiest.
|
|
||
| ### Update hardhat.config.ts | ||
|
|
||
| When compiling Safe contracts with solidity 0.8.x the bytecode size exceeds the limit of 24KB. To overcome this, set `allowUnlimitedContractSize` to `true` in the hardhat config. |
There was a problem hiding this comment.
this means after the tutorial the contract will not be deployable to public networks, not sure if this is a good experience
There was a problem hiding this comment.
In practice, users will only deploy module code and not the Safe contracts.
| ); | ||
|
|
||
| bytes32 hash = keccak256( | ||
| abi.encodePacked("\x19\x01", getDomainSeparator(), signatureData) |
There was a problem hiding this comment.
getDomainSeparator is used here but only added in the next step. I'd define everything that's needed for signing first
|
|
||
| ```solidity | ||
| function getDomainSeparator() private view returns (bytes32) { | ||
| uint256 chainId; |
There was a problem hiding this comment.
in 0.8.x block.chainid can be used
pages/advanced/smart-account-modules/smart-account-modules-tutorial.mdx
Outdated
Show resolved
Hide resolved
…orial.mdx Co-authored-by: Mikhail <[email protected]>
…orial.mdx Co-authored-by: Mikhail <[email protected]>
…orial.mdx Co-authored-by: Mikhail <[email protected]>
…orial.mdx Co-authored-by: Mikhail <[email protected]>
There was a problem hiding this comment.
Overall a great tutorial. I especially like the gradual process, as it focusses the learner's attention on the individual added code blocks.
There seems to be an issue with TestToken which is not part of the tutorial code, which needs to be resolved.
TokenTransferModule Tests
1) "before all" hook for "Should successfully transfer tokens to bob"
0 passing (594ms)
1 failing
1) TokenTransferModule Tests
"before all" hook for "Should successfully transfer tokens to bob":
HardhatError: HH700: Artifact for contract "TestToken" not found.
|
|
||
| This tutorial will guide creating a Safe Module that allows users to withdraw ERC20 tokens from a Safe account, provided that the Safe owners have given their approval in the form of off-chain signatures. The primary use case is to enable beneficiaries to withdraw tokens themselves without requiring the Safe owners to execute transactions directly. | ||
|
|
||
| ### Use cases |
There was a problem hiding this comment.
This list is a duplication of the text above. Choose either one (I would go with the text).
| ### Limitations | ||
| - Beneficiaries must be externally owned accounts (EOAs) and cannot be smart contracts. | ||
| - The nonce is sequential for simplicity | ||
| - The contract will work only for specific token and Safe address provided during deployment. |
There was a problem hiding this comment.
This is gramatically incorrect or hard to understand. (I recommend running all of the text through ChatGPT).
| ``` | ||
|
|
||
| ```bash | ||
| npm init |
There was a problem hiding this comment.
Could you add a line like "you can choose all default values".
Also, it would be nice to have a line that says, "add npm-modules to .gitignore." Or you leave out the git init command.
|
|
||
| ### Initialize hardhat project | ||
|
|
||
| Select create TypeScript project. |
|
|
||
| ### Initialize hardhat project | ||
|
|
||
| Select create TypeScript project. |
There was a problem hiding this comment.
The other options to choose should be namen, too.
(If it is possible to start with an empty hardhat.config, because it gets overwritten in the next step, this option should be chosen because it is easiest.
|
|
||
| Create a new file named `utils.ts` in the `test/utils` directory and include the code below. This file contains utility functions to generate the EIP-712 digest and execute transaction through the Safe account. | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
This file is wordy :D Does it have to be so wordy? Do the digest and the other hashes need to be generated on the spot, or are they constant? Is there any way to shorten the code and make it more easy to understand?
| }; | ||
| ``` | ||
|
|
||
| #### Step 3: Setup contracts and variables |
There was a problem hiding this comment.
Change the order, so that it is code first, and then explanation to be consistent.
Also, only include the before section, because the import scripts are added in the skeleton file.
|
|
||
| ``` | ||
|
|
||
| #### Step 4: Add test case |
There was a problem hiding this comment.
As above, change the order, so it is code first, and then the explanation.
| import { expect } from "chai"; | ||
| import { Signer, ZeroAddress } from "ethers"; | ||
| import { Safe__factory, TestToken, TokenWithdrawModule } from "../typechain-types"; | ||
| import { SafeTransaction } from "@safe-global/safe-contracts"; |
There was a problem hiding this comment.
Remove this line. This import is not used and the line is removed from the complete file.
| import { ethers } from "hardhat"; | ||
| import { expect } from "chai"; | ||
| import { Signer, ZeroAddress } from "ethers"; | ||
| import { Safe__factory, TestToken, TokenWithdrawModule } from "../typechain-types"; |
There was a problem hiding this comment.
I get an error here. Module '"../typechain-types"' has no exported member 'TestToken'.
Fixes: https://github.com/orgs/safe-global/projects/29/views/1?pane=issue&itemId=92625006&issue=safe-global%7Cdeveloper-experience%7C328
Changes in PR: