feat: modify OptimismMintableERC20Factory for convert #17

agusduha · 2024-08-08T14:33:26Z

Closes OPT-185
Closes OPT-186

linear · 2024-08-08T14:33:28Z

OPT-185 OptimismMintableERC20Factory: Implement Changes

0xDiscotech · 2024-08-08T15:51:03Z

packages/contracts-bedrock/src/L2/IOptimismERC20Factory.sol

    /// @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);


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_);

Newer contracts like L2ToL2CrossDomainMessenger have it like that, so not fixing for now unless it is asked for

0xDiscotech · 2024-08-08T15:52:49Z

packages/contracts-bedrock/src/universal/OptimismMintableERC20Factory.sol

    }
+
+    function _createWithCreate3(


Missing NATSPEC here? Maybe a small one regarding the purpose of the virtual function

Good catch, will add them

0xDiscotech · 2024-08-08T16:06:34Z

packages/contracts-bedrock/src/universal/OptimismMintableERC20FactoryInterop.sol

+    /// @custom:storage-location erc7201:optimismMintableERC20FactoryInterop.storage
+    struct OptimismMintableERC20FactoryInteropStorage {
+        /// @notice Mapping of local token address to remote token address.
+        mapping(address => address) deployments;


I've seen that you implemented it only here, but do we also need it on the OptimismMintableERC20Factory file?

As per this spec, yes, but not completely sure if something has changed
https://github.com/ethereum-optimism/specs/pull/294/files#diff-93042f57f61e17a3b53f6177cce570383fd0e91501a9b0665608dec89c962519R148

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.

ahh perfect then!

0xDiscotech · 2024-08-08T16:08:00Z

packages/contracts-bedrock/src/universal/OptimismMintableERC20FactoryInterop.sol

+    /// @param _symbol      ERC20 symbol.
+    /// @param _decimals    ERC20 decimals.
+    /// @return _localToken Address of the newly created token.
+    function _createWithCreate3(


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?

Hmm let me think about it, maybe we can override a public one and will be enough

yeah I think so

…-factory

0xParticle

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.

0xParticle · 2024-08-13T13:17:31Z

packages/contracts-bedrock/src/universal/OptimismMintableERC20Factory.sol

@@ -104,29 +109,29 @@ contract OptimismMintableERC20Factory is ISemver, Initializable {
    /// @param _name        ERC20 name.
    /// @param _symbol      ERC20 symbol.
    /// @param _decimals    ERC20 decimals
-    /// @return Address of the newly created token.
+    /// @return _localToken Address of the newly created token.


not sure if there is a convention, but they dont use underscore for localToken in their current version.

0xParticle · 2024-08-13T13:17:50Z

packages/contracts-bedrock/src/universal/OptimismMintableERC20Factory.sol

    function createOptimismMintableERC20WithDecimals(
        address _remoteToken,
        string memory _name,
        string memory _symbol,
        uint8 _decimals
    )
        public
-        returns (address)
+        returns (address _localToken)


They use returns (address) instead of declaring the variable here

Ok we can rollback it to make the less changes possible

* 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

* 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

* 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>

agusduha added 4 commits August 6, 2024 17:41

test: add L2 standard bridge interop unit tests

5542ee9

fix: add tests natspec

ef1785f

fix: add generic factory interface

a6b9fcb

feat: modify OptimismMintableERC20Factory for convert

dcae6b0

agusduha requested review from 0xng and 0xDiscotech August 8, 2024 14:33

agusduha self-assigned this Aug 8, 2024

0xDiscotech reviewed Aug 8, 2024

View reviewed changes

fix: use only a public function for create3

ce50ea8

Base automatically changed from fix/add-generic-factory-interface to sc/liquidity-migration August 9, 2024 17:04

Merge branch 'sc/liquidity-migration' into feat/modify-mintable-erc20…

90d1e7d

…-factory

0xParticle approved these changes Aug 12, 2024

View reviewed changes

feat: rollback interop factory, modify legacy one

8209d4b

0xParticle reviewed Aug 13, 2024

View reviewed changes

fix: delete local token return variable

f8e736e

0xParticle approved these changes Aug 13, 2024

View reviewed changes

agusduha merged commit 014c25d into sc/liquidity-migration Aug 13, 2024
3 checks passed

agusduha deleted the feat/modify-mintable-erc20-factory branch August 13, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modify OptimismMintableERC20Factory for convert #17

feat: modify OptimismMintableERC20Factory for convert #17

agusduha commented Aug 8, 2024 •

edited

Loading

linear bot commented Aug 8, 2024

0xDiscotech Aug 8, 2024

agusduha Aug 8, 2024

0xDiscotech Aug 8, 2024

agusduha Aug 8, 2024

0xDiscotech Aug 8, 2024

0xDiscotech Aug 8, 2024

agusduha Aug 8, 2024

0xDiscotech Aug 8, 2024

0xDiscotech Aug 8, 2024

agusduha Aug 8, 2024

0xDiscotech Aug 8, 2024

0xParticle left a comment

0xParticle Aug 13, 2024

0xParticle Aug 13, 2024

agusduha Aug 13, 2024

	function deployments(address _localToken) external view returns (address _remoteToken);
	function deployments(address _localToken) external view returns (address remoteToken_);

feat: modify OptimismMintableERC20Factory for convert #17

feat: modify OptimismMintableERC20Factory for convert #17

Conversation

agusduha commented Aug 8, 2024 • edited Loading

linear bot commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xParticle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agusduha commented Aug 8, 2024 •

edited

Loading