-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
meta(library linking): tracking issue for library linking improvements #4443
Comments
Hey adding #4339 here as it seems related. Upon a bit investigation what seems to be happening here is that In
In this case the appropriate fix would be to only have one library deployed. |
thanks, this could also be a script issue. will debug this shortly. lib-linking will also get a makeover |
@mattsse I created a minimal reproduction here: sakulstra/forge-bug-repro@main...bug/library-deploy As you can see the library is included twice in the dry run: |
thanks for this! |
Tried to have a look at an actual fix myself but i don't really understand where to fix this due to lacking rust fu. I guess one could filter duplicates here: https://github.com/foundry-rs/foundry/blob/master/cli/src/cmd/forge/script/cmd.rs#L127 , but seems wrong as probably should not land here in first place. I tried manually deploying the external libraries and using compiler linking, but seems manually deploying libraries doesn't work either as i'm getting |
Has there been any progress on this? It's a pretty big blocker for us. I get there is a rewrite happening, but in the mean time we really need a work around or a fix on this specifically. Anything we can do to help here? |
Here's a possible resolution:
@Evalir will drive here, we've just been less close to this code and hadn't fully appreciated how much of a blocker this would be for people. |
That sounds great. Really appreciate this. |
I think we can close this! Fix was made in #5164 |
Reopening as we've had to revert the fix on #5164 due to strange behavior observed by some users. We're working on a new fix |
While there is a final fix for this issue, I found out a workaround to handle Library deployments with Foundry Solidity scripts and
In this way, the process is automated, there is no library duplication and the developer have full control of the libraries deployment step, allowing the developer to perform the deployment of libraries in one transaction instead of multiple transactions or tweak the order of deployment. The downside is that this requires multiple compilation steps, is tedious to setup and could produce side-effects if the procedure is not correctly verified. Would love to share a snippet to show this pattern to others once I find some time, but hope it could help others in the meantime. |
Hey all! Posting back after a week. We took some days debugging and #5194 was recently merged with an additional fix that should hopefully alleviate these. Could you try the latest nightly and report back if it works for y'all? @kartojal @hexonaut @sakulstra |
It appears to be working. I just did a deploy /w verification of Aave V3 Pool: https://goerli.etherscan.io/address/0x4662c88c542f0954f8ccccde4542eec32d7e7e9a#code |
Closing again, as #5194 has fixed this (again) |
Could you share the snippet that allowed you to achieve this? I am having trouble in obtaining a create2 address for a contract dependent on linked libraries |
Component
Forge
Describe the feature you would like
WIP
Additional context
No response
The text was updated successfully, but these errors were encountered: