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

refactor: use submodules for contracts #1396

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

zimpha
Copy link
Member

@zimpha zimpha commented Jun 24, 2024

Purpose or design rationale of this PR

This PR remove contracts directory and add scroll-contracts repo ad submodule.

Q: Why not reuse contracts directory for the submodule?
A: It contains submodules before, changing it to another submodule would lead some problems in git.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • No, this PR doesn't involve a new deployment, git tag, docker image tag
  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

@georgehao
Copy link
Member

georgehao commented Jun 24, 2024

How about don't put submoudle to monorepo, also remove l2geth submodule @Thegaram @HAOYUatHZ

@Thegaram
Copy link
Contributor

How about don't put submoudle to monorepo, also remove l2geth submodule @Thegaram @HAOYUatHZ

Sounds good to me. I'm fine with removing it. But as I recall a few integration tests are using it. (Maybe it's fine to remove those.)

@zimpha
Copy link
Member Author

zimpha commented Jun 25, 2024

How about don't put submoudle to monorepo, also remove l2geth submodule @Thegaram @HAOYUatHZ

Sounds good to me. I'm fine with removing it. But as I recall a few integration tests are using it. (Maybe it's fine to remove those.)

agreed.

@0xmountaintop
Copy link
Member

How about don't put submoudle to monorepo, also remove l2geth submodule @Thegaram @HAOYUatHZ

ok

@zimpha zimpha closed this Jun 25, 2024
@zimpha zimpha deleted the feat/use_submodules_for_contracts branch June 25, 2024 01:53
@colinlyguo colinlyguo restored the feat/use_submodules_for_contracts branch July 1, 2024 07:34
@colinlyguo
Copy link
Member

colinlyguo commented Jul 1, 2024

Reopen this PR, we will still use codec implementations to check the workflow (mainly hash calculation) in infra components.

These hash checks are minimum requirements for merging new features, it can at least align contract and infra components to some extent.

@colinlyguo colinlyguo reopened this Jul 1, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.31%. Comparing base (16673e2) to head (4fec94b).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1396   +/-   ##
========================================
  Coverage    54.31%   54.31%           
========================================
  Files          156      156           
  Lines        11918    11918           
========================================
  Hits          6473     6473           
  Misses        4950     4950           
  Partials       495      495           
Flag Coverage Δ
rollup 59.83% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@colinlyguo colinlyguo merged commit ab9c541 into develop Jul 2, 2024
5 checks passed
@colinlyguo colinlyguo deleted the feat/use_submodules_for_contracts branch July 2, 2024 08:34
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.

6 participants