Refactor MultisigScript

[xenoliss]

This PR refactors the MultisigScript logic to remove unnecessary Multicall wrapping, provide more flexibility when building script calls, and use our custom CBMulticall instead of the canonical implementation.

The _buildCalls() method now returns an array of Call structs:

struct Call {
    Enum.Operation operation;
    address target;
    bytes data;
    uint256 value;
}

Each call specifies its own execution mode (Call or DelegateCall) via the operation field. The internal _buildCallsChecked() method validates these calls, preventing invalid combinations such as non-zero value for delegate calls.

The Safe-to-Safe approval chain has been simplified: approvals are now direct approveHash calls without any multicall wrapping.

The only remaining multicall wrapping occurs when _buildCalls() returns multiple calls. If there is exactly one call, it is executed directly. When multiple calls exist, _buildAggregatedScriptCall() groups and aggregates compatible calls. For example, the following calls returned by _buildCalls():

[
    Call(Call, ContractA, abi.encodeCall(a, ()), 0),
    Call(Call, ContractB, abi.encodeCall(b, ()), 0),
    Call(DelegateCall, OPCM, abi.encodeCall(upgrade, ()), 0),
    Call(Call, ContractC, abi.encodeCall(c, ()), 100),
    Call(Call, ContractD, abi.encodeCall(d, ()), 200)
]

are aggregated into:

CBMulticall.aggregateDelegateCalls(
    Call3(
        target = Multicall,
        allowFailure = false,
        calldata = abi.encodeCall(Multicall.aggregate3, (
            Call3(ContractA, false, abi.encodeCall(a, ())),
            Call3(ContractB, false, abi.encodeCall(b, ()))
        ))
    ),
    Call3(
        target = OPCM,
        allowFailure = false,
        calldata = abi.encodeCall(upgrade, ())
    ),
    Call3(
        target = Multicall,
        allowFailure = false,
        calldata = abi.encodeCall(Multicall.aggregate3Value, (
            Call3Value(ContractC, false, 100, abi.encodeCall(c, ())),
            Call3Value(ContractD, false, 200, abi.encodeCall(d, ()))
        ))
    )
)

[cb-heimdall]

🟡 Heimdall Review Status

Requirement	Status	More Info
Reviews	🟡 0/1	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 1 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 0 Global minimum 0 Max 1 1 1 if commit is unverified 0 Sum 1

[jackchuma]

I'm not sure we need to support combinations of call + delegate calls. We've never needed that before and it's unknown if we will in the future, however this adds ~300 lines of code to support this use case. It could end up being useful, so I'm ok with adding this as an option. Just pointing out that it still is inherently a confusing component that isn't super easy to work with

[jackchuma]

this is unused in the script

[xenoliss]

Yes but I figured it might still be useful if a script needs to do it (unlikely but it might be the case that we want to build a call to a Multicall from within the script directly). I don't feel strongly here though and ok to remove.

[jackchuma]

this is unused in the script

[xenoliss]

Yes but I figured it might still be useful if a script needs to do it (unlikely but it might be the case that we want to build a call to a Multicall from within the script directly). I don't feel strongly here though and ok to remove.

[jackchuma]

is value alwayas 0?

[xenoliss]

I think the value would/should already be on the safe

[xenoliss]

@jackchuma

I'm not sure we need to support combinations of call + delegate calls.

I agree that we don’t need to support combinations of call and delegatecall, although future use cases might eventually require it. The only reason it is currently supported is because the user can explicitly set the operation for each call in _buildCalls().

however this adds ~300 lines of code to support this use case.

It’s closer to ~150 additional lines. A good portion of that comes from adding proper NatSpec comments that weren’t present before. Still, I agree that handling the _buildCalls() output requires slightly more logic.

Just pointing out that it still is inherently a confusing component that isn't super easy to work with

I also agree that this component has always been somewhat confusing. I find this version easier to reason about, because the script only manipulates Calls and directly expresses how each call should be executed by the Safe. By contrast, I find the existing _useDelegateCalls() flag ambiguous: does it mean we delegatecall into the Multicall contract, or that we perform a regular call into a multicall that itself performs delegatecalls? Or does it cover both cases?

I feel like the assumptions are clearer and safer in this version. We always delegatecall into a multicall, approvals are always performed with regular calls, and we avoid using a multicall when there is only a single call to execute.

[jackchuma]

Not overly opinionated on this but I'd prefer to leave the linter enabled. I've found it helpful

[xenoliss]

This is a temporary workaround for this issue foundry-rs/foundry#11668

[jackchuma]

Style guide returns

[xenoliss]

Not too opinionated but I think implicit return makes sense here as we're building the array element by element (and the returned value is named)