Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to implicitly specify "self" in multi-send #695

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

Philogy
Copy link
Contributor

@Philogy Philogy commented Nov 8, 2023

Safes may want to be able to use multi-send for combining configuring calls such as enableModule, setGuard with each other or other external calls however specifying the safe's own address is impossible to currently do in initialization calls as the create2 salt for the safe depends on the hash of the initializer data which creates an unresolvable circular dependency. Allowing address(0) to act as an alias for "self" allows configuring calls to easily be specified at initialization which is useful for people who want to deploy safes that enable certain modules/guards from the start.

Implementation Explanation

The expression uses a binary-OR combined with a multiplication to implement a branch-less version of the following statement:

to = to == address(0) ? address(this) : to;

Here's the reasoning for why or(to, mul(iszero(to), address()) is equivalent to the above ternary expression:

if (to == 0) { or(to, mul(address(), iszero(to))) } else /* to == [1, 2^160-1] */ {  or(to, mul(address(), iszero(to))) }
if (to == 0) { or(0 , mul(address(), 1)         ) } else /* to == [1, 2^160-1] */ {  or(to, mul(address(),     0     )) }
if (to == 0) { or(0 ,     address()             ) } else /* to == [1, 2^160-1] */ {  or(to,                    0      ) }
if (to == 0) {            address()               } else /* to == [1, 2^160-1] */ {     to                              }

Copy link

github-actions bot commented Nov 8, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Philogy
Copy link
Contributor Author

Philogy commented Nov 8, 2023

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Nov 8, 2023
@nlordell
Copy link
Collaborator

nlordell commented Nov 9, 2023

Hey, thanks for the contribution. Personally, I think this is a very cool idea:

  1. Sending transactions to address 0 is not really useful in and of itself (you can always use another address to burn ETH such as 0xee..ee), so it doesn't really impose any practical restrictions on MultiSend contract usage
  2. Generally speaking it allows the Safe setup to do a lot of things (where you don't know the Safe address upfront, so you can use address(0) in the to to call enableModule multiple times for example instead of requiring a specialised contract to do so).
  3. The solution uses an elegant branchless programming trick and has a low gas overhead

The biggest drawback to the change is the requirement to deploy new MultiSend* contracts and update where they get used, but I do think that it is useful enough to merit the downsides.

@mmv08
Copy link
Member

mmv08 commented Nov 9, 2023

Hey, thanks for the contribution. Personally, I think this is a very cool idea:

  1. Sending transactions to address 0 is not really useful in and of itself (you can always use another address to burn ETH such as 0xee..ee), so it doesn't really impose any practical restrictions on MultiSend contract usage
  2. Generally speaking it allows the Safe setup to do a lot of things (where you don't know the Safe address upfront, so you can use address(0) in the to to call enableModule multiple times for example instead of requiring a specialised contract to do so).
  3. The solution uses an elegant branchless programming trick and has a low gas overhead

The biggest drawback to the change is the requirement to deploy new MultiSend* contracts and update where they get used, but I do think that it is useful enough to merit the downsides.

We can merge this and include it in the upcoming 1.5 release.

@Philogy do you think you'd have time to add a couple of tests for your change?

@coveralls
Copy link

coveralls commented Nov 9, 2023

Pull Request Test Coverage Report for Build 6800828476

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.307%

Totals Coverage Status
Change from base Build 6720462527: 0.0%
Covered Lines: 400
Relevant Lines: 409

💛 - Coveralls

@mmv08 mmv08 merged commit 5aad84f into safe-global:main Nov 13, 2023
7 of 12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants