Chain-specific Safe addresses #560
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Pull Request Test Coverage Report for Build 6627146939
💛 - Coveralls |
|
I have read the CLA Document and I hereby sign the CLA |
tmjssz
force-pushed
the
chain-specific-safe-addresses
branch
from
172daf7
to
6602eba
Compare
| @@ -1,5 +1,6 @@ | |||
| import chai from 'chai' | |||
| import chaiAsPromised from 'chai-as-promised' | |||
| import { keccak256 } from 'ethereumjs-util' | |||
There was a problem hiding this comment.
@DaniSomoza this lib is deprecated at your PR, could you recommend an alternative here?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thx, I updated the PR to use @noble/hashes now.
| * Provides a default salt nonce with appended chainId to generate different addresses for the | ||
| * same Safe in different networks. | ||
| */ | ||
| export function getChainSpecificDefaultSaltNonce(chainId: number): string { |
There was a problem hiding this comment.
What is your opinion about to provide a full JSDoc for this function?. Here's a suggested format:
/**
* Provides a chain-specific default salt nonce for generating unique addresses
* for the same Safe configuration across different chains.
*
* @param {number} chainId - The chain ID associated with the chain.
* @returns {string} The chain-specific salt nonce in hexadecimal format.
*/There was a problem hiding this comment.
Yes makes total sense! Added it
There was a problem hiding this comment.
🤓 : the largest number in javascript is (2^53) – 1, but chain IDs in ethereum theoretically can be 256 bits numbers
There was a problem hiding this comment.
From here:
Adds a new opcode CHAINID at 0x46, which uses 0 stack arguments. It pushes the current chain ID onto the stack. Chain ID is a 256-bit value. The operation costs G_base to execute.
There was a problem hiding this comment.
Ah ok, so BigInt should be used? But would it not be better then to change it everywhere in the repo, instead of only in this function? Because the ethAdapter.getChainId() returns a simple number value and that is where the chainId comes from when being passed to this function.
There was a problem hiding this comment.
Ah, then it is irrelevant for this PR. Thanks for the explanation
There was a problem hiding this comment.
Makes sense! I will update the type to BigInt in the PR of the Ethers v6 migration.
There was a problem hiding this comment.
BigInt or string both should work. Ethers also has a BigNumberish type
There was a problem hiding this comment.
What it solves
Resolves #464
How this PR fixes it
Adding the
getChainSpecificDefaultSaltNoncefunction which appends the givenchainIdto thePREDETERMINED_SALT_NONCE. This results in different default addresses for the Safe contract on different chains.Example
Deployed two Safes with identical parameters on two different chains without defining a custom salt nonce. Received different addresses:
0x6a724ffd8bab7ce14144cbfafa44994121ec8b160x0c78086f0b398d044e54dcfdf86387942ef05713