feat: modify OptimismMintableERC20Factory for convert#17
Merged
agusduha merged 8 commits intosc/liquidity-migrationfrom Aug 13, 2024
Merged
feat: modify OptimismMintableERC20Factory for convert#17agusduha merged 8 commits intosc/liquidity-migrationfrom
agusduha merged 8 commits intosc/liquidity-migrationfrom
Conversation
0xDiscotech
reviewed
Aug 8, 2024
| /// @param _localToken The address of the ERC20 token to check the deployment. | ||
| /// @return _remoteToken The address of the remote token if it is deployed or `address(0)` if not. | ||
| function deployments(address _token) external view returns (address _remoteToken); | ||
| function deployments(address _localToken) external view returns (address _remoteToken); |
There was a problem hiding this comment.
Same question here with the returned arg
Suggested change
| function deployments(address _localToken) external view returns (address _remoteToken); | |
| function deployments(address _localToken) external view returns (address remoteToken_); |
Member
Author
There was a problem hiding this comment.
Newer contracts like L2ToL2CrossDomainMessenger have it like that, so not fixing for now unless it is asked for
| return _createWithCreate3(_remoteToken, _name, _symbol, _decimals); | ||
| } | ||
|
|
||
| function _createWithCreate3( |
There was a problem hiding this comment.
Missing NATSPEC here? Maybe a small one regarding the purpose of the virtual function
Member
Author
There was a problem hiding this comment.
Good catch, will add them
| /// @custom:storage-location erc7201:optimismMintableERC20FactoryInterop.storage | ||
| struct OptimismMintableERC20FactoryInteropStorage { | ||
| /// @notice Mapping of local token address to remote token address. | ||
| mapping(address => address) deployments; |
There was a problem hiding this comment.
I've seen that you implemented it only here, but do we also need it on the OptimismMintableERC20Factory file?
There was a problem hiding this comment.
As per this spec, yes, but not completely sure if something has changed
https://github.com/ethereum-optimism/specs/pull/294/files#diff-93042f57f61e17a3b53f6177cce570383fd0e91501a9b0665608dec89c962519R148
Member
Author
There was a problem hiding this comment.
The thing is that this OptimismMintableERC20FactoryInterop is the new implementation for the factory predeploy so it will have it anyway. The proxy will point to this new contract that inherits the old one.
| /// @param _symbol ERC20 symbol. | ||
| /// @param _decimals ERC20 decimals. | ||
| /// @return _localToken Address of the newly created token. | ||
| function _createWithCreate3( |
There was a problem hiding this comment.
Why do we need to create a public/internal set of function with the exact same NATSPEC? Can't we just leave the logic on the public one?
Member
Author
There was a problem hiding this comment.
Hmm let me think about it, maybe we can override a public one and will be enough
Base automatically changed from
fix/add-generic-factory-interface
to
sc/liquidity-migration
August 9, 2024 17:04
0xParti
approved these changes
Aug 12, 2024
0xParti
left a comment
There was a problem hiding this comment.
Looks good. We are still waiting for the feedback from their side, as they might prefer to only modify the content of the createOptimismMintableERC20WithDecimals function instead.
0xParti
reviewed
Aug 13, 2024
| /// @param _symbol ERC20 symbol. | ||
| /// @param _decimals ERC20 decimals | ||
| /// @return Address of the newly created token. | ||
| /// @return _localToken Address of the newly created token. |
There was a problem hiding this comment.
not sure if there is a convention, but they dont use underscore for localToken in their current version.
| ) | ||
| public | ||
| returns (address) | ||
| returns (address _localToken) |
There was a problem hiding this comment.
They use returns (address) instead of declaring the variable here
Member
Author
There was a problem hiding this comment.
Ok we can rollback it to make the less changes possible
0xParti
approved these changes
Aug 13, 2024
agusduha
added a commit
that referenced
this pull request
Aug 22, 2024
* feat: add L2 standrad bridge interop contract * test: add L2 standard bridge interop unit tests (#13) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: unit tests fixes * fix: super to legacy tests failing * fix: mock and expect mint and burn * fix: add generic factory interface (#14) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert (#17) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable * fix: PR fixes * test: fix address assuming * test: fix view warning * fix: snapshots * test: small fixes
0xng
pushed a commit
that referenced
this pull request
Sep 10, 2024
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable
0xng
pushed a commit
that referenced
this pull request
Sep 11, 2024
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable
0xng
pushed a commit
that referenced
this pull request
Sep 11, 2024
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable
0xng
pushed a commit
that referenced
this pull request
Sep 12, 2024
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable
0xng
added a commit
that referenced
this pull request
Sep 12, 2024
* test: add L2 standard bridge interop unit tests (#13) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: unit tests fixes * fix: super to legacy tests failing * fix: mock and expect mint and burn * fix: add generic factory interface (#14) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert (#17) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable * fix: PR fixes * feat: add superchain erc20 factory implementation (#23) * feat: add superchain erc20 factory implementation * fix: remove createX comments * test: add superchain erc20 factory tests (#25) * test: add superchain erc20 factory tests * test: add erc20 asserts * test: fix expect emit * fix: remove comments * feat: add constructor to superchain ERC20 beacon (#34) * test: remove factory predeploy etch ---------- Co-authored-by: 0xng <ng@defi.sucks> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> * fix: set an arbitrary address for superchain erc20 impl * fix: deploy a proxy for the beacon on genesis (#45) --------- Co-authored-by: 0xng <ng@defi.sucks> * fix: conflicts and imports * fix: interfaces * chore: add .testdata * fix: adding back .testdata to gitignore * fix: new conflicts from ci improvements --------- Co-authored-by: 0xng <ng@defi.sucks> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
0xiamflux
pushed a commit
that referenced
this pull request
Jan 27, 2026
feat: extract optimism genesis info
0xOneTony
pushed a commit
that referenced
this pull request
Mar 6, 2026
…thereum-optimism#19272) * contracts: implement audit code fixes and add tests Add onlyDelegateCall enforcement to upgradeSuperchain, upgrade, and migrate functions (#17). Include msg.sender in deploy salt to prevent cross-caller CREATE2 collisions (#17). Add duplicate instruction key detection in upgrade validation (#9). Validate startingRespectedGameType against enabled game configs (#10). Add code-existence check in loadBytes (#18). Add setUp guard to VerifyOPCM.runSingle (#4). Remove unused _findChar function (#5). Pass real AddressManager in migrator proxy deploy args (#11). Add tests covering all audit fix behaviors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * contracts: regenerate semver-lock.json for OPContractsManagerV2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * contracts: bump OPContractsManagerV2 version to 7.0.10 Semver-diff requires a patch version bump when bytecode changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
3 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.
Closes OPT-185
Closes OPT-186