Skip to content

Fix typo#2808

Closed
Vsevo1od wants to merge 1 commit intoethereum-optimism:masterfrom
Vsevo1od:patch-1
Closed

Fix typo#2808
Vsevo1od wants to merge 1 commit intoethereum-optimism:masterfrom
Vsevo1od:patch-1

Conversation

@Vsevo1od
Copy link
Copy Markdown

@Vsevo1od Vsevo1od commented Jun 17, 2022

Description
safeTransferFrom checks if the token contract has code, not a from address. So this looks like a typo.

Additional context

Metadata

@Vsevo1od
Fix typo
5997e75 
safeTransferFrom checks if the *token* contract has code, not a *from* address
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jun 17, 2022

⚠️ No Changeset found

Latest commit: 5997e75

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 C-protocol-critical Category: Modifies protocol-critical code labels Jun 17, 2022
@mergify mergify bot requested a review from smartcontracts June 17, 2022 05:25
maurelian
maurelian reviewed Jun 21, 2022
View reviewed changes
packages/contracts/contracts/L1/messaging/L1StandardBridge.sol
// When a deposit is initiated on L1, the L1 Bridge transfers the funds to itself for future
// withdrawals. safeTransferFrom also checks if the contract has code, so this will fail if
// _from is an EOA or address(0).
// _l1Token is an EOA or address(0).
Copy link
Copy Markdown
Contributor

@maurelian maurelian Jun 21, 2022

Choose a reason for hiding this comment

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

I'm power sure that safeTransferFrom dies this check on the recipient. In this case that's this contract, so I'm not sure why used it.

Copy link
Copy Markdown
Contributor

@maurelian maurelian Jun 21, 2022

Choose a reason for hiding this comment

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

Actually no, it handles broken tokens: https://docs.openzeppelin.com/contracts/2.x/api/token/ERC20

@maurelian maurelian mentioned this pull request Jun 22, 2022
Merged
@maurelian
Copy link
Copy Markdown
Contributor

maurelian commented Jun 22, 2022

Thanks for this @Vsevo1od! Though I'm closing as the fix is not quite right, and I have a replacement in #2831.

For ref, see the impl of safeTransferFrom here.

@mslipper mslipper closed this Jun 22, 2022
@Vsevo1od
Copy link
Copy Markdown
Author

Vsevo1od commented Jun 22, 2022

@maurelian thank you!
I think what the comment may meant is that inside _callOptionalReturn (which is called by safeTransferFrom) is a .functionCall

Which checks if the called address has code

        require(isContract(target), "Address: call to non-contract");

And the called address is _l1token in this case.

But your fix is fine with me.
I just got a little confused while reading the code and decided to share :)

@maurelian
Copy link
Copy Markdown
Contributor

maurelian commented Jun 22, 2022

Thanks, and definitely appreciate the help! :)

theochap added a commit that referenced this pull request Dec 10, 2025
@theochap
fix(node/tests): fix kurtosis ci (#2808)
81734d7 
## Description

This PR fixes kurtosis jobs in CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maurelian maurelian maurelian left review comments

@tynes tynes tynes approved these changes

@smartcontracts smartcontracts Awaiting requested review from smartcontracts

+1 more reviewer

@mslipper mslipper mslipper approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C-protocol-critical Category: Modifies protocol-critical code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Vsevo1od @maurelian @mslipper @tynes