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

chore: move Foundry project to repo root #103

Merged
merged 3 commits into from
Nov 4, 2024
Merged

Conversation

acuarica
Copy link
Contributor

@acuarica acuarica commented Nov 2, 2024

Description:

This PR moves the Foundry project from the @hts-forking into the repo root. This will allow us to have both Hardhat and Foundry solution within the same Foundry project.

Related issue(s):

Fixes #102.

Notes for reviewer:

Important

We use a symbolic link @hts-forking/resources/HtsSystemContract.json to the compilation output of HTS. This allow us to modify the HTS contract and test the npm package without and intermediate step to copy the compilation output.
It's is important that all requires within the npm package are inside the @hts-forking folder, so when we publish it all files are contained.

Note

To commit symbolic links that are processed by prettier, e.g., json files, I had to temporarily disable prettier. Otherwise, I got the error

✖ prettier --write:
[error] Explicitly specified pattern "/<path>/@hts-forking/resources/HtsSystemContract.json" is a symbolic link.

Looks like prettier does not support symbolic links.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Luis Mastrangelo <luis@swirldslabs.com>
@acuarica acuarica linked an issue Nov 2, 2024 that may be closed by this pull request
@acuarica acuarica self-assigned this Nov 2, 2024
@acuarica acuarica added build Improve build, bundling or deployment process test Improve tests or tests quality labels Nov 2, 2024
Signed-off-by: Luis Mastrangelo <luis@swirldslabs.com>
Signed-off-by: Luis Mastrangelo <luis@swirldslabs.com>
@acuarica acuarica marked this pull request as ready for review November 2, 2024 00:31
@acuarica acuarica requested review from a team as code owners November 2, 2024 00:31
@acuarica acuarica changed the title Move Foundry project to repo root chore: move Foundry project to repo root Nov 2, 2024
Copy link

@mishomihov00 mishomihov00 left a comment

Choose a reason for hiding this comment

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

Review applies to:
.github/workflows/test.yml

@arianejasuwienas
Copy link
Contributor

arianejasuwienas commented Nov 4, 2024

@acuarica there is only one thing that concerns me. Merging this PR will make the architecture from branch:

#86

not working.

Should I move this whole foundry forking library to the parent / root directory as well?

@acuarica
Copy link
Contributor Author

acuarica commented Nov 4, 2024

@arianejasuwienas could you explain why? Keep in mind this is not an architectural change, only a refactor.

@arianejasuwienas
Copy link
Contributor

@acuarica after moving @hts-fork foundry.toml to the parent directory I won't be able to reference it as a library. I'm not sure why, but when I have a structure like:

  • children/foundry.toml,
  • foundry.toml

And I try to include the code from parent as a library in the children (libs = [".."]) then it ends up in the infinite loop of dependencies.

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
Aborted

The problem does not occur for previous structure:

  • children1/foundry.toml,
  • children2/foundry.toml

libs = ["../children2"]

@acuarica
Copy link
Contributor Author

acuarica commented Nov 4, 2024

The idea is to have both solutions under a single Foundry project, not two. See #101 for reference.

@arianejasuwienas
Copy link
Contributor

@acuarica
Ok, I though so. I created this new PR for forking:
#105
(to be merged after this one - #103).

Copy link
Contributor

@victor-yanev victor-yanev left a comment

Choose a reason for hiding this comment

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

LGTM

@acuarica acuarica merged commit aea47ad into main Nov 4, 2024
11 checks passed
@acuarica acuarica deleted the 102-move-foundry-repo-root branch November 4, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Improve build, bundling or deployment process test Improve tests or tests quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Foundry project to the repo root
4 participants