Skip to content

feat(forge): Support library linking #586

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

Merged
merged 24 commits into from
Jan 31, 2022
Merged

feat(forge): Support library linking #586

merged 24 commits into from
Jan 31, 2022

Conversation

brockelmore
Copy link
Member

@brockelmore brockelmore commented Jan 26, 2022

Adds support for library linking at deploy time. Works by determining dependencies, calculating address in correct order, linking the dependency addresses recursively on the way up to a target contract, and deploying dependencies before deploying a test contract.

Todo:

  • create command support
  • run command support
  • merge upstream changes into ethers-rs

@onbjerg onbjerg added the T-feature Type: feature label Jan 26, 2022
@brockelmore
Copy link
Member Author

brockelmore commented Jan 26, 2022

Closes #222, Closes #349

@odyslam
Copy link
Contributor

odyslam commented Jan 26, 2022

woot

@brockelmore brockelmore marked this pull request as ready for review January 28, 2022 20:54
@brockelmore brockelmore requested a review from gakonst January 28, 2022 21:11
@brockelmore
Copy link
Member Author

dropping trying to support create as it is kinda counter-intuitive for that command to deploy multiple contracts

@onbjerg
Copy link
Collaborator

onbjerg commented Jan 28, 2022

dropping trying to support create as it is kinda counter-intuitive for that command to deploy multiple contracts

how does dapptools do it? ig u deploy lib manually and then pass it as an opt?

@brockelmore
Copy link
Member Author

how does dapptools do it? ig u deploy lib manually and then pass it as an opt?

ye - we already support dapptools style compile-time linking. also the official solidity docs recommend not manually linking and letting the compiler do it. For contracts going on mainnet, I tend to agree.

@onbjerg
Copy link
Collaborator

onbjerg commented Jan 28, 2022

any chance we can dedupe the linking part by extracting it as a function?

@brockelmore
Copy link
Member Author

ye we probably can - there are some differences between the two but probably can be combined 90%

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

halfway through
highlighted the things that stood out.
need another stab at it to fully understand what's going on

brockelmore and others added 5 commits January 29, 2022 17:07
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
brockelmore and others added 3 commits January 31, 2022 07:41
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

approving cautiously, given integration tests seem to pass I feel OK about it.

2 things as follow-ups:

  1. Is there any large real repo with libs and linking which we could use as an integration test?
  2. Should we upstream these into ethers-solc?

Feels like we should do a full review of ethers-solc / or a writeup because the pipeline has gotten quite complex at this point

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
@brockelmore
Copy link
Member Author

approving cautiously, given integration tests seem to pass I feel OK about it.

2 things as follow-ups:

1. Is there any large real repo with libs and linking which we could use as an integration test?

2. Should we upstream these into ethers-solc?

Feels like we should do a full review of ethers-solc / or a writeup because the pipeline has gotten quite complex at this point

  1. I tested with Scott Sumarto's library who opened the Lib.Enum issue. It is private for now, but when it opens up i think it should be a good test as he rated it as "as complex as it can get" for library linking and we verified the linking together.
  2. maybe? there are some intricacies here that make me say no, but i would be okay with it

we should def do a writeup/review, lmk how you want to proceed on that front

@brockelmore
Copy link
Member Author

Closes #612 but we should open another issue for decoding collided function signatures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiling a solidity library file with enum fails Support library linking
5 participants