combineSources for large library imports does not work

[0mkara]

combineSources function loops through duplicate imports and try to resolve files even if its already resolved. This makes impossible of aragonOS and alike library usage.

[0mkara]

After some work and re-thinking about the compilation issue I feel this issue should be resolved by redesigning the way Etheratom resolves imports now. Bellow is the current model of how Etheratom resolves imports now.

Proposed solution

Etheratom should go through all the imports. Resolve them using resolver-engine. Put all the contents together and send it for compile. This whole process should be carried out by a node child process. This child process should emit information about currently resolving file so that etheratom can display the information to users.

combineSource() & import resolve & compilation related code can be found at - https://github.com/0mkara/etheratom/blob/master/lib/web3/methods.js#L39, https://github.com/0mkara/etheratom/blob/master/lib/helpers/compiler-imports.js

Please feel free to improve the proposal and or suggest any better solution to the problem you have in mind.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 500.0 DAI (500.0 USD @ $1.0/DAI) attached to it as part of the ECF fund.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
  • Want to chip in? Add your own contribution here.
  • Questions? Checkout Gitcoin Help or the Gitcoin Slack
  • $44,968.41 more funded OSS Work available on the Gitcoin Issue Explorer

[evgeniuz]

@0mkara There's an issue with https://github.com/resolver-engine/resolver-engine: it's not published on NPM yet. But this issue can be fixed without resolver-engine:

There are couple of approaches:

1. fix issue in the current combineSource without relying on external libraries (this approach has minimal changes, so less likely to break anything);
2. rely on some other resolvers, one good candidate would be truffle-compile package that includes import parser and resolver (and also it's one of the most popular frameworks, so having full compatibility with that might be good idea);

Both approaches can be implemented as a child process (although it's not really needed in first approach if this issue is fixed).

What approach should I take?

[0mkara]

Hi @evgeniuz, I wanted to use resolver-engine because that aims to resolve all the imports from web or local itself. If truffle-compile suits the purpose then its fine to use truffle-compile.

How can you fix current combineSource ? Although it resolves github imports & local imports pretty good but there exists several problems.

If you can take a look into remix repo and follow similar approach to resolve imports it will be even better because etheratom tends to use remix libraries mostly.

For all the approaches of resolving imports I think it is necessary to implement child process, because resolving github imports from web takes time. While imports are resolved we don't want to block the main process flow.

p.s I would prefer if you can use remix approach or resolver-engine. I can add small fixes into remix if required. resolver-engine is also a new library and I think it will be easier to add fixes if required.

[yann300]

I've heard that a team is working on providing standardisation around import. can't remember the name right now..

[evgeniuz]

How can you fix current combineSource ? Although it resolves github imports & local imports pretty good but there exists several problems.

It's possible to track sources that were already processed and skip them, preventing this issue.

For all the approaches of resolving imports I think it is necessary to implement child process, because resolving github imports from web takes time. While imports are resolved we don't want to block the main process flow.

In current implementation the github imports themselves shouldn't block main process, as that is async call, i. e. it starts fetching and releases flow until it's completed. The problem with this issue is that essentially there is infinite cycle, that blocks main process (it's not doing much itself, but it's sync and doesn't release flow).

You are right, truffle doesn't have github imports, only NPM, EPM and local. Will try to take a look at remix.

[0mkara]

I just have a conversation with @yann300 and we are going to separate compiler-imports from remix-ide to remix. So I think we should use this module to do the task.

We can add this as related issue and if possible you can implement both.

[evgeniuz]

Ok, just reference this issue here somewhere and I will do it as well.

[gitcoinbot]

@evgeniuz Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

@evgeniuz due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

• [x] reminder (3 days)
• [x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[evgeniuz]

I am continuing working on this, will submit PR shortly to related remix issue.

[0mkara]

https://github.com/ethereum/remix/tree/remix-resolve/remix-resolve in this branch I tried to gather all existing code related to resolving imports. And https://github.com/ethereum/remix/issues/1059 here is the issue where we are talking about this module.

For now remix team has decided to work on this module internally.

[0mkara]

@evgeniuz can you take a look at https://github.com/ethereum/remix/tree/remix-resolve/remix-resolve branch and try resolving this issue with that library ?

Let me know if you have any implementation suggestions.

[gitcoinbot]

@evgeniuz Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[spm32]

Hey @evgeniuz any updates on this front?

[evgeniuz]

@0mkara Hi, there were couple of minor issues with configuration this library: sleep package and bin setting in package.json were causing issues with npm link (to add it as dependency, since it's not published yet), and both are not needed. I'm now working on moving all import handling into it's own worker.

My approach (although not working yet) to remix-resolve implementation was to put resolution into remix-solidity and provide one more callback to Compiler: handleLocalImport. Then we can use gatherImports function of Compiler to parse sources and fetch imports. Reason for second callback (handleLocalImport) is that etheratom uses local FS, but remix uses file providers, so it's impossible to abstract local imports into remix-resolve. My idea was that this separated implementation will load remote imports for local import it will use handleLocalImport callback, thus supporting both remix and etheratom.

[0mkara]

@evgeniuz yeah you are right. There are many problems with resolving imports. As remix-ide has its own files provider, we will not be able to implement remix-resolve as common module. On recent updates we have decided to implement a PoC for this as a node module. This will be used in several remix modules for resolving imports.

The latest solc compiler has a callback function. Upon calling solc.compile(input, handleImportCb) this callback function pushes missing import into an array. Then remix-ide tries to resolve the file and sends the sources object to the compiler. This process repeats itself unless all the imports are resolved.

The current implementation of remix-resolve library has a combineSource() function. Etheratom should have a worker that will use remix-resolve.combineSource() function to gather all the imports and then send that object to the compiler worker. It would be helpful if you can implement that first.

Then on the remix-resolve module we plan to do few things to fix import issues.

• [ ] get all the imports into one single object.

  const sources = {
      'greeter.sol': { content: 'pragma solidity ^0.5.0;\n\ncontract Greeter is Mortal....' },
      '../mortal.sol': { content: 'pragma solidity ^0.5.0;\n\ncontract Mortal....' }
  }

  Note: This object will have full path of the files as in import statements.

• [ ] when a missing file is required in handleImportCb(), search that file in the previous mentioned object and return its contents.

  Future improvements

• [ ] use cache to not resolve github/gitlab imports repeatedly
• [ ] resolve swarm, ipfs imports
• [ ] use keccak256 hashes of files

Sorry that we don't have a clear API and all the details are fuzzy.

[evgeniuz]

@0mkara in addition to #223, I've also submitted https://github.com/ethereum/remix/pull/1085, it fixes minor issue in combineSource function and package.json configuration issue that prevents installation in atom.

[gitcoinbot]

@evgeniuz Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[evgeniuz]

@0mkara just a heads up, is there anything I can improve here? Or just wait until remix-resolve is published and then check that it's still working against published version. Current state is:

• import worker is implemented as atom task and works in background;
• notifies with notifications if there's an issue with import;
• uses remix-resolve (for now locally linked, until it is published) to resolve imports;
• test for imports are fixed to work with background task.

[evgeniuz]

P. S. I've rebased https://github.com/ethereum/remix/pull/1085 on top of recent changes. Also, please, note that currently remix-resolve/src/index.js does not export combineSource.

[0mkara]

@evgeniuz we should wait till remix-resolve is published. There are some big changes going to be implemented in remix libraries. We should wait till these changes are published. We will need atleast 2 weeks for anything further.

[gitcoinbot]

@evgeniuz Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [ ] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[rmshea]

hey @evgeniuz any updates on this?

[eswarasai]

@0mkara - Is this one still open and not being worked on? If so, can you please let me know what exactly is needed here as I can see based on the above conversations, there's still a dependency on remix-resolve to be published? Thanks!

[0mkara]

This issue is being addressed in our new release work v4.5.0. After that release if it still does not work, we will let you know.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

Work has been started.

These users each claimed they can complete the work by 2 days from now. Please review their action plans below:

1) prorokeskobar1 has started work.

Vitalik's tweetstorm response to your request

Learn more on the Gitcoin Issue Details page.

[0mkara]

@jordankpuri if you are working on it please provide your workplan.