Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ctb): type definition in ERC721Bridge #10755

Merged
merged 7 commits into from
Jun 8, 2024

Conversation

0xfuturistic
Copy link
Contributor

@0xfuturistic 0xfuturistic commented Jun 6, 2024

context copied from https://github.com/ethereum-optimism/client-pod/issues/601

Found by @tynes and discussed in slack.

See this line, the type is StandardBridge but it shouldn't be StandardBridge because the erc721 bridge never interacts with the erc20 bridge, it should be ERC721Bridge.

This does not impact functionality, and just affects integrators calling the otherBridge getter. Currently they must either cast the returned value or use their own interface which returns the correct value

@0xfuturistic 0xfuturistic requested review from tynes and mds1 June 6, 2024 01:46
@0xfuturistic 0xfuturistic self-assigned this Jun 6, 2024
@0xfuturistic 0xfuturistic requested a review from a team as a code owner June 6, 2024 01:46
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.26%. Comparing base (aed9ee1) to head (12178cb).
Report is 8 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #10755       +/-   ##
============================================
- Coverage    54.65%   39.26%   -15.39%     
============================================
  Files           37       27       -10     
  Lines         2944     1821     -1123     
  Branches       415      415               
============================================
- Hits          1609      715      -894     
+ Misses        1303     1106      -197     
+ Partials        32        0       -32     
Flag Coverage Δ
cannon-go-tests ?
chain-mon-tests 27.14% <ø> (ø)
sdk-tests 40.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 10 files with indirect coverage changes

@tynes
Copy link
Contributor

tynes commented Jun 7, 2024

This is great and we can merge it if we bump the semver for the L1 and L2 bridges with a patch + beta, so X.Y.Z-beta.1 where Z increments

Copy link
Contributor

semgrep-app bot commented Jun 7, 2024

Semgrep found 1 sol-style-return-arg-fmt finding:

  • packages/contracts-bedrock/src/cannon/MIPS.sol

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

Semgrep found 3 sol-style-malformed-revert findings:

Malformed revert statement style.

Ignore this finding from sol-style-malformed-revert.

Semgrep found 1 sol-style-input-arg-fmt finding:

  • packages/contracts-bedrock/src/cannon/MIPS.sol

Inputs to functions must be prepended with an underscore (_)

Ignore this finding from sol-style-input-arg-fmt.

Semgrep found 2 invalid-usage-of-modified-variable findings:

Variable unsafeInNode is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Ignore this finding from invalid-usage-of-modified-variable.

@0xfuturistic 0xfuturistic enabled auto-merge June 7, 2024 22:53
@0xfuturistic 0xfuturistic added this pull request to the merge queue Jun 7, 2024
Merged via the queue into develop with commit 522c681 Jun 8, 2024
67 checks passed
@0xfuturistic 0xfuturistic deleted the diego/fix-ERC721-otherBridge-type branch June 8, 2024 00:04
rdovgan pushed a commit to rdovgan/optimism that referenced this pull request Jun 24, 2024
* contracts-bedrock: change type of otherBridge in ERC721Bridge to ERC721Bridge

* contracts-bedrock: update semver-lock

* contracts-bedrock: update snapshots

* contracts-bedrock: bump version for L2ERC721Bridge

* contracts-bedrock: bump version for L1ERC721Bridge

* contracts-bedrock: update semver-lock

* contracts-bedrock: update snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants