feat: liquidity migration by agusduha · Pull Request #10 · defi-wonderland/optimism

agusduha · 2024-08-05T14:33:25Z

Closes OPT-175
Closes OPT-174
Closes OPT-177

tynes · 2024-08-06T04:07:38Z

+interface IOptimismMintableERC20Factory {
+    function deployments(address) external view returns (address);
+}
+


We could use an interface that is generic with just the deployments function. We currently do this sometimes, it reduces the number of files when verifying a contract and i think it will reduce compile time if we use interfaces everywhere rather than importing full implementations

tynes · 2024-08-06T04:08:23Z

This is looking great so far!

linear · 2024-08-06T15:05:54Z

OPT-175 Convert: Implement convert() function in the L2StandardBridge

0xDiscotech · 2024-08-06T15:43:21Z

The contract implementation looks good to me as well!

* 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

* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface

* 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

linear · 2024-08-13T13:41:07Z

OPT-174 Implement Convert

OPT-177 Modify OptimismMintableERC20Factory

0xDiscotech

This way of handling the new semver is missing: https://github.com/ethereum-optimism/design-docs/blob/main/smart-contract-feature-development.md#handling-semver on L2StandardBridgeInterop.
Also, I think you need to sum a minor patch on the L2StandardBridge contract since you are adding functions and logic to it with the interop extension.

0xDiscotech · 2024-08-13T16:49:03Z

+error InvalidTokenPair();
+
+// TODO: Use an existing interface with `mint` and `burn`?
+interface MintableAndBurnable is IERC20 {


WDYT about this?

Suggested change

interface MintableAndBurnable is IERC20 {

interface Mintable {

0xParti · 2024-08-13T14:36:35Z

+        // 3. Valid SuperchainERC20 check
+        address _superRemoteToken =
+            IOptimismERC20Factory(Predeploys.OPTIMISM_SUPERCHAIN_ERC20_FACTORY).deployments(_superAddr);
+        if (_superRemoteToken == address(0)) revert InvalidSuperchainAddress();


Not convinced by the error name. It's understandable in context, but for a first time reader it might be confusing. Maybe InvalidSuperchainERC20Address, even if it sounds ugly. Wdty? I know it also affects the Legacy error, so maybe nay

0xParti · 2024-08-13T14:40:35Z

+/// @notice The L2StandardBridgeInterop is an extension of the L2StandardBridge that allows for
+///         the conversion of tokens between legacy tokens (OptimismMintableERC20 or StandardL2ERC20)
+///         and SuperchainERC20 tokens.
+contract L2StandardBridgeInterop is L2StandardBridge {


Why are extending the standard bridge instead of updating it? did you have this conversation with mark?

It is how the interop contracts are being develop, here is a design guide https://github.com/ethereum-optimism/design-docs/blob/main/smart-contract-feature-development.md#composition-through-inheritance

0xParti · 2024-08-13T16:59:44Z

+        // 3. Valid SuperchainERC20 check
+        address _superRemoteToken =
+            IOptimismERC20Factory(Predeploys.OPTIMISM_SUPERCHAIN_ERC20_FACTORY).deployments(_superAddr);
+        if (_superRemoteToken == address(0)) revert InvalidSuperchainAddress();


Is this check necessary? you are already checking that _legacy !=0 and that _legacy == _super

I think is just for the revert error. I'd leave it, doesn't harm since we'll be using an OP chain so gas is not an issue

Yeah, its also a very cheap check. Let's leave it for clarity :)

0xParti

lesgooo

0xDiscotech · 2024-08-14T00:20:11Z

Amazing job sir!

agusduha · 2024-08-14T12:43:05Z

External review at ethereum-optimism#11479

agusduha · 2024-08-22T12:29:06Z

Merged at ethereum-optimism#11479

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

…edStaker Cantina audit finding #10: when a beneficiary removes a staker from their allowlist, the staker's lastUpdate is reset via _increasePeData, losing accumulated staking weight. Document this as an inherent trust assumption of the delegation model. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs(staking): document pause bypass behavior in setAllowedStaker * fix(staking): only reset lastUpdate on full unstake in _decreasePeData Cantina audit finding #7: _decreasePeData unconditionally reset lastUpdate on every decrease, penalizing partial unstakers by resetting their staking weight. Now lastUpdate is only reset when effectiveStake reaches zero. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(staking): implement two-step ownership transfer Cantina audit finding #8: transferOwnership now nominates a pending owner who must call acceptOwnership() to finalize the transfer, preventing irrevocable ownership loss from incorrect addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(staking): document lastUpdate reset trust assumption in setAllowedStaker Cantina audit finding #10: when a beneficiary removes a staker from their allowlist, the staker's lastUpdate is reset via _increasePeData, losing accumulated staking weight. Document this as an inherent trust assumption of the delegation model. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: undo version change * chore: upgrade natspec * chore: update event emission order * fix: wrong event args * chore: add interface comments * fix: pre-pr * refactor: make ownable private functions public * fix: pre-pr * feat(ownable): reset pending owner on zeroaddress ownership transfer * refactor: inherit OZ's ownable and use helper contract for mapping storage slot * fix: ci --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

feat: add L2 standrad bridge interop contract

64dc6d4

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

tynes reviewed Aug 6, 2024

View reviewed changes

agusduha self-assigned this Aug 6, 2024

agusduha mentioned this pull request Aug 7, 2024

fix: add generic factory interface #14

Merged

agusduha added 3 commits August 8, 2024 17:23

test: add L2 standard bridge interop unit tests (#13)

de677a2

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

593efad

* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface

Merge branch 'develop' into sc/liquidity-migration

b8bd100

agusduha requested a review from 0xParti August 13, 2024 14:02

0xDiscotech reviewed Aug 13, 2024

View reviewed changes

0xParti reviewed Aug 13, 2024

View reviewed changes

fix: PR fixes

809a71b

0xParti approved these changes Aug 13, 2024

View reviewed changes

0xDiscotech approved these changes Aug 14, 2024

View reviewed changes

agusduha added the hold Do not merge for now label Aug 14, 2024

agusduha added 7 commits August 14, 2024 12:16

Merge branch 'develop' into sc/liquidity-migration

f4d0bb3

test: fix address assuming

2a3a723

test: fix view warning

8b74736

Merge branch 'develop' into sc/liquidity-migration

e51f42b

fix: snapshots

4305edc

Merge branch 'develop' into sc/liquidity-migration

06d662d

Merge branch 'develop' into sc/liquidity-migration

1ef52a7

test: small fixes

9090320

agusduha closed this Aug 22, 2024

agusduha deleted the sc/liquidity-migration branch August 22, 2024 12:29

0xChin mentioned this pull request Mar 9, 2026

fix: staking audit fixes #892

Closed

	interface MintableAndBurnable is IERC20 {
	interface Mintable {

Conversation

agusduha commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tynes Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

tynes commented Aug 6, 2024

Uh oh!

linear Bot commented Aug 6, 2024

Uh oh!

0xDiscotech commented Aug 6, 2024

Uh oh!

linear Bot commented Aug 13, 2024

Uh oh!

0xDiscotech left a comment

Choose a reason for hiding this comment

Uh oh!

0xDiscotech Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xParti Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

0xParti Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

agusduha Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xParti Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

0xDiscotech Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

0xParti Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

0xParti left a comment

Choose a reason for hiding this comment

Uh oh!

0xDiscotech commented Aug 14, 2024

Uh oh!

agusduha commented Aug 14, 2024

Uh oh!

agusduha commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agusduha commented Aug 5, 2024 •

edited

Loading

0xDiscotech Aug 13, 2024 •

edited

Loading