Skip to content

Comments

nft: N-11 Unused inherited contract#3459

Closed
sam-goldman wants to merge 3 commits intoethereum-optimism:developfrom
Fanbase-Labs:ctp/unused-ownable-import
Closed

nft: N-11 Unused inherited contract#3459
sam-goldman wants to merge 3 commits intoethereum-optimism:developfrom
Fanbase-Labs:ctp/unused-ownable-import

Conversation

@sam-goldman
Copy link
Contributor

Removes unused OwnableUpgradeable import in NFT bridge contracts. Also removes this import in OptimismMintableERC721Factory, which is not mentioned in this finding, but the import is unused nonetheless.

@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2022

⚠️ No Changeset found

Latest commit: 57f8c45

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added 2-reviewers A-op-bindings Area: op-bindings labels Sep 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 13, 2022

Hey @sam-goldman! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Sep 13, 2022
@mergify mergify bot removed the conflict label Sep 13, 2022
@@ -89,9 +87,6 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable {
function initialize(address _messenger, address _otherBridge) public initializer {
messenger = _messenger;
otherBridge = _otherBridge;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cheaper and more consistent with the direction of our StandardBridge to make this an immutable variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to messenger, otherBridge, or both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this in this PR, where it seemed more in scope.

@@ -52,9 +50,6 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable {
function initialize(address _bridge, uint256 _remoteChainId) public initializer {
bridge = _bridge;
remoteChainId = _remoteChainId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, what's this used for?

Copy link
Contributor Author

@sam-goldman sam-goldman Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We pass it in as an argument when we create an OptimismMintableERC721 contract here. It's used in the OptimismMintableERC721 here to create the EIP-681 URI, which is used to query the L1 token URI.

Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Generally LGTM once CI is working.

@sam-goldman
Copy link
Contributor Author

Closed by #3489.

This was referenced Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-op-bindings Area: op-bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants