fix: don't use chainId when calculating domain hash of <=1.2.0
#4616
Conversation
Branch preview✅ Deploy successful! Storybook: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
-
Data Generation Consistency:
faker.string.numeric()may generate a numeric string that does not accurately represent a valid Ethereum chain ID. Consider using a predefined valid chain ID for tests to ensure consistency. Example:const chainId = '1'; // Mainnet. -
Version Handling: Consider refactoring version strings ('1.0.0', '1.1.1', '1.2.0', '1.3.0', '1.4.1') to constants or enumerations to improve maintainability. This can prevent errors due to typos in version strings and make changes easier if more versions are added later.
-
Test Coverage: Check if you've covered all relevant versions, including potential edge cases not covered in major or minor versions (e.g., patch versions).
-
Redundancy in Code: The two
it.eachblocks are similar except for the domain hash logic. Consider creating a helper function that abstracts out the common code and accepts parameters for differences. This will adhere to the DRY (Don't Repeat Yourself) principle:function performDomainHashTest(version, typeHash, includeChainId) { const chainId = '1'; // or other valid chain ID const safeAddress = faker.finance.ethereumAddress(); const result = getDomainHash({ chainId, safeAddress, safeVersion: version }); const encodedParameters = includeChainId ? ['bytes32', 'uint256', 'address'] : ['bytes32', 'address']; const expectedHash = keccak256( AbiCoder.defaultAbiCoder().encode( encodedParameters, includeChainId ? [typeHash, chainId, safeAddress] : [typeHash, safeAddress] ) ); expect(result).toEqual(expectedHash); }
-
Magic Strings: The hardcoded type hashes (
OLD_DOMAIN_TYPEHASHandNEW_DOMAIN_TYPEHASH) could be accompanied by comments that describe their origin or computations, ensuring comprehension when the code is revisited.
Overall, the approach seems effective for the intended functionality, but consider the above points to enhance code robustness and clarity.
| chainId, | ||
| verifyingContract: safeAddress, | ||
| }) | ||
| const domainHash = getDomainHash({ chainId, safeAddress, safeVersion }) | ||
| const messageHash = safeTxData | ||
| ? TypedDataEncoder.hashStruct('SafeTx', { SafeTx: getEip712TxTypes(safeVersion).SafeTx }, safeTxData) | ||
| : undefined |
There was a problem hiding this comment.
-
Inline Function Complexity: The logic for determining whether to include the
chainIdin the domain hash can be simplified for readability. Consider extracting the version check logic into a separate variable for clarity.const includeChainId = semverSatisfies(safeVersion, NEW_DOMAIN_TYPE_HASH_VERSION); return TypedDataEncoder.hashDomain({ ...(includeChainId && { chainId }), verifyingContract: safeAddress, });
-
Type Consistency: Ensure all types are declared and used consistently. The type annotations for the
getDomainHashparameters are correct but double-check that all related functions and components use consistent types, especially forSafeVersion. -
DRY Principle: The logic for creating the domain hash is now effectively extracted into
getDomainHash, improving maintainability. However, ensure that this function is reused throughout the codebase wherever the domain hash is constructed to adhere to the DRY principle.
No other significant issues found.
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
1006.28 KB (🟡 +66 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Coverage report
Test suite run success1667 tests passing in 227 suites. Report generated by 🧪jest coverage report action from 0fa8cff |
| safeVersion: SafeVersion | ||
| }): string { | ||
| return TypedDataEncoder.hashDomain({ | ||
| ...(semverSatisfies(safeVersion, NEW_DOMAIN_TYPE_HASH_VERSION) && { chainId }), |
There was a problem hiding this comment.
ChatGPT has a point, it would be more readable with
const includeChainId = semverSatisfies(safeVersion, NEW_DOMAIN_TYPE_HASH_VERSION)
|
|
||
| export function getDomainHash({ | ||
| chainId, | ||
| safeAddress, |
There was a problem hiding this comment.
No significant issues found.
-
Consider renaming
includeChainIdto more clearly indicate its purpose, such asisChainIdIncluded, to improve readability. -
Ensure that
semverSatisfiesandNEW_DOMAIN_TYPE_HASH_VERSIONare properly defined and imported, as they are critical for the logic.
|
Verified |
What it solves
Resolves #4613
How this PR fixes it
For Safe versions <= 1.2.0, the domain separator typehash of the Safe contracts does not include a
chainId:For versions >= 1.3.0, a
chainIdis included:When calculating the domain hash, we do not take the contract version into account, calculating based on the newer domain separator typehash.
This conditionally includes the
chainIdwhen calculating the domain hash for the current Safe, therefore correcting it for <= 1.2.0 versions.How to test it
Using a Ledger device, observe the domain hash matching that in the interface for all versions from >= 1.0.0.
Checklist