Generalize the nested multisig #136
Conversation
✅ Heimdall Review Status
|
mdehoog
force-pushed
the
michael/generalize-multisig
branch
2 times, most recently
from
d9d1261 to
0f4b5e5
Compare
mdehoog
force-pushed
the
michael/generalize-multisig
branch
3 times, most recently
from
bb928e3 to
2d18709
Compare
mdehoog
force-pushed
the
michael/generalize-multisig
branch
from
2d18709 to
f18c457
Compare
| * Same as `sign()` for non-nested safes. | ||
| */ | ||
| function sign() public { | ||
| sign(new address[](0)); |
There was a problem hiding this comment.
are these overloads helpful or confusing? also below
There was a problem hiding this comment.
Seems unnecessary in my opinion
There was a problem hiding this comment.
Opted to keep them for backwards compatibility with existing scripts, but marked them as deprecated
| // first try SAFE_NONCE | ||
| try vm.envUint("SAFE_NONCE") { | ||
| nonce = vm.envUint("SAFE_NONCE"); | ||
| } catch {} |
There was a problem hiding this comment.
This is slightly different in that it now tries to load SAFE_NONCE for all levels of the safe, rather than just the non-nested version. Retained for backwards compatibility.
script/universal/MultisigBuilder.sol
Outdated
| /* | ||
| * Same as `verify()` for a double layer of nesting. | ||
| */ | ||
| function verify(address _signerSafe, address _intermediateSafe, bytes memory _signatures) public { |
There was a problem hiding this comment.
If keeping these, this and the following verify can be marked as view
|
|
||
| emit DataToSign(txData); | ||
|
|
||
| console.log("---\nIf submitting onchain, call Safe.approveHash on %s with the following hash:", _safe); |
There was a problem hiding this comment.
Do we need this log? I know it was already there but doesn't seem relevant to our ops process
There was a problem hiding this comment.
yeah not relevant to us, but for the OP multisig one of their signers is a multisig, so they added this to make it easier to submit an approveHash rather than generate an EOA sig. Kept in for that purpose
With the
DoubleNestedMultisigBuilderaddition (#112), maintaining different implementations for various levels of nesting has gotten complicated.This PR replaces the
MultisigBuilder,NestedMultisigBuilder, andDoubleNestedMultisigBuilderwith a single contract that supports arbitrary levels of nesting.Tested by retaining the existing tests but replacing the implementation with the generalized version.