Conversation
* chore: update OZ lib to V5 * chore: update openzeppelin to v5 - updated paths were needed - added constructor arguments for Ownable update - updated OZErrors and the expectRevert that were using them to work with custom errors - replaces Address.isContract with the logic it used, as it has been removed * chore: update Solidity version 0.8.19 -> 0.8.20 * format: format code * chore: fix CI and update version The CI failed because of imports, which are now corrected. Next to this we decided to set the Solidity version to 0.8.23 because it will be more up-to-date when we launch to mainnet * Delete .gas-snapshot * Update .gitignore * Change TestsupportsInterface to only check immediate Iterfaces --------- Co-authored-by: hieser <47272854+FHieser@users.noreply.github.com> Co-authored-by: FHieser <felix_hieser@web.de>
…thub-ci-to-run-scripts sc 126: update GitHub ci to run scripts
CI FIX: solved address collision in bc test
* chore: update OZ lib to V5 * chore: update openzeppelin to v5 - updated paths were needed - added constructor arguments for Ownable update - updated OZErrors and the expectRevert that were using them to work with custom errors - replaces Address.isContract with the logic it used, as it has been removed * chore: update Solidity version 0.8.19 -> 0.8.20 * format: format code * 394 metatransactions dev (#399) * Modify ModuleManager to enact ERC2771 * Intermediate Push I need to check something and pushing makes most sense he * Fix inheritance to enable build * Expose isTrustedForwarder in Orchestrator * Implement modified ERC2771 to Module * Fix Tests A lot of modules havent been properly initialized in the tests. The fix targets that by actually using the proxy Clone implementation and then properly initializing them * Add Module Test * Implement MinimalForwarder * Fix MakeFile * Create E2ETest * Add specific Role section in E2E Test * Remove todo * Marvin G Fix * Foundryup formatting * Fix after openZepplelin Update * Create Transaction forwarder * Update IModule.sol * Change to Transaction forwarder * chore: fix CI and update version The CI failed because of imports, which are now corrected. Next to this we decided to set the Solidity version to 0.8.23 because it will be more up-to-date when we launch to mainnet * WIP * Fixing tests * Cleanup * Add Multicall Section into E2ETest * Cleanup * Merge fix * Fix based on comments * Delete .gas-snapshot * Update .gitignore * Change TestsupportsInterface to only check immediate Iterfaces * Fix merge * Create DeployAndSetUpTransactionForwarder.s.sol * Add Transaction Forwarder Script to deployment scripts --------- Co-authored-by: Zitzak <marvin88gr@gmail.com>
* Rename Contracts * Refactor Beacons and add Version function * Make it buildable * Fix test * Cleanup * Update InverterBeacon.t.sol * Update InverterBeacon.t.sol * add BeaconProxy Test * Clarify Outcommented * Fix Beacon Upgrade Script * Update UpgradeBeacon.s.sol * Add Intervention Mechanism to InverterBeacon * Make it buildable * PR comment fix * Add tests * Add Interface * Cleanup * Move and update Beacon e2e * Update InverterBeaconE2E.t.sol * Cleanup * Adapt naming * Fix after merge
marvinkruse
requested review from
0xNuggan,
FHieser,
Zitzak,
ben-kaufman and
jellegerbrandy
src/modules/base/Module.sol
Outdated
| if (__Module_orchestrator.isTrustedForwarder(msg.sender)) { | ||
| // The assembly code is more direct than the Solidity version using `abi.decode`. | ||
| /// @solidity memory-safe-assembly | ||
| assembly { |
There was a problem hiding this comment.
Should also verify calldatasize is more than 20 to be sure
There was a problem hiding this comment.
It is better to use the ERC2771Context methods from OZ instead of writing your own, to avoid these kind of mistakes, toh here as in _msgData. You can override isTrustedForwarder easily
Contributor
There was a problem hiding this comment.
I did it as you recommended and inherited from ERC2771Context
I hope I did the inheritance correctly this time :3
Contributor
|
Last change looks good to me. I still think maybe it's too verbose to declare the isTrustedForwarder in the Orchestrator itself only to overwrite it, but as we said it's just taste, and if it makes interactions from other Modules easier then so be it :) |
0xNuggan
approved these changes
Apr 12, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
reviewinto thedevbranch.At the time of creating this PR (until it is merged), the
reviewbranch contains the latest state that has received a security review, including the fixes. Anything inreviewhas been reviewed internally, but not received a security review. We are merging intodevas the currentdevbranch is still pending the finalization of the audit.➡️ Process
This PR will receive an external security review. Additionally, at least one of our developers will review it as well, ideally someone who didn't work on it. Please raise any findings/questions directly within this PR, either as a code comment, referencing the corresponding line or as a general comment to the discussion. Please mark comments that are resolved as resolved.
📋 Contents
This PR contains...