feat(`dependencies`): support pinning of tags / revs when using `.gitmodules` with `foundry.lock`

[yash-atreya]

Motivation

Closes #7225

• git submodule doesn't have support for pinning to tags/revisions, only branches in .gitmodules
• This creates unintended behavior in forge when updating deps.
• Reproduction of unintended behaviour

1. Install oz with tag forge install OpenZeppelin/[email protected].
2. git submodule status shows the oz dep checked out at https://github.com/OpenZeppelin/openzeppelin-contracts/commit/69c8def5f222ff96f2b5beff05dfba996368aa79
3. Run forge update
4. oz dep is now checked out at the most recent commit of the master branch instead of the commit of the tag.

Solution

Introduce a basic lockfile foundry.lock that consists of a mapping from relative lib_path to DepIdentifier.

Every time forge update is run, this file is inferred to correctly check out the dep and maintain tag or rev pinning if specified.

In case we want to override a dep and set a new tag, this can be done like so: forge update owner/dep@new-tag

TODO

• [x] account for foundry.lock generation the first time.
• [x] migration tests
• [x] monorepo test
• [x] forge remove

[yash-atreya]

I think this makes sense overall, 2 scenarios was thinking of

1. update deps of dependencies

• forge init and then install specific tag forge install OpenZeppelin/openzeppelin-contracts@tag=v4.9.4. This will result in 2 deps in lib/openzeppelin-contracts/lib:

erc4626-tests
forge-std

• forge update updates dependencies of Oz dependency, resulting in 3 deps

erc4626-tests
forge-std
halmos-cheatcodes

This probably cannot be fixed even when openzeppelin-contracts contracts will add its own forge-submodule-info.json?

2. updating dependency to a different version

• cd in lib/openzeppelin-contracts/ and git checkout v5.0.2
• forge update silently checks out v4.9.4
• in order to persist then forge-submodule-info.json should be manually edited and "lib/openzeppelin-contracts":{"Tag":"v4.9.4"} updated to "lib/openzeppelin-contracts":{"Tag":"v5.0.2"}

Maybe on forge update we should print out versions (and should follow up with docs / book update).

(1). Yeah, this is because we run git submodule update --remote as before, and checkout to the correct tag/rev after the updates have been downloaded. EDIT: On second thought, we could remove the --remote flag for deps that specify a tag/rev, this should fix it. Now this is as good as not running update for these deps as --remote flag is only intended for modules pinned to a branch.

(2) Deps can be updated along with the values in foundry.lock like so forge update OpenZeppelin/[email protected] no need to manually checkout any lib or edit forge-submodule-info.json

[zerosnacks]

When running:

forge init counter (commit)

followed by

forge install openzeppelin/openzeppelin-contracts@tag=v4.9.4 the following error is thrown

Installing openzeppelin-contracts in /home/zerosnacks/Projects/counter/lib/openzeppelin-contracts (url: Some("https://github.com/openzeppelin/openzeppelin-contracts"), tag: Some("v4.9.4"))
Error: The target directory is a part of or on its own an already initialized git repository,
and it requires clean working and staging areas, including no untracked files.

Check the current git repository's status with `git status`.
Then, you can track files with `git add ...` and then commit them with `git commit`,
ignore them in the `.gitignore` file, or run this command again with the `--no-commit` flag.

It looks like the foundry.lock file is being written prior to .gitmodules being created which causes this issue

This also means the foundry.lock is added to the repo whilst the .gitmodules addition reverts causing issues when trying to re-run

Side note: it was never really clear to me why we are creating a commit upon adding a dependency

[zerosnacks]

introduce a basic lockfile foundry.lock that consists of a mapping from relative lib_path to DepIdentifier. This file is committed with every forge install.

because we don't auto-commit anymore this is no longer true - correct?

[zerosnacks]

Would be great if this PR was accompanied by an update to the book to document the forge update owner/dep@new-tag behavior as it will be a common question

[yash-atreya]

Would be great if this PR was accompanied by an update to the book to document the forge update owner/dep@new-tag behavior as it will be a common question

@grandizzy @zerosnacks

Related book PR: https://github.com/foundry-rs/book/pull/1494

[yash-atreya]

@zerosnacks conflicts resolved, rfr

[mario-eth]

I'm gonna open a separate discussion but wouldn't make more sense to make forge install route via soldeer instead? Soldeer is becoming one of the most used package manager second to github and i feel like making it a default to forge makes the more sense.

The only concern people had with soldeer, even tho, just a few did raise this concern, was the backend being a web2 component. BUT you can just use soldeer straight with github bypassing any backend interaction.

I wrote to this issue to reconsider if this makes sense to finish it or not cause soldeer.lock already does a mapping of dependencies.

[yash-atreya]

Code lgtm 👍

I am however unable to succesfully run forge update to update OpenZeppelin 5.1.0 to 5.3.0 as I was previously hinting at a regression in recent changes

Steps to reproduce:

forge init counter cd counter forge install OpenZeppelin/[email protected] git add . + git commit -m 'init' forge update

Expected: updates to 5.3.0, instead remains at 5.1.0

Yes, this is expected behaviour and indeed a breaking change from the current version.

However, point to note, in the current forge version if you ran forge update it would update OZ to it's master branch. This is unsafe behaviour as the master branch may not be audited. OZ has also put a warning in their repo about this:

If you wish to update to the v5.3.0 tag, you must specify it in the forge update command like so:

forge update OpenZeppelin/[email protected]

Documentation already updated accordingly: https://getfoundry.sh/forge/reference/forge-update#lockfile

[sambacha]

sorry to drive by, but it would be nice to have this lockfile conform to GitHub's dependency API endpoint for submissions.

https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/using-the-dependency-submission-api

We consider all packages that are defined in the Package URL spec as being valid package types.

see https://github.com/github/dependency-submission-toolkit/blob/main/src/package.ts

Can we have an optional field for minimum solc version per dep?

[zerosnacks]

Good call, I think it is worth considering how to make this lockfile format future proof inside of the context of this PR. I would like to avoid introducing optional fields in this PR, we should spec and scope that independently to avoid being a blocker. cc @yash-atreya @grandizzy

[zerosnacks]

After reflecting, imo we ship the lockfile as is. If we do decide to make a breaking change and start applying a versioning scheme we can introduce a version field then. We can then also decide to make the lockfile adhere to the proposed package URL spec. For the time being this is out of scope, the lockfile is at this point only really relevant to us.