Libraries can link to other libraries with public functions #1252

asselstine · 2019-10-17T20:18:51Z

I needed to deploy a library that linked to another library that had public functions. I made changes to allow the deployment of any dependent libraries.

Altered NetworkController#_getAllSolidityLibNames to recursively retrieve libraries and detect cycles
Altered NetworkController#uploadSolidityLibs to push libraries in order so that dependencies to ensure linking addresses exist
Altered NetworkController#_uploadSolidityLib to link libraries

spalladino · 2019-10-18T18:43:01Z

Hey @asselstine, thanks for the contribution! We'll check why the CI is complaining (there is some error with truffle apparently). In the meantime, could we ask you to add a test for this, in order to merge it? Thanks!

asselstine · 2019-10-18T22:43:21Z

@spalladino I didn't see any tests for the NetworkController so I thought I might get away with no tests ;) I'll make sure to add one.

I did notice that I had to add the CI flag when running locally; i.e. CI=true npm test. It couldn't seem to compile the artifacts correctly otherwise.

asselstine · 2019-10-29T04:30:19Z

@spalladino tests added!

spalladino · 2019-10-29T19:13:32Z

Awesome, thanks so much Brendan! Will review during the week :-)

spalladino

Awesome work Brendan, thanks a lot! I left a question on the loop detection. I may be wrong, but I think there is a scenario where it may yield false positives. Let me know if you have the availability to review this!

packages/cli/src/models/network/NetworkController.ts

asselstine · 2019-10-31T17:05:08Z

@spalladino I've improved the algorithm and added a test to ensure that it orders dependencies correctly. I think this is the one!

ylv-io · 2019-11-05T15:02:34Z

Hey @asselstine! Thanks for the awesome work 🚢
I have an idea to try to use a recursion for the _getAllSolidityLibNames method just to make it more compact and readable. What do you think about that?

asselstine · 2019-11-05T17:28:09Z

@ylv-io Good idea; it would be a good use of recursion. I've added you as a collaborator to the fork, so feel free to push to this branch.

packages/cli/src/models/network/NetworkController.ts

packages/cli/test/models/network/NetworkController.test.js

ylv-io · 2019-11-05T19:08:31Z

@asselstine for some reason it didn't work. I can't push and not on the list of collaborators.

asselstine · 2019-11-05T22:34:27Z

@ylv-io Github's "invitations" are overly complicated, in my opinion. I've had this problem countless times.

Your invite to the fork is still pending, so I'm guessing you need to check the email that is attached to your Github profile. There will be an invite email from the Github repo. Once you click that you'll be able to push to the branch.

ylv-io · 2019-11-06T10:14:30Z

@asselstine That is exactly the case. Indeed, this is a confusing system.

packages/cli/src/models/network/NetworkController.ts

asselstine · 2019-11-06T18:55:33Z

Come to think of it, cycle detection is needed for this function. It should throw an error if there is a library dependency ordering such as:

Lib_1 requires Lib_2
Lib_2 requires Lib_3
Lib_3 requires Lib_1

While the above can be compiled, it can't be deployed all at once so OZ SDK should throw an error here.

ylv-io · 2019-11-07T14:09:55Z

Hey @asselstine. I've added suggested changes. Thanks again for your amazing contribution!

asselstine

@ylv-io great collaboration! I like the recursion and the tests are tight.

packages/cli/test/models/network/NetworkController.test.js

asselstine · 2019-11-07T23:53:59Z

I realize that we are still treating loop as legit DAGs; as per Santiago's original comment.

What we need is a topological sort.

I'll have a look at integrating one.

- Altered NetworkController#_getAllSolidityLibNames to recursively retrieve libraries and detect cycles - Altered NetworkController#uploadSolidityLibs to push libraries in order so that dependencies to ensure linking addresses exist - Altered NetworkController#_uploadSolidityLib to link libraries

…for hoisting dependencies

asselstine · 2019-11-08T00:34:18Z

@ylv-io Ok. This should be it!

It now recursively builds up a graph which is then topologically sorted.

Repeated deps
cycle detection.

I think we're done.

ylv-io · 2019-11-08T14:05:33Z

@asselstine. It is a pleasure to collaborate with you!
Using TopologicalSort is a great idea. Just reminds me that we should always stand of the shoulders of giants!
I've polished code a bit and If there will be any CI tests errors let me handle that!

nventuro

Great work @asselstine and @ylv-io! Topological sorting indeed seems like the perfect solution for this problem

packages/cli/package.json

packages/cli/src/models/network/NetworkController.ts

packages/cli/test/models/network/NetworkController.test.js

Co-Authored-By: Nicolás Venturo <[email protected]>

ylv-io · 2019-11-28T06:56:14Z

I've replaced the topological-sort library with toposort. I also had to adjust tests which is not ideal but required because toposort outputs libs in the different order which is still valid.

nventuro

Thank you very much @asselstine and @ylv-io! This looks ready to merge 😁

nventuro · 2019-11-28T15:00:32Z

packages/cli/src/models/network/NetworkController.ts


    contractNames.forEach(contractName => {
-      const deps = this._getContractDependencies(contractName);
-      deps.forEach(dependencyContractName => {


Nice, I hadn't realized this loop was duplicated 👍

nventuro · 2019-11-28T17:56:20Z

The CI test run is done on the branch, not on the result of the merge: it turns out this PR broke tests on the CLI due to #1288.

nventuro · 2019-11-28T18:00:17Z

Fixed in 7f641a7

asselstine · 2019-11-28T18:04:36Z

Great collab @ylv-io! I'm pleased to help and I'm excited to have this in the build. Now I won't have to worry about our library structure.

ylv-io · 2019-11-29T07:47:38Z

@nventuro this is a great example of how CI tests can be successful on a branch but break a master.

spalladino · 2020-01-22T14:30:14Z

Hey @asselstine, this PR is now part of the 2.7 RC if you want to try it!

asselstine · 2020-01-22T19:04:19Z

@spalladino Fantastic!

spalladino added the status:changes-requested PR needs changes before approving label Oct 18, 2019

asselstine force-pushed the feature/linked-libraries branch from b380887 to 112014d Compare October 29, 2019 04:27

spalladino added status:to-review Awaiting review and removed status:changes-requested PR needs changes before approving labels Oct 29, 2019

spalladino reviewed Oct 30, 2019

View reviewed changes

packages/cli/src/models/network/NetworkController.ts Outdated Show resolved Hide resolved

spalladino assigned ylv-io Nov 5, 2019

ylv-io reviewed Nov 5, 2019

View reviewed changes

packages/cli/src/models/network/NetworkController.ts Outdated Show resolved Hide resolved

ylv-io reviewed Nov 5, 2019

View reviewed changes

packages/cli/test/models/network/NetworkController.test.js Outdated Show resolved Hide resolved

ylv-io requested a review from spalladino November 6, 2019 11:42

asselstine commented Nov 6, 2019

View reviewed changes

packages/cli/src/models/network/NetworkController.ts Outdated Show resolved Hide resolved

packages/cli/src/models/network/NetworkController.ts Outdated Show resolved Hide resolved

asselstine commented Nov 7, 2019

View reviewed changes

packages/cli/test/models/network/NetworkController.test.js Outdated Show resolved Hide resolved

asselstine and others added 6 commits November 7, 2019 16:24

Added test for cli NetworkController

5f6ff6b

Cleaned up red herrings from ImplV1b

22192df

Added mock-stblib-libdeps to root yarn.lock

8a6d799

Improved dependency collection in NetworkController and added a test …

f10d5ae

…for hoisting dependencies

Recursive version of GetSolidityLibNames

8a845a2

ylv-io and others added 3 commits November 7, 2019 16:28

Fix update.test.js

6598155

Fix circular deps

1088251

Integrated topological sorting into the library dependency resolution

d49cbf9

asselstine force-pushed the feature/linked-libraries branch from d4f2a47 to d49cbf9 Compare November 8, 2019 00:28

Merged camelcase changes from master

e247236

Refactor code a bit

929f8f8

nventuro reviewed Nov 26, 2019

View reviewed changes

ylv-io and others added 4 commits November 27, 2019 20:38

Update packages/cli/src/models/network/NetworkController.ts

c7278e9

Co-Authored-By: Nicolás Venturo <[email protected]>

Update packages/cli/test/models/network/NetworkController.test.js

d3c8859

Co-Authored-By: Nicolás Venturo <[email protected]>

Update packages/cli/test/models/network/NetworkController.test.js

3e8a2c5

Co-Authored-By: Nicolás Venturo <[email protected]>

Switch to toposort instead of topological-sort

80e8c28

ylv-io requested review from ylv-io and nventuro November 28, 2019 06:56

nventuro approved these changes Nov 28, 2019

View reviewed changes

nventuro merged commit 813ab58 into OpenZeppelin:master Nov 28, 2019

spalladino mentioned this pull request Dec 12, 2019

Handle recursive solidity libraries #243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Libraries can link to other libraries with public functions #1252

Libraries can link to other libraries with public functions #1252

asselstine commented Oct 17, 2019

spalladino commented Oct 18, 2019

asselstine commented Oct 18, 2019

asselstine commented Oct 29, 2019

spalladino commented Oct 29, 2019

spalladino left a comment •

edited

Loading

asselstine commented Oct 31, 2019

ylv-io commented Nov 5, 2019 •

edited

Loading

asselstine commented Nov 5, 2019

ylv-io commented Nov 5, 2019

asselstine commented Nov 5, 2019

ylv-io commented Nov 6, 2019

asselstine commented Nov 6, 2019

ylv-io commented Nov 7, 2019

asselstine left a comment

asselstine commented Nov 7, 2019

asselstine commented Nov 8, 2019 •

edited

Loading

ylv-io commented Nov 8, 2019

nventuro left a comment

ylv-io commented Nov 28, 2019

nventuro left a comment

nventuro Nov 28, 2019

nventuro commented Nov 28, 2019

nventuro commented Nov 28, 2019

asselstine commented Nov 28, 2019

ylv-io commented Nov 29, 2019

spalladino commented Jan 22, 2020

asselstine commented Jan 22, 2020

Libraries can link to other libraries with public functions #1252

Libraries can link to other libraries with public functions #1252

Conversation

asselstine commented Oct 17, 2019

spalladino commented Oct 18, 2019

asselstine commented Oct 18, 2019

asselstine commented Oct 29, 2019

spalladino commented Oct 29, 2019

spalladino left a comment • edited Loading

Choose a reason for hiding this comment

asselstine commented Oct 31, 2019

ylv-io commented Nov 5, 2019 • edited Loading

asselstine commented Nov 5, 2019

ylv-io commented Nov 5, 2019

asselstine commented Nov 5, 2019

ylv-io commented Nov 6, 2019

asselstine commented Nov 6, 2019

ylv-io commented Nov 7, 2019

asselstine left a comment

Choose a reason for hiding this comment

asselstine commented Nov 7, 2019

asselstine commented Nov 8, 2019 • edited Loading

ylv-io commented Nov 8, 2019

nventuro left a comment

Choose a reason for hiding this comment

ylv-io commented Nov 28, 2019

nventuro left a comment

Choose a reason for hiding this comment

nventuro Nov 28, 2019

Choose a reason for hiding this comment

nventuro commented Nov 28, 2019

nventuro commented Nov 28, 2019

asselstine commented Nov 28, 2019

ylv-io commented Nov 29, 2019

spalladino commented Jan 22, 2020

asselstine commented Jan 22, 2020

spalladino left a comment •

edited

Loading

ylv-io commented Nov 5, 2019 •

edited

Loading

asselstine commented Nov 8, 2019 •

edited

Loading