###### tags: `Technical Notes` # Solcjs Pull Request Status Report (04.10.2022) 1. "What?": A summary of what the PR does. 2. "Reviews summary": A attempt to collect and merge the ideas suggested by the reviewers to better understand which direction the PR should go from now on. 3. "Decision": A suggestion of what to do or questions about what is still unclear about the PR. `*` Some descriptions were freely taken from the original authors in their respective PRs. ## tl;dr ### Close - [x] <https://github.com/ethereum/solc-js/pull/205> already implemented in other prs and we will not do part of it. (**@cameel**: 👍) - [x] <https://github.com/ethereum/solc-js/pull/210> already implemented (closed) (**@cameel**: 👍) - [x] <https://github.com/ethereum/solc-js/pull/442> already implemented in other prs (**@cameel**: 👍) - [x] <https://github.com/ethereum/solc-js/pull/447> we will not do it (**@cameel**: 👍) - [x] <https://github.com/ethereum/solc-js/pull/476> we will not do it (**@cameel**: 👍) - [ ] <https://github.com/ethereum/solc-js/pull/630> we will not do it (**@cameel**: needs team decision) ### Stale: OP can finish or we take over - [ ] <https://github.com/ethereum/solc-js/pull/242> (**@cameel**: 👍) - [x] <https://github.com/ethereum/solc-js/pull/298> (**@cameel**: close) - [x] <https://github.com/ethereum/solc-js/pull/415> (**@cameel**: close PR, implement later) - [x] <https://github.com/ethereum/solc-js/pull/416> (**@cameel**: close PR, implement later) - [ ] <https://github.com/ethereum/solc-js/pull/441> (**@cameel**: close PR, create issues) ### In Progress - [x] <https://github.com/ethereum/solc-js/pull/524> (**@cameel**: 👍) - [ ] <https://github.com/ethereum/solc-js/pull/609> (**@cameel**: 👍 but probably fine to take over) - [ ] <https://github.com/ethereum/solc-js/pull/615> (**@cameel**: 👍, badly needs reviews) - [x] <https://github.com/ethereum/solc-js/pull/662> (**@cameel**: merged) ### Waiting approval/comments - [x] <https://github.com/ethereum/solc-js/pull/590> (**@cameel**: close) - [ ] <https://github.com/ethereum/solc-js/pull/648> (**@cameel**: 👍) - [ ] <https://github.com/ethereum/solc-js/pull/659> (**@cameel**: 👍) ### Need review - [ ] <https://github.com/ethereum/solc-js/pull/658> (**@cameel**: 👍) - [ ] <https://github.com/ethereum/solc-js/pull/220> take over (**@cameel**: 👍) - [ ] <https://github.com/ethereum/solc-js/pull/563> take over (**@cameel**: needs review, @axic will finish) - [ ] <https://github.com/ethereum/solc-js/pull/312> (**@cameel**: 👍) ## List of PRs (by chronological order) ### [Adds Typescript definitions #205](https://github.com/ethereum/solc-js/pull/205) (2018) 1. What? Add an initial set of typescript definitions. 2. Reviews summary Should not enforce type definitions for JSON input/output format, since they are part of Solidity and not solc-js. Update tests to use the type definitions, and thus certify that they work properly. We should only add types to the public methods on wrapper (e.g. without the standard JSON/ABI) - see PR#615 3. Decision Should it be closed in favor of the [#615](https://github.com/ethereum/solc-js/pull/615)? Still relevant to add type definitions for the JSON and ABI? PR[#630](https://github.com/ethereum/solc-js/pull/630) attempted to do it but it seems that it was a concensus that this is not the way to proceeed. The main concern is how to easily maintain the sync between solidity breaking changes and solcjs type definitions. Therefore, I think we should close it. 4. Other opinions **@cameel**: I'd close, in favor of #630. Still not sure what to do about #630 but of the two it seems more comprehensive anyway so we can safely close #205. ### [Implements solc-latest feature for nightly builds #210](https://github.com/ethereum/solc-js/pull/210) (2018) 1. What? The current PR changes `getVersionList` to download the latest nightly version instead of package.json release version. 2. Reviews summary It may be better to promote the `loadRemoteVersion` feature to work with nightlies. 3. Decision Close it. `loadRemoteVersion` can already download nightly builds based on the version string. However, it seems there is a bug since the string version is rewritten to `-ci` instead of `-nightly`. For instance: ```javascript= solc.loadRemoteVersion("v0.8.15-nightly.2022.5.27+commit.095cc647", (err, compiler) => { console.log(compiler.version()) }) ``` Returns: `0.8.15-ci.2022.5.27+commit.095cc647.Emscripten.clang` 4. Other opinions **@cameel**: Maintaining a separate branch that tracks the nightly is not the best solution IMO. I agree that we're better off closing it. ### [Change rules for selecting filename for generated files #220](https://github.com/ethereum/solc-js/pull/220) (2018) 1. What? Solcjs generate different file names than solc. The PR proposes to generate the same output as solc in solcjs by changing the regex in `contractFileName` in solc.js [line 238](https://github.com/ethereum/solc-js/blob/master/solc.ts#L238). Example: ``` solcjs --bin -o ./bin/contract-artifacts ./contracts/* solc --bin -o ./bin/contract-artifacts ./contracts/* solcjs: contracts_test_sol_LookupContract.bin solc: LookupContract.bin ``` 2. Reviews summary The solution works but raised the question about what happens in case of collisions. `solc` has the option `--overwrite` to force update when collisions are detected. 3. Decision Extend the PR to add the overwrite option to solcjs cli as well. Ask the OP about the possibility of taking over. 4. Other opinions **@cameel**: Looks like a good change, but needs proper handling of collisions. Adding `--overwrite` sounds fine too. IMO fine to take it over and finish. ### [Fix translating linkReferences #242](https://github.com/ethereum/solc-js/pull/242) (2018) 1. What? Ensures backward compatibility with `linkReferences` of old compiler versions. 2. Reviews summary The PR modifies the `linker.ts` to perform an additional test in the library name placeholder and extract the correct file and library name. Changes `linkReferences` to a mapping of mapping instead of array. Requires test to verify the behaviour. The test should use `findLinkReferences` with an old style link reference (`__LibraryName_____...`) and check if the output match the JSON standard. Starting from Solidity version `0.8.1` the separator `=` was introduced and thus should also be supported by the regex. Reference: https://docs.soliditylang.org/en/develop/using-the-compiler.html#library-linking 3. Decision We should do it. Ask the OP about the possibility of taking over. 4. Other opinions **@cameel**: We should eproduce it first - it does not link to an issue and there's no failing test case there so hard to say if the issue is real and which versions it affects. If it's real, we should take over and finish it. ### [Adds TypeScript definition for the package's supported public surface area #298](https://github.com/ethereum/solc-js/pull/298) (2018) 1. What? Splits PR #205 adding type definitions to the `wrapper.ts`. 2. Reviews summary It should also include definitions for other entry points: `abi`, `linker`, and `translate`. Most probably as separate modules, as they can be used independently. Add a test case that uses the type definitions and ensure that the interfaces are valid. Note: The OP proposes to change the `compileStandardWrapper` function (and potentially other) to accept and returns javascript objects instead of JSON strings, which would ease the use and verification of such in applications using solc-js. However, this would introduce breaking changes and require more discussion. 3. Decision We should finish it. Add the missing definitions and apply them to the tests. Ask the OP about the possibility of taking over. About the change from JSON strings to objects, this need to be decided later. Open an issue for that? 4. Other opinions **@cameel**: The PRs is very outdated and was created before the big API refactor we did in 0.5.0 (see [#120](https://github.com/ethereum/solc-js/issues/120)). It's also small enough that there's not much to salvage there. I think that [#615](https://github.com/ethereum/solc-js/pull/615/files) does the same thing and more for the current API. We should close it. ### [Link bytecode using references #312](https://github.com/ethereum/solc-js/pull/312) (2018) 1. What? Fixes [Update linkBytecode to support linkReferences structure from jsonio #193](https://github.com/ethereum/solc-js/issues/193). Makes `linkReferences` map an optional third parameter for `linkBytecode`. If it is not provided then `findLinkReferences` is used to find it. If library name is not present in `linkReferences` then it falls back to manual search. 2. Reviews summary It seems that the PR was taken over. There is no comment about its status. 3. Decision We should finish it. Need to update the code based on the current typescript version. 4. Other opinions **@cameel**: We should finish it. ### [Change solcjs to be async #415](https://github.com/ethereum/solc-js/pull/415) (2019) 1. What? Makes solcjs cli async. 2. Reviews summary None so far. 3. Decision We should do it. However the PR still a **draft**. Ask the OP about the possibility of taking over. Having the cli async would ease the solutions of some issues. For instance: - [TypeError: Cannot read properties of undefined (reading '1') in solc.js when passing the input for --standard-json via stdin #601](https://github.com/ethereum/solc-js/issues/601) and related ["Error: EAGAIN: resource temporarily unavailable, read" when using --standard-json flag #460](https://github.com/ethereum/solc-js/issues/460). The [process.stdin.fd](https://github.com/ethereum/solc-js/blob/master/solc.ts#L127) is not a real tty file descriptor in OS X. Also, the nodejs `process.stdin` is a stream, and an attempt to read it synchronously will return EAGAIN error when there is nothing to read. Changing the solc-js cli to be asynchronous would ease reading from the stdin stream. 4. Other opinions **@cameel**: Does it actually have anything substantial in it? Does not look like it's making anything async yet. Since there's so little code in it, I think we should close it and maybe create an issue for the task. Then do it later in due order, considering priorities of other tasks. **@r0qs**: Actually it doesn't do anything substantial indeed. I just liked the idea and would like to do it ;) And I think it would help to improve the `cli.ts` and potentially fix some issues that we have in it. So, for now, I will just open an issue as you said. ### [Export load and loadRemoteVersion without loading a compiler #416](https://github.com/ethereum/solc-js/pull/416) (2019) 1. What? It changes the loaded compiler wrapper object not to include the `loadRemoteVersion` method. The method is exported independently. 2. Reviews summary It was accepted but the PR was outdated and with failing CI test. The PR still a **draft**. 3. Decision We should do it since it make sense to split the logic of the local and remote load. It requires update the code to the current state of the repo. It introduces a **breaking changes** in the API for projects using the`loadRemoteVersion`, like [Remix](https://github.com/ethereum/remix-project/blob/master/libs/remix-solidity/src/compiler/compiler.ts#L186). Ask the OP about the possibility of taking over. 4. Other opinions **@cameel**: This code has been changed completely so the PR will need to be redone from scratch. It's also breaking so it's not urgent (we won't merge it until 0.9.0). I think we should close it, report an issue and implement later. ### [Workarounds for running tests against wasm rebuilds of old compilers. #441](https://github.com/ethereum/solc-js/pull/441) (2020) 1. What? It adds tests to verify the backward compatibility of compiler versions in the solc-js cli. 2. Reviews summary There is currently a set of shellscripts that were used to recreate wasm build of old versions of the solidity compiler (available at [wasm-rebuild](https://github.com/ethereum/solidity/tree/develop/scripts/wasm-rebuild) directory). The OP suggests that if we plan to make the CLI compatible with old versions, then we should always test it agaist those versions. Potentially as part of a CI job to check backward compatibility of the CLI. He also suggests to adapt the current test suite (based on [tape](https://www.npmjs.com/package/tape)) to perform the tests agaist a particular `soljson.js` version (i.e. make the `soljson.js` a optional argument to the tests, where the latest release is the default). The PR is a **draft**. 3. Decision If we plan to support old versions in the CLI, we definitely should finish this PR. The `compiler.ts` already has a method `runTests` in which it receives a wrapped `soljson.js` as parameter and run tests on it. A similar logic could be applied in the CLI tests wrapping the tape logic in a similar `runTests` method. Ask the OP about the possibility of taking over. Idea: We could have a configuration file for the tests (json or yml) with the list of versions that need to be considered in all test cases (not only compiler or cli). We could also add a flag to our test suite to only run for the latest version if necessary. And the test suite could just call the `exec` to run the tape on each specific version. 4. Other opinions **@cameel**: We do support old versions in the CLI. This is just about testing them. I think it's a good idea to make it possible, but also not urgent. Since the PR is so small, I'd close it and instead create an issue to make the CLI testable with different versions. I also like the idea of having the test suite always run against a single `soljson` binary. Then we'd adjust the CI config to run it with versions that are currently hard-coded. We could also have a run that tests all release binaries and runs on tagged release commits. This would be a separate issue though. ### [Migrating to Typescript #442](https://github.com/ethereum/solc-js/pull/442) (2020) 1. What? Migrate the library to typescript. 2. Reviews summary The PR is a complete rewrite of the solc-js, and thus it is very hard to review. The tests were also rewritten and there is no way to know if the current functionality of the library was preserved. The PR also adds runtime dependencies, not only dev-dependencies (i.e. axios). 3. Decision We should close it in favor of the already existing migration to typescript. Namely: - PR [#614](https://github.com/ethereum/solc-js/pull/614) which was already merged. - PR [#615](https://github.com/ethereum/solc-js/pull/615) in progress - PR [#205](https://github.com/ethereum/solc-js/pull/205) if it still relevant. - PR [#298](https://github.com/ethereum/solc-js/pull/298) needs refactor 4. Other opinions **@cameel**: We should close. We're currently in the middle of doing an alternative approach by sthephensli. There won't be anything to salvage when we're done. ### [Ability to override remote download URL with env var #447](https://github.com/ethereum/solc-js/pull/447) (2020) 1. What? The PR proposes overwrite the download URL when fetching remote compiler versions using a environment variable. 2. Reviews summary The hashes of the downloaded versions are not enforced and we should not encourage the use of unofficial compiler versions. 3. Decision Close it since it is not a desirable feature. 4. Other opinions **@cameel**: We should close. I think that being able to download the binaries from different sources would be a good thing but the list should still come from a trusted source. So if we do it we won't reuse the code from the PR anyway. ### [Add option to return warning as error #476](https://github.com/ethereum/solc-js/pull/476) (2020) 1. What? It adds an option in the CLI to return an warning as error. 2. Reviews summary The CLI should match the `solc` behaviour and in issue [solidity#10277](https://github.com/ethereum/solidity/issues/10277) it was decided to not add such functionality in the compiler. 3. Decision Close it, as we will not implement it. 4. Other opinions **@cameel**: Agreed, we shouled close. The CLI in solc-js should match `solc`. ### [Proper SMT CHC tests #524](https://github.com/ethereum/solc-js/pull/524) (2021) 1. What? The PR adds SMTChecker tests. 2. Reviews summary The PR still a **draft**. It depends on PR [solidity#11421](https://github.com/ethereum/solidity/pull/11421) which was already merged. 3. Decision We should have it. The tests need to be updated to the current solc-js version. Ask the OP to finish it and review. 4. Other opinions **@cameel**: This is @leo's work-in-progress PR, important for the SMTChecker. I think it's stuck mostly on the lack of PRs. Definitely don't close. He will finish it eventually. ### [Sanitize nightly version string which had leading zeroes #563](https://github.com/ethereum/solc-js/pull/563) (2021) 1. What? Fixes issue [Older nightlies with leading zeros in version string crash soljson #562](https://github.com/ethereum/solc-js/issues/562). It adjusts the `versionToSemver` to handle old nightly version style. 2. Reviews summary The work seems almost done. However, the reviewer identified one mistake that was not fixed yet. The suggested regex expression has some corner cases for two-digit month and day values that don't start with 0. Like 12 or 27. But the expression below fixes it: ```javascript= const nightlyParsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0?([1-9]|1[012])\.0?(3[01]+|[12]?[0-9])(.*)$/); ``` 3. Decision We should do it. Fix the regex, rebase and merge. 4. Other opinions **@cameel**: The PR was actually updated after my review so it might already be done. So tt's stuck waiting for a review. @axic is active now and can finish it. ### [Add a wrapper script to keep ./solcjs working #590](https://github.com/ethereum/solc-js/pull/590) (2022) 1. What? The rename of the solc binary from `solcjs` to `solc-js` done in PR [#587](https://github.com/ethereum/solc-js/pull/587)broke any tests that relies on the binary name. The PR adds a shellscript named `solcjs` that wraps the `solc.js` binary resulted from the package build in order to keep it compatible with the old CLI behaviour. 2. Reviews summary The symlink `node_modules/.bin/` is only created when an npm package is installed as a dependency. The way that solidity uses solc-js, by cloning and building the repo, does not create the symlink. The command `npm link` does not help since it creates the symlink from the global node_modules to the package directory. Still, one suggestion that needs to be applied by the OP (making the directory changes in the script scoped). And not all reviewers agreed with the proposed solution. The OP pointed out that it is missing development documentation for solc-js. 3. Decision To keep it backward compatible, it seems fine to me to merge it after applying the suggested changes. We should also open an issue to add documentation of development usage in solc-js. 4. Other opinions **@cameel**: I'd close it. It was meant for backwards compatibility but at this point it's moot. We already released without it and it does not seem to have caused problems. ### [Initial bits for integrating the LSP API. #609](https://github.com/ethereum/solc-js/pull/609) (2022) 1. What? The PR introduces the use of the language server in solc-js. 2. Reviews summary The PR still a **draft**. The code needs linting and tests for the introduced functionalities. The documentation also need some improvement. 3. Decision We should have it. Ask the OP to finish it and review. 4. Other opinions **@cameel**: We need to finish it but I think there are some problems to be resolved first. @chrisparpart will finish it eventually but he's focused on the `solc` part now and I suspect he would not mind if someone good with JS/TS took over the solc-js part. ### [Provide type support to wrapper.ts #615](https://github.com/ethereum/solc-js/pull/615) (2022) 1. What? The PR adds type support to the `wrapper.ts` methods. It also export the to the consumer of the solc module. It has one dependency that already got merged, PR [#614](https://github.com/ethereum/solc-js/pull/614). 2. Reviews summary The PR looks good in general. However, one major concern is the mix of the type definitions with helpers functions. This was not introduced by the specific PR, but maybe could be already solved by it, since the PR already refactor these files. 3. Decision We should have it. The OP is currently working on finish the PR. A suggestion about how the library could be published was given but required consensus. 4. Other opinions **@cameel**: This is an important part of our TypeScript migration effort. OP is an external contributor willing to get it through to the end and just need to review his contributions. I think that mixing definitions with functions is a separate concern. Should potentially be reported as an issue, to be resolved later. ### [Add types for compiler input & output #630](https://github.com/ethereum/solc-js/pull/630) (2022) 1. What? It adds type for the compiler input and output standard and ABI spec. 2. Reviews summary This functionality was discussed initially on PR[#205](https://github.com/ethereum/solc-js/pull/205#discussion_r172769874), and it seems a hard to maintain feature since the JSON I/O standard is not consistent between compiler versions. The reviewer suggested some alternatives for a better direction of the PR, since solc-js is meant to support all compiler versions and JSON I/O standards: a) Creating a JSON schema in the Solidity repo and its use in the solc-js, and other clients. b) Keep type definitions for different versions (mainly the breaking ones) or a superset of it. The reviewer is in favor of adding a JSON schema generator in the main compiler repo and initially adding support to it in solc-js for the current version. 3. Decision Option (a) seems the most reasonable one, and that will require less maintenance over time. The PR can be closed since it will not be the way that we will probably go. 4. Other opinions **@cameel**: This needs discussion in the team and a decision. Will probably be closed eventually but I'd leave it open until we decide. You could add it to our language discussion agenda for one of the meetings. ### [Add a test to check the download of the latest version #648](https://github.com/ethereum/solc-js/pull/648) (2022) 1. What? The PR adds unit tests to `getVersionList` and `downloadBinary` functions and modifies them to allow testing. The overall goal is to ensure that the downloaded compiler versions are correct. 2. Reviews summary The reviewer suggested mocking positive and negative cases for both functions, which was done. He also suggested the creation of a CI job to perform an integration test (this was added in another subsequent PR[#659](https://github.com/ethereum/solc-js/pull/659). The PR adds one dev-dependency (i.e. nock) to mock https connections. It also adds some mocked files, like dummy certificates. GitGuardian complained about the dummy keys used by the mocked https server. 3. Decision The PR is waiting for approval. 4. Other opinions **@cameel**: Good PR. Just needs reviews. ### [Update documentation #658](https://github.com/ethereum/solc-js/pull/658) (2022) 1. What? This PR updates the documentation of solc-js fixing the following issues: - Adds documentation recommending the use of [SRI](https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity) when fetching the binaries: https://github.com/ethereum/solc-js/issues/334 - Adds documentation about the necessity of using long version string in `loadRemoteVersion`: https://github.com/ethereum/solc-js/issues/583 - Adds note about backward compatibility of low-level legacy functions: https://github.com/ethereum/solc-js/issues/610 - Fix ES6 import syntax: https://github.com/ethereum/solc-js/issues/627 2. Reviews summary Not reviewed. 3. Decision Should have it. But I need to choose somebody to review it ;) 4. Other opinions **@cameel**: Good PR. Just needs reviews. ### [Add CI job to check if updateBinary downloads the correct release #659](https://github.com/ethereum/solc-js/pull/659) (2022) 1. What? It adds a CI job to test if the current version is equal to the downloaded release when using the `updateBinary` command. 2. Reviews summary The reviewer suggested shell script linting changes, such as: - 4-space indents in shell scripts - newline on the end of files - use `"${x}"` form when the variable is surrounded by text in a string and `"$x"` when it's on it's own - use `local` when declaring local function variables - quote variables - use of double square brackets in conditional expression in bash scripts (bash operators extension) He also suggested the addition of shell coding style in the solidity [CODING_STYLE.md](https://github.com/ethereum/solidity/blob/develop/CODING_STYLE.md), and the configuration of a shellcheck script for solc-js repo. The latter was done in this PR[#662](https://github.com/ethereum/solc-js/pull/662). 3. Decision The PR seems done and is waiting for approval. 4. Other opinions **@cameel**: Good PR. Just needs reviews. ### [Add CI job to check coding style #662](https://github.com/ethereum/solc-js/pull/662) (2022) 1. What? The PR adds a CI job to perform coding style checks in the solc-js repo. The runs the linter for typescript and shell script files. 2. Reviews summary Not reviewed. 3. Decision Should have it. 4. Other opinions **@cameel**: Already reviewed and merged.