chore: add support for Call3Value calls #132
Conversation
🟡 Heimdall Review Status
|
jackchuma
left a comment
jackchuma
left a comment
There was a problem hiding this comment.
I think it all looks great - might just want to add some testing around the new contracts added? (MultisigBuilderNoValue, MultisigBuilderWithValue, NestedMultisigBuilderNoValue, NestedMultisigBuilderWithValue)
|
@jackchuma I refactored the approach to instead introduce an |
xenoliss
force-pushed
the
bo/multicall-Call3Value
branch
from
2406fe0 to
c57a17c
Compare
xenoliss
changed the title
xenoliss
changed the title
| /** | ||
| * @notice Returns whether ETH transfers are allowed to be performed by the Multicall calls. | ||
| */ | ||
| function _allowEthTransfer() internal view virtual returns (bool) { |
There was a problem hiding this comment.
Doesn't have to be in this PR but I'm noticing now that all these virtual functions could be moved to MultisigBase.sol so they only need to be defined once
There was a problem hiding this comment.
I like the consistency of keeping this alongside _buildCalls(), so this seems okay for now. But we could consider moving all of the common virtual functions up a level in a follow up?
| run(); | ||
| } | ||
|
|
||
| function testRevert_buildCalls() external { |
There was a problem hiding this comment.
Interesting, it passes locally. Can you post the error you get?
There was a problem hiding this comment.
Should be solved, let me know.
| } | ||
|
|
||
| function _simulateForSigner(address _safe, IMulticall3.Call3[] memory _calls) | ||
| function _buildCallsChecked() private view returns (IMulticall3.Call3Value[] memory) { |
There was a problem hiding this comment.
what's the reason for the additional _allowEthTransfer() method gating? can we just trust the simulation / verification done by signers?
There was a problem hiding this comment.
The reason is mostly for extra security. I am fine removing it but there was some concern around missing a transfer.
There was a problem hiding this comment.
I personally prefer the simplicity of what we had previously... it should be up to the signers to validate what is happening, including value transfers. There doesn't seem to be a particular reason to single out value transfer from the safes vs other actions the safe does. But don't feel strongly.
cb-heimdall
dismissed
mdehoog’s stale review
Approved review 2761889576 from mdehoog is now dismissed due to new commit. Re-request for approval.
|
Review Error for jackchuma @ 2025-04-12 11:47:21 UTC |
|
Review Error for BoomchainLabs @ 2025-04-13 03:39:41 UTC |
| bytes32 hash = _getTransactionHash(_safe, _data); | ||
| _signatures = Signatures.prepareSignatures(_safe, hash, _signatures); | ||
|
|
||
| bytes memory simData = _execTransactionCalldata(_safe, data, _signatures); |
|
Closing because out of date. |
This PR adds support for
Call3Valuecalls by updating theMultisigBuilderandNestedMultisigBuildercontracts.It also adds new specific Multisig (and NestedMultisig) builder implementations that tasks should now inherit from:MultisigBuilderNoValue: for tasks that are not supposed to transfer ETH in the Multicall calls.MultisigBuilderWithValue: for tasks that do transfer ETH in the Multicall calls.An additional virtual method
_allowEthTransferis added to these contracts, with a default implementation that returnsfalse.The
sign,approve, andrunmethods now rely on calling an internal_buildCallsCheckedmethod that ensures the calls returned by_buildCallsare consistent with the policy defined by the_allowEthTransfermethod.