Skip to content
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

Add all resolc dependencies to resolc_packed.js file #176

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

smiasojed
Copy link
Collaborator

@smiasojed smiasojed commented Jan 27, 2025

Add all resolc dependencies to resolc_packed.js file
Format the JS files
Fix the issue with module overwriting in the preloaded JS

This change requires support for CSP headers on the hosting server.
Most probably it will be difficult to host compilers on GH Pages because
HTTP Headers (e.g. Content-Security-Policy) are not supported

We could temporarily host compilers on revive-remix-backend, where CSP is supported.

@smiasojed smiasojed changed the title Compilation flags cleanup Add all resolc dependencies to resolc_packed.js file Jan 29, 2025
@smiasojed smiasojed marked this pull request as ready for review January 29, 2025 14:48
Comment on lines +5 to +6
const SOLJSON_URI =
"https://binaries.soliditylang.org/wasm/soljson-v0.8.28+commit.7893614a.js";
Copy link
Member

Choose a reason for hiding this comment

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

Are we planning to this dynamically inside remix in the future?

I think this should be done client side. In remix , the user can select the solc version there. Remix will download resolc and bundle it together. This way we can support all solc versions (since resolc is always compatible with any solc, the resolc version can be hardcoded to whatever is currently compatible with the node).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How it works in Remix: Remix lists the compiler files and loads the selected soljson-0.8.28+commit.xxxx. The file must follow this format because the soljson-0.8.28 part is used internally, for example, in pragma checking.

Steps to bundle it on the Remix side:
Modify the compiler tab to browse two lists of compilers.
Store this information in Remix's internal objects.
Update the web worker logic and API to accept two compilers.
Modify the non worker compilation flow in remix.
Modify the test logic, as it also expects a single compiler.

This change will affect many files in Remix. I wanted to implement it, but first, I need to run Remix's unit and E2E tests. A side effect of this change is that any future Remix updates will take more time to integrate.

I think I could remove soljson dep from revive and add it during compiler list generation on backend

Copy link
Member

Choose a reason for hiding this comment

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

We have two options here:

  1. Implement this properly in remix and don't bundle anything together.
  2. Bundle resolc + solc. The solc version bundled is the latest (supported) version. We declare support for other solc versions as out of scope. Honestly I don't think remix is being used for serious dApp development and deployments but rather as a playground for quick experimentation, demos and so forth.

The second option would also allow us to spend much less resources in optimizing the compiler for the in-browser environments. Honestly I think the second option would allow us to move forward fast and focus on more important issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to decide where to host the compilers if GitHub Pages does not support enabling CSP headers.
When creating the resolc_packed (bundle) release, we must upload the WASM file to a server that supports CSP.

Copy link
Collaborator Author

@smiasojed smiasojed Jan 30, 2025

Choose a reason for hiding this comment

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

I will put them in the Remix repo. No one else will use them, so it's safe to keep them there.

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -87,6 +87,7 @@ jobs:
path: |
${{ env.REVIVE_WASM_INSTALL_DIR }}/resolc.js
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to release it? Does it have any worth over resolc_packed.js? How would you use it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolc.js makes sense for use with Node.js, where it is not dynamically downloaded, for example in js-revive

@@ -87,6 +87,7 @@ jobs:
path: |
${{ env.REVIVE_WASM_INSTALL_DIR }}/resolc.js
${{ env.REVIVE_WASM_INSTALL_DIR }}/resolc.wasm
${{ env.REVIVE_WASM_INSTALL_DIR }}/resolc_packed.js
Copy link
Member

Choose a reason for hiding this comment

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

Does the suffix packed make sense here? Given that this is not really packed but loads the wasm file (i.e not containing it).

Copy link
Collaborator Author

@smiasojed smiasojed Feb 3, 2025

Choose a reason for hiding this comment

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

It does not make sense. I am not sure if I will merge it—perhaps I should find a better solution.
Such a solution has a few issues:

  1. We need to store the WASM file on a public server with CSP enabled - it will not work with GH pages.
  2. During the release process, we would need to upload it to such a server and properly set RESOLC_WASM_URI = "http://127.0.0.1:8080/resolc.wasm, which is embedded in resolc.js.

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

Successfully merging this pull request may close these issues.

3 participants