-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(forge): [ALPHA] add soldeer as an optional package manager. #7161
Conversation
I've updated the pull request with version 0.2.6 of soldeer.
|
Updated to version 0.2.7. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if possible, can we also add a simple integration test? see e.g. https://github.com/foundry-rs/foundry/tree/master/crates/forge/tests/it
Co-authored-by: Oliver Nordbjerg <[email protected]>
Resolve conflicts
I have some issues on how to approach this, most of the integrations are related with cheatcodes as i can see. |
@mario-eth Sorry about that I meant to link https://github.com/foundry-rs/foundry/tree/master/crates/forge/tests/cli which has some CLI tests |
Added cli tests here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comment on the dependency format
I also think we ideally ditch the "v" prefix so we can support semver operators later, see e.g. https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-cratesio
Removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is a great first step, lot of things we can add on top and expand upon. keeping it simple for now is a good idea imo, and as long as we agree on how dependencies should be specified long term, the internals do not matter as much, as the experience will be stable for the user
ptal @mattsse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the delay, I'm okay with proceeding here.
only have a few nits
crates/config/src/lib.rs
Outdated
/// Soldeer dependencies | ||
pub dependencies: Option<BTreeMap<String, SoldeerDependency>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want this to be a new type that wraps the betreemap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, does that work? or maybe i misunderstood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last pedantic nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try this!
@mario-eth, before this is merged, wanted to ask on the concept of nested dependencies. I'm using soldeer to install Moreover, the imports from such and similar contracts often use other "package" names, so that if you even install one, you have to remap them manually. Is that something that's being tackled or planned to do after the merge? |
Hey, The reason why it does not track it's because, in your example, what version of @uniswap/lib-dependency does the uniswap-v2-periphery-1.1.0-beta.0 uses? We can't assume it's the last one. While this is the first step, we must start define how we want these dependencies to be tracked, a step towards that i tried to do in soldeer, in the remappings, i tried to map the package+version as a path that it will be hardcoded in the solidity files so that instead of using |
Any timeline when this would be released? Really exited to not use submodules |
Bumped the Soldeer version to v0.2.15. I had to solve two bugs:
|
okay last rebase then I'm sending it |
Motivation
This pull request is related to issue #5966, which essentially outlines the reasons I believe a dedicated dependency manager for Solidity is necessary.
To summarize, relying on git submodules is not a reliable solution. As a security researcher, I've observed numerous projects defaulting to using npmjs to avoid dealing with git submodules. Considering npmjs was designed for the JavaScript ecosystem, why not create a package manager specifically tailored for Solidity?
Solution
My proposal involves developing an open-source Rust-based dependency manager that functions similarly to crates.io or npmjs for Solidity. I'm introducing Soldeer, a straightforward package manager with four key features:
install
: This function allows the installation of dependencies from https://soldeer.xyz/, or from a custom link provided by the user. The dependency must be pre-zipped. For instance, you could zip your dependency, host it on a CDN, and share the link with your team.update
: This function is designed to update the existing dependencies specified in the project.login
: Enables users to log into the repository, allowing them to push new dependencies.push
: Allows users to push a directory to their account on https://soldeer.xyz/ as a new project version.Integration
Soldeer is built as a library that can be also used standalone.
For foundry, i've just integrated the library and based on the commands, i redirect the logic to the library. The integration is not that big.
For reviewing probably one will need to check the open-source github repository to see how it works.
https://github.com/mario-eth/soldeer
Future Plans:
ALPHA STAGE
The current implementation of Soldeer is intended to serve as an alternative to git submodules, rather than a replacement, until it matures sufficiently to become the default package manager.