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.

Sign up for GitHub

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

Jump to bottom

contracts-bedrock: foundry-ify #6470

Merged
merged 9 commits into from
Jul 27, 2023
Merged

contracts-bedrock: foundry-ify #6470

merged 9 commits into from
Jul 27, 2023

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Jul 27, 2023 •
edited
Loading

Description

Move the contracts directory to src and then move the tests into a top level test dir so that its
standard foundry style for a repo.

This is going to break any in flight PRs so worth trying to get in relatively quickly, will be no fun to rebase a bunch of PRs.

No semver bumps required because no production source code is changing

@tynes
contracts-bedrock: foundry-ify
f54a223 
Move the `contracts` directory to `src` and then move
the tests into a top level `test` dir so that its
standard foundry style for a repo.

This is going to break any in flight PRs so worth trying
to get in relatively quickly, will be no fun to rebase a
bunch of PRs.

Will need to follow up with another commit that bumps the
patch versions.
@tynes tynes requested a review from a team as a code owner July 27, 2023 00:37
@tynes tynes requested a review from clabby July 27, 2023 00:37
@changeset-bot
Copy link

changeset-bot bot commented Jul 27, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 550f5ad

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 the C-protocol-critical Category: Modifies protocol-critical code label Jul 27, 2023
@mergify mergify bot requested a review from smartcontracts July 27, 2023 00:38
@tynes tynes requested review from maurelian and removed request for smartcontracts July 27, 2023 00:39
@mergify mergify bot requested a review from smartcontracts July 27, 2023 00:39
@tynes tynes requested a review from a team as a code owner July 27, 2023 00:40
@tynes tynes requested a review from refcell July 27, 2023 00:40
refcell
refcell reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

King, looks good to me, but will await other's review before approving

@tynes
Copy link
Contributor Author

tynes commented Jul 27, 2023

@maurelian asked for this this morning

@tynes tynes removed the request for review from smartcontracts July 27, 2023 00:54
@tynes
Copy link
Contributor Author

tynes commented Jul 27, 2023

The last thing is the artifacts output path but not changing that as part of this PR as it will require a diff in typescript code

@mergify mergify bot requested a review from smartcontracts July 27, 2023 00:55
maurelian
maurelian approved these changes Jul 27, 2023
View reviewed changes
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.

No bindings changed, so I checked the diff in the storage layout, everything LGTM.
Thanks for this!

@codecov
Copy link

codecov bot commented Jul 27, 2023 •
edited
Loading

Codecov Report

Merging #6470 (550f5ad) into develop (7b97a44) will decrease coverage by 0.03%.
Report is 16 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6470      +/-   ##
===========================================
- Coverage    40.74%   40.71%   -0.03%     
===========================================
  Files          466      359     -107     
  Lines        33509    31360    -2149     
  Branches      1949     1538     -411     
===========================================
- Hits         13652    12769     -883     
+ Misses       18379    17154    -1225     
+ Partials      1478     1437      -41
Flag Coverage Δ
bedrock-go-tests 43.97% <ø> (-0.03%) ⬇️
cannon-go-tests 62.40% <ø> (ø)
chain-mon-tests 26.95% <ø> (ø)
common-ts-tests ?
contracts-bedrock-tests ∅ <ø> (∅)
contracts-ts-tests 6.88% <ø> (ø)
core-utils-tests ?
sdk-next-tests 42.60% <ø> (ø)
sdk-tests 42.60% <ø> (ø)

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

see 108 files with indirect coverage changes

@tynes tynes merged commit e8b50b3 into develop Jul 27, 2023
6 checks passed
@tynes tynes deleted the feat/foundry-ize-package branch July 27, 2023 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refcell refcell refcell left review comments

@maurelian maurelian maurelian approved these changes

@clabby clabby Awaiting requested review from clabby clabby is a code owner automatically assigned from ethereum-optimism/contract-reviewers

@smartcontracts smartcontracts Awaiting requested review from smartcontracts

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.
3 participants
@tynes @refcell @maurelian