Compare all deployment type addresses when relaying #1955
Conversation
Pull Request Test Coverage Report for Build 11106301842Details
💛 - Coveralls |
| /** | ||
| * Helper to remap {@link SingletonDeploymentV2} to a list of checksummed addresses. | ||
| * | ||
| * @param getDeployments - deployment getters from @safe-global/safe-deployments |
| return formatDeployments(_getMultiSendDeployments, args); | ||
| } | ||
|
|
||
| type Filter = { |
There was a problem hiding this comment.
We should either move the type to the top of the file before the functions or another file. My suggestion is to separate concerns as much as possible
There was a problem hiding this comment.
I think separate files for types is quite excessive outside of a class interface. I moved it to the top of the file in 842426e.
| args.address || multiSendCallDeployment?.defaultAddress === args.address | ||
| ); | ||
| const multiSendCallDeployments = getMultiSendDeployments(args); | ||
| return multiSendCallDeployments.includes(args.address); |
There was a problem hiding this comment.
Nit: wdyt about simplifying this a bit by:
return (
getMultiSendCallOnlyDeployments(args).includes(args.address) ||
getMultiSendDeployments(args).includes(args.address)
);There was a problem hiding this comment.
Agreed. Changed in 79fa30c.
| const isL2Singleton = | ||
| safeL2Deployment?.networkAddresses[args.chainId] === singleton; | ||
| const isL1Singleton = safeL1Deployments.includes(singleton); | ||
| const isL2Singleton = safeL2Deployments.includes(singleton); |
There was a problem hiding this comment.
Nit: in this case, I think it could be also simplified, and optimized (potentially avoiding a second lookup):
return (
getSafeSingletonDeployments(args).includes(singleton) ||
getSafeL2SingletonDeployments(args).includes(singleton)
);Wdyt?
There was a problem hiding this comment.
Agreed. Changed in 79fa30c.
| multiSendCallDeployment?.networkAddresses[args.chainId] === | ||
| args.address || multiSendCallDeployment?.defaultAddress === args.address | ||
| ); | ||
| return getMultiSendDeployments(args).includes(args.address); |
There was a problem hiding this comment.
Nit: we could just
return getMultiSendCallOnlyDeployments(args).includes(args.address) || getMultiSendDeployments(args).includes(args.address);And therefore get rid of all the variables and the if statement, right? Do you think it would be less readable?
There was a problem hiding this comment.
Initially it was to avoid getting all deployment addresses. However, it's must simpler. Changed in c5115d5.
Adjusts all usage of the deployments package, ensuring we retrieve and compare against all deployment type addresses: - Create helpers to retrieve deployments, and add linting rule to ensure usage of them. - Modify relaying checks to compare against a list of addresses instead of a singular one. - Update tests accordingly.
Adjusts all usage of the deployments package, ensuring we retrieve and compare against all deployment type addresses: - Create helpers to retrieve deployments, and add linting rule to ensure usage of them. - Modify relaying checks to compare against a list of addresses instead of a singular one. - Update tests accordingly.
Summary
Resolves #1954
It is possible to get both singular or multiple deployment addresses since safe-global/safe-deployments#668 and, as such, there are respective
get*Deploymentandget*Deploymentsfunctions.Since this adjustment in the deployments package, we never took this into account in the project. Therefore, if a chain has both canonical and EIP-155 deployments, for example, we would only be retrieving the first address found.
This adjusts all usage of the deployments package, ensuring we retrieve and compare against all deployment type addresses.
Changes