Skip to content

fix(CI): Fail CI if contracts-bedrock tests fail#5475

Merged
OptimismBot merged 5 commits intodevelopfrom
clabby/ci/fix-ctb-tests
Apr 18, 2023
Merged

fix(CI): Fail CI if contracts-bedrock tests fail#5475
OptimismBot merged 5 commits intodevelopfrom
clabby/ci/fix-ctb-tests

Conversation

@clabby
Copy link
Contributor

@clabby clabby commented Apr 17, 2023

Overview

Adds a new task to CI, contracts-bedrock-tests, which runs forge test rather than forge coverage. forge coverage will not return an error exit code if any tests that it runs fail.

@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2023

⚠️ No Changeset found

Latest commit: 2760ae3

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

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for opstack-docs ready!

Name Link
🔨 Latest commit 2760ae3
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/643eccc99b6c7a0008ca2988
😎 Deploy Preview https://deploy-preview-5475--opstack-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@clabby
Copy link
Contributor Author

clabby commented Apr 17, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@clabby clabby force-pushed the clabby/ci/fix-ctb-tests branch 2 times, most recently from 06d244d to f8a747b Compare April 18, 2023 01:58
@clabby clabby force-pushed the clabby/ci/fix-ctb-tests branch from f8a747b to a526d8a Compare April 18, 2023 02:11
@clabby clabby marked this pull request as ready for review April 18, 2023 03:07
@clabby clabby requested review from a team as code owners April 18, 2023 03:07
@clabby clabby requested review from mslipper and tynes April 18, 2023 03:07
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 a nit, but overall LGTM.

@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2023

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

@OptimismBot OptimismBot merged commit 1ce4fdf into develop Apr 18, 2023
@OptimismBot OptimismBot deleted the clabby/ci/fix-ctb-tests branch April 18, 2023 17:15
@mergify mergify bot removed the on-merge-train label Apr 18, 2023
seolaoh added a commit to kroma-network/kroma that referenced this pull request Aug 9, 2023
`CrossDomainMessager` whereby `relayMessage` naively reset the
`xDomainMsgSender` after a call. The issue here was that a nested call would
reset this value to the `Constants.DEFAULT_L2_SENDER` which deviates from the
expected behavior that the `xDomainMsgSender` is set to the `_sender` for the
entirety of the subcall.
This change sets a message as failed upon re-entry to the `relayMessage`
function by checking the value of `xDomainMsgSender`. We then also remove the
`reentrancyLocks` from storage and don't have to worry about re-entrency here
since any subcall through the `SafeCall` lib to the XDM `relayMessage` will be
caught and recorded as failed by the aforementioned `xDomainMsgSender` check.
Also removes the revert in `relayMessage` when the minimum gas threshold is not
met.

See:
  - ethereum-optimism/optimism#5475
  - ethereum-optimism/optimism#5444
  - ethereum-optimism/optimism#5493
  - ethereum-optimism/optimism#5508
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.

5 participants