Skip to content

Commit 46f7c60

Browse files
akshay-apnlordell
andauthored
Fix compilation issue with solidity versions >= 0.8.12 (safe-global#772)
Fixes: safe-global#735 and safe-global#773 ### Changes in PR - Add a hook function `_onAfterExecTransactionFromModule` that gets called after execution from module. - SafeL2 overrides this function to emit event `SafeModuleTransaction`. - Add test that checks if `execTransactionFromModuleReturnData` in SafeL2 emits `SafeModuleTransaction` event. #### Codesize increase by `33` bytes with compiler version 0.7.6 #### This PR: ``` Safe 21814 bytes (limit is 24576) SafeL2 22632 bytes (limit is 24576) ``` #### Main branch ``` Safe 21781 bytes (limit is 24576) SafeL2 22623 bytes (limit is 24576) ``` ### Impact on gas usage with `Safe` contract Changes in this PR add additional overhead of `49` gas #### This PR: ``` ·------------------------------------------------------------|----------------------------|-------------|------------------------------· | Solc version: 0.7.6 · Optimizer enabled: false · Runs: 200 · Block limit: 100000000 gas │ ·····························································|····························|·············|······························· | Methods │ ·····················|·······································|·············|··············|·············|···············|··············· | Contract · Method · Min · Max · Avg · # calls · usd (avg) │ ·····················|·······································|·············|··············|·············|···············|··············· | Safe · execTransactionFromModule · 51630 · 184223 · 127865 · 11 · - │ ·····················|·······································|·············|··············|·············|···············|··············· | Safe · execTransactionFromModuleReturnData · 52273 · 183979 · 107919 · 5 · - │ ``` #### Main branch ``` ·------------------------------------------------------------|----------------------------|-------------|------------------------------· | Solc version: 0.7.6 · Optimizer enabled: false · Runs: 200 · Block limit: 100000000 gas │ ·····························································|····························|·············|······························· | Methods │ ·····················|·······································|·············|··············|·············|···············|··············· | Safe · execTransactionFromModule · 51581 · 184186 · 127818 · 11 · - │ ·····················|·······································|·············|··············|·············|···············|··············· | Safe · execTransactionFromModuleReturnData · 52224 · 183930 · 107870 · 5 · - │ ``` Why this approach? - Clean code - Introduces a hook that allows contracts inheriting Safe to do additional post processing. **Alternatives considered** A. - Move the function `execTransactionFromModule(...)` from `ModuleManager` to `Safe` contract. safe-global@3521a4b - But this is not an ideal approach as `ModuleManager` does not implement `execTransactionFromModule`. - Pro: Minimal code changes, no impact on codesize B. - Create `_onExecTransactionFromModule` method that gets called by `execTransactionFromModule` - ~~Con: This approach still has repeating code~~ - Approach is similar to `A` with new function. Here, `_onExecTransactionFromModule` still calls its parent. - Pro: Increase in codesize by only `24` bytes Side note: Why event `SafeModuleTransaction` is not emitted by `execTransactionFromModuleReturnData` in `SafeL2`? Why `SafeL2` does not override `execTransactionFromModuleReturnData`. -> `execTransactionFromModuleReturnData` in `SafeL2` should also emit event. Might have been missed in the past. ~~Would be covered in a separate PR.~~. This PR fixes the issue. --------- Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
1 parent 499b17a commit 46f7c60

File tree

3 files changed

+44
-8
lines changed

3 files changed

+44
-8
lines changed

contracts/SafeL2.sol

+8-6
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ import {Safe, Enum} from "./Safe.sol";
66
// Imports are required for NatSpec validation of the compiler, and falsely detected as unused by
77
// the linter, so disable the `no-unused-imports` rule for the next line.
88
// solhint-disable-next-line no-unused-import
9-
import {ISafe, IModuleManager} from "./interfaces/ISafe.sol";
9+
import {ISafe} from "./interfaces/ISafe.sol";
10+
// solhint-disable-next-line no-unused-import
11+
import {ModuleManager} from "./base/ModuleManager.sol";
1012

1113
/**
1214
* @title SafeL2 - An implementation of the Safe contract that emits additional events on transaction executions.
@@ -69,15 +71,15 @@ contract SafeL2 is Safe {
6971
}
7072

7173
/**
72-
* @inheritdoc IModuleManager
74+
* @inheritdoc ModuleManager
7375
*/
74-
function execTransactionFromModule(
76+
function onAfterExecTransactionFromModule(
7577
address to,
7678
uint256 value,
7779
bytes memory data,
78-
Enum.Operation operation
79-
) public override returns (bool success) {
80+
Enum.Operation operation,
81+
bool /*success*/
82+
) internal override {
8083
emit SafeModuleTransaction(msg.sender, to, value, data, operation);
81-
success = super.execTransactionFromModule(to, value, data, operation);
8284
}
8385
}

contracts/base/ModuleManager.sol

+19-2
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,11 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
152152
uint256 value,
153153
bytes memory data,
154154
Enum.Operation operation
155-
) public virtual override returns (bool success) {
155+
) public override returns (bool success) {
156156
(address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation);
157-
158157
success = execute(to, value, data, operation, type(uint256).max);
159158
postModuleExecution(guard, guardHash, success);
159+
onAfterExecTransactionFromModule(to, value, data, operation, success);
160160
}
161161

162162
/**
@@ -185,6 +185,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
185185
}
186186
/* solhint-enable no-inline-assembly */
187187
postModuleExecution(guard, guardHash, success);
188+
onAfterExecTransactionFromModule(to, value, data, operation, success);
188189
}
189190

190191
/**
@@ -275,4 +276,20 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
275276
moduleGuard := sload(slot)
276277
}
277278
}
279+
280+
/**
281+
* @notice A hook that gets called after execution of {execTransactionFromModule*} methods.
282+
* @param to Destination address of module transaction.
283+
* @param value Ether value of module transaction.
284+
* @param data Data payload of module transaction.
285+
* @param operation Operation type of module transaction.
286+
* @param success Boolean flag indicating if the call succeeded.
287+
*/
288+
function onAfterExecTransactionFromModule(
289+
address to,
290+
uint256 value,
291+
bytes memory data,
292+
Enum.Operation operation,
293+
bool success
294+
) internal virtual {}
278295
}

test/l2/Safe.Execution.spec.ts

+17
Original file line numberDiff line numberDiff line change
@@ -89,5 +89,22 @@ describe("SafeL2", () => {
8989
.to.emit(safe, "ExecutionFromModuleSuccess")
9090
.withArgs(user2.address);
9191
});
92+
93+
it("should emit SafeModuleTransaction event in execTransactionFromModuleReturnData", async () => {
94+
const {
95+
safe,
96+
mock,
97+
signers: [user1, user2],
98+
} = await setupTests();
99+
const mockAddress = await mock.getAddress();
100+
const user2Safe = safe.connect(user2);
101+
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);
102+
103+
await expect(user2Safe.execTransactionFromModuleReturnData(mockAddress, 0, "0xbaddad", 0))
104+
.to.emit(safe, "SafeModuleTransaction")
105+
.withArgs(user2.address, mockAddress, 0, "0xbaddad", 0)
106+
.to.emit(safe, "ExecutionFromModuleSuccess")
107+
.withArgs(user2.address);
108+
});
92109
});
93110
});

0 commit comments

Comments
 (0)