contracts-bedrock: fix `OptimismMintableERC20Created` by tynes · Pull Request #3104 · ethereum-optimism/optimism

tynes · 2022-07-27T03:43:01Z

Description

The names of the event args were backwards from what they
were supposed to be. I noticed this while writing integration
tests for the sdk.

The event is used like this:

emit OptimismMintableERC20Created(_remoteToken, address(localToken), msg.sender);

The event was updated to look like this:

event OptimismMintableERC20Created(
    address indexed remoteToken,
    address indexed localToken,
    address deployer
);

The remoteToken is first and the localToken is second.

changeset-bot · 2022-07-27T03:43:04Z

🦋 Changeset detected

Latest commit: 761f90f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@eth-optimism/contracts-bedrock	Patch
@eth-optimism/actor-tests	Patch
@eth-optimism/sdk	Patch
@eth-optimism/drippie-mon	Patch
@eth-optimism/message-relayer	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

maurelian · 2022-07-27T14:42:27Z

Good catch. I think this bug is a good argument for the changed proposed in #3064.

This lgtm, but please see my message in slack about a proposal for how to handle all impl changes during the OZ audit.

maurelian · 2022-07-28T16:01:13Z

Can we move this here so that it can get reviewed along with audit fixes?

maurelian · 2022-07-29T16:35:08Z

I moved your commit to here, so I think we can close this for now.

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

github-actions · 2022-08-16T02:15:39Z

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

maurelian · 2022-09-16T13:40:53Z

@tynes I think this got stale because it was moved into the fixes repo temporarily. I'm reopening it now, and will push a rebase shortly.

mergify · 2022-09-19T05:34:22Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

maurelian · 2022-09-21T01:02:34Z

Need to do a bit of debugging to figure out why this broke the deposit-erc20 integration test

mergify · 2022-09-21T01:03:36Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

tynes · 2022-09-21T19:37:35Z

See this comment for fixing the tests:

optimism/packages/sdk/tasks/deposit-erc20.ts

Lines 78 to 80 in 4a5e183

    
           // TODO(tynes): may need to be updated based on 
        
           // https://github.com/ethereum-optimism/optimism/pull/3104 
        
           const l2WethAddress = event.args.remoteToken

The names of the event args were backwards from what they were supposed to be. I noticed this while writing integration tests for the sdk. The event is used like this: ```solidity emit OptimismMintableERC20Created(_remoteToken, address(localToken), msg.sender); ``` The event was updated to look like this: ```solidity event OptimismMintableERC20Created( address indexed remoteToken, address indexed localToken, address deployer ); ``` The `remoteToken` is first and the `localToken` is second.

mergify · 2022-09-21T21:09:36Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

mergify · 2022-09-21T22:16:28Z

This PR has been added to the merge queue, and will be merged soon.

mergify · 2022-09-21T22:16:36Z

This PR is next in line to be merged, and will be merged as soon as checks pass.

* contracts-bedrock: fix `OptimismMintableERC20Created` The names of the event args were backwards from what they were supposed to be. I noticed this while writing integration tests for the sdk. The event is used like this: ```solidity emit OptimismMintableERC20Created(_remoteToken, address(localToken), msg.sender); ``` The event was updated to look like this: ```solidity event OptimismMintableERC20Created( address indexed remoteToken, address indexed localToken, address deployer ); ``` The `remoteToken` is first and the `localToken` is second. * ctb: Avoid extra casting from contract to address * chore: Regen bindings and snapshot * chore: Update bindings Co-authored-by: Maurelian <maurelian@protonmail.ch> Co-authored-by: Matthew Slipper <me@matthewslipper.com>

tynes requested a review from maurelian July 27, 2022 03:43

github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Jul 27, 2022

maurelian reviewed Aug 1, 2022

View reviewed changes

packages/contracts-bedrock/contracts/universal/OptimismMintableERC20Factory.sol Show resolved Hide resolved

github-actions bot added the Stale label Aug 16, 2022

github-actions bot closed this Aug 21, 2022

maurelian reopened this Sep 16, 2022

maurelian force-pushed the fix/ctb-event branch from 761ab09 to afb414a Compare September 16, 2022 13:52

sam-goldman mentioned this pull request Sep 16, 2022

nft: L-13 and M-08 #3491

Merged

maurelian force-pushed the fix/ctb-event branch from afb414a to 0c20a11 Compare September 16, 2022 19:27

github-actions bot added the A-pkg-sdk Area: packages/sdk label Sep 16, 2022

maurelian requested a review from smartcontracts September 16, 2022 19:37

github-actions bot removed the Stale label Sep 17, 2022

mergify bot added the conflict label Sep 19, 2022

maurelian force-pushed the fix/ctb-event branch from 0c20a11 to 8381dcd Compare September 20, 2022 23:46

mergify bot removed the conflict label Sep 20, 2022

smartcontracts approved these changes Sep 21, 2022

View reviewed changes

mergify bot added the conflict label Sep 21, 2022

maurelian force-pushed the fix/ctb-event branch from 8381dcd to 3b52c1e Compare September 21, 2022 20:31

maurelian added 2 commits September 21, 2022 16:41

ctb: Avoid extra casting from contract to address

bd41717

chore: Regen bindings and snapshot

8692b26

maurelian force-pushed the fix/ctb-event branch from 3b52c1e to 8692b26 Compare September 21, 2022 20:46

mergify bot removed the conflict label Sep 21, 2022

mergify bot added the conflict label Sep 21, 2022

mslipper added 2 commits September 21, 2022 16:01

Merge remote-tracking branch 'upstream/develop' into fix/ctb-event

6f25628

chore: Update bindings

761f90f

mslipper approved these changes Sep 21, 2022

View reviewed changes

mergify bot removed the conflict label Sep 21, 2022

mergify bot added the on-merge-train label Sep 21, 2022

mergify bot merged commit 76c8ee2 into develop Sep 21, 2022

mergify bot deleted the fix/ctb-event branch September 21, 2022 22:16

mergify bot removed the on-merge-train label Sep 21, 2022

Comments

Conversation

tynes commented Jul 27, 2022

Uh oh!

changeset-bot bot commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

maurelian commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maurelian commented Jul 28, 2022

Uh oh!

maurelian commented Jul 29, 2022

Uh oh!

Uh oh!

github-actions bot commented Aug 16, 2022

Uh oh!

maurelian commented Sep 16, 2022

Uh oh!

mergify bot commented Sep 19, 2022

Uh oh!

maurelian commented Sep 21, 2022

Uh oh!

mergify bot commented Sep 21, 2022

Uh oh!

tynes commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Sep 21, 2022

Uh oh!

mergify bot commented Sep 21, 2022

Uh oh!

mergify bot commented Sep 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

changeset-bot bot commented Jul 27, 2022 •

edited

Loading

maurelian commented Jul 27, 2022 •

edited

Loading

tynes commented Sep 21, 2022 •

edited

Loading