Skip to content

feat(forge): Support library linking#586

Merged
brockelmore merged 24 commits intomasterfrom
brock/lib_linking
Jan 31, 2022
Merged

feat(forge): Support library linking#586
brockelmore merged 24 commits intomasterfrom
brock/lib_linking

Conversation

@brockelmore
Copy link
Copy Markdown
Member

@brockelmore brockelmore commented Jan 26, 2022 •
edited
Loading

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
Copy Markdown
Member Author

brockelmore commented Jan 26, 2022 •
edited
Loading

Closes #222, Closes #349

This was linked to issues Jan 26, 2022
Support library linking #222
Closed
Compiling a solidity library file with enum fails #349
Closed
@odyslam
Copy link
Copy Markdown
Contributor

odyslam commented Jan 26, 2022

woot

@tarrencev tarrencev mentioned this pull request Jan 28, 2022
Merged
@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
Copy Markdown
Member Author

brockelmore commented Jan 28, 2022

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

@onbjerg
Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

brockelmore commented Jan 28, 2022

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
Copy Markdown
Member

onbjerg commented Jan 28, 2022

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

@brockelmore
Copy link
Copy Markdown
Member Author

brockelmore commented Jan 28, 2022

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

mattsse
mattsse requested changes Jan 29, 2022
View reviewed changes
Copy link
Copy Markdown
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
@brockelmore @mattsse
Update cli/src/cmd/run.rs
d530f22 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@brockelmore @mattsse
Update forge/src/multi_runner.rs
a950083 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@brockelmore @mattsse
Update cli/src/cmd/run.rs
bd5ff97 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@brockelmore
nits
67dd53d
@brockelmore
refactor post_link
28cc504
gakonst
gakonst reviewed Jan 31, 2022
View reviewed changes
brockelmore and others added 3 commits January 31, 2022 07:41
@brockelmore @gakonst
Update cli/src/cmd/run.rs
9b76df0 
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
@brockelmore @gakonst
Update forge/testdata/LibLinking.sol
e1c24eb 
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
@brockelmore @gakonst
Update cli/src/cmd/run.rs
2179499 
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
gakonst
gakonst approved these changes Jan 31, 2022
View reviewed changes
Copy link
Copy Markdown
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

@brockelmore @gakonst
Update forge/src/multi_runner.rs
95d5886 
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
@brockelmore
Copy link
Copy Markdown
Member Author

brockelmore commented Jan 31, 2022

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
Copy Markdown
Member Author

brockelmore commented Jan 31, 2022

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

@brockelmore brockelmore merged commit 568534d into master Jan 31, 2022
@gakonst gakonst deleted the brock/lib_linking branch January 31, 2022 16:20
@gakonst gakonst mentioned this pull request Jan 31, 2022
Closed
@lattejed lattejed mentioned this pull request Feb 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gakonst gakonst gakonst approved these changes

@mattsse mattsse mattsse approved these changes

Assignees

No one assigned

Labels

T-feature Type: feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Compiling a solidity library file with enum fails Support library linking

5 participants

@brockelmore @odyslam @onbjerg @gakonst @mattsse