Incorporating devtools' tests suites as part of the integration tests

[alcuadrado]

Hi,

I'm creating this issue to propose incorporating parts of the different devtools' test suites as integration tests for solc. We believe this would make the devtools ecosystem much more stable.

Here's the rationale behind this idea.

The current situation has multiple problems:

• Small changes in the compilation output break the devtools without any previous notice
  • This is due to changes in the AST format
  • Also happened a few times in how source maps are emitted
    • Solidity 0.6.3 is a good example
    • Solidity 0.6.9 also break multiple things while reverting the change of 0.6.3
• Devtools have to implement complex and fragile heuristics to keep offering the same functionality to their users
  • If they don't do this, users blame them for breaking
  • The more they have to introduce, the more complex they are to maintain
  • These can break with any minor release, so tool developers live in a constant fear of their tools breaking at any time
• Each version of solc has to be supported forever by the different devtools
  • The reason for this is that once a project deploys an immutable contract compile with a given version of solc, they can't change it on-chain
    • Most projects use multiple versions of Solidity because of this
      • They use newer versions for new contracts
      • They compile already-deployed contracts with the same version used for deployment
      • They run their test combining the output of multiple compilations
        • Tools need to support this usecase, which is extremely common in the defi ecosystem
• The happiness of users with Solidity & Ethereum as a development platform is highly influenced by devtools being stable. Despite this, the Solidity project doesn't currently have an early feedback process on how their actions could affect tools.
  • They only get to know this after the tools break, and if tool developers get to report it
    • Given how often this happens, not everything gets reported.

Proposals for improving this situation

• Incorporate the relevant parts of the main tools' tests suites, as part of Solidity integration testing
  • This has been done in Web3.js recently and helped improved the stability of the project a lot. Some very subtle bugs were discovered, that weren't detected by Web3.js' tests, but broke some consumer's test suites
  • Candidates for this are
    • Truffle
    • Remix
    • Buidler EVM
    • Brownie
      • It does some very advanced things with Solidity, so it can provide coverage to parts of the compiler than other tools won't.
    • Tenderly
      • If the relevant parts are open source. Are they, @BencicAndrej ?
  • This will provide immediate feedback on how changes in Solidity affect the different tools
    • It will also help identify which of them would break, making it easier to start conversations on how to better approach each change
    • This may mean creating a dedicated communication channel for direct consumers of solc. The current channels are too broad for tool developers to keep up to date

[chriseth]

While I agree that we should add external projects' test suites to our CI - we already do that with several smart contracts - I think we should also work on removing the heuristics and replacing them by real debugging data.

As an example, the issue about the source maps you referenced: Especially with activated optimizer we can never guarantee that source maps are fine-grained enough. As soon as code from multiple places results in the same code, this code can only have one source map. So I would say the solution is not undoing the inlining but allowing multiple source locations per opcode instead.

[fzeoli]

As Patricio wrote, tools must support every version of the compiler forever, so unless you mean going back into each compiler version and introducing debugging symbols to each without modifying the compiler's output, the heuristics can't really be replaced. Do you mean to release minor updates for each of the old compilers with additional data?

And I think the point here wasn't to discuss the specific issue about source maps that caused some problems, but the abstract situation of solc changing without coordinating with tool developers causing tools to break for end users, and sometimes creating significant code complexity given that tooling code must support every single compiler version.

Tools have a backwards looking support requirement, so improving things in future versions doesn't mean the issues go away. There's an immutability component to the output of each compiler version that should be managed carefully.

[chriseth]

I don't understand how this is related to older solidity compilers. We cannot modify older compilers but adding external tests to the repo will also not help with older compilers.

I mainly explained the issue about the source maps to exemplify that such a failing test with a dev tool does not necessarily mean that we cannot merge the change, but it means that either the heuristic has to be changed, the PR has to be modified or a totally different solution has to be found that improves the life of everybody.

Regardless of this, I think we have produced way too much text here already. Could you coordinate with tool developers how they can add tests to our repository?

[fzeoli]

Solidity 0.6.3 changed the way source maps work for reverts without messages. Tools like Buidler and Brownie use these source maps to show stack traces to users. When the output of the compiler changed in 0.6.3, Buidler and Brownie need to keep doing everything they were doing before 0.6.3, but in case the compiler being used is 0.6.3 do things differently, and then if the compiler is 0.6.9 also differently. 0.6.9 reverted changes made in 0.6.3, but there's no such thing as really fully reverting, since if someone deployed a contract using 0.6.3 they'll need tools to work with that version of the compiler. Even though you reverted a change, that change still needs to be managed by tools to support your end users in working with Solidity. Code complexity increases a lot due to always having to support every version of the compiler, regardless of what the latest version of the compiler is doing. Once you release a version of the compiler you need to assume someone deployed an immutable contract with it, and needs tooling support for it.

The vast majority of the ecosystem interacts with solc through an orchestration tool (Buidler, Brownie, Truffle, etc), so solc's primary consumer are these tools, and only after them come the Solidity developers.
If a release breaks the tools this directly impacts Solidity developers, who aren't set up to interact directly with the compiler (and they shouldn't). Breaking a tool isn't that much different from breaking the compiler itself, when it comes to the end user's experience (specific features in this Solidity version don't work). If servicing the Solidity developer needs is the responsibility of Solidity, then given that developers interact primarily with Solidity through these tools, servicing the needs of the tools is equally important to servicing the needs of the developers, as the impact on tooling linearly impacts developers. While your software is at the bottom of the stack, the final product users interact with is a joint creation between Solidity and its tools.

Indeed the fact that a change breaks a tool doesn't mean that you can't perform the change, but knowing before the change is released into production allows for coordination and feedback on the full consequences of the change being proposed.

This isn't particularly related to older compilers, but the fact that the current compiler will also one day be considered an older compiler that still needs to be supported by tools. What we want to achieve here is:

1. for increased maintainability of this complexity that tools must endure, by increasing coordination and reducing future issues like the 0.6.3 change having to be managed even though it was reverted a few versions afterward
2. improving the developer experience of interacting with Solidity, which is dependant on the effective collaboration between Solidity and its tools

Generally, I think the coordination between Solidity and the main tools developers use to interface with the compiler is of critical importance for a higher quality Solidity experience. Integrating the tests of tools isn't for the benefits of tools, but for the benefit of Solidity, who's inadvertently publishing broken releases to their users when it comes to the full package Solidity experience of a developer.

Speaking in representation of Buidler, we're happy to make any changes needed to make this possible, and I'm sure the other tools will as well. Beyond this specific project, I think Solidity should create and own a space for generic and fluid coordination with tool developers, and an integration like this would be a great kickoff to this new dynamic.

[chriseth]

@fzeoli @alcuadrado I suspect some misunderstanding in this issue, so I would propose to add additional realtime communication channels to this discussion to clear it up, if needed. My opinion is:

• early feedback through integration tests between solidity and tools is a good idea
• I see two ways to do this:

1. tools use our nightly builds to test: https://github.com/ethereum/solc-bin/raw/gh-pages/bin/soljson-nightly.js
2. tools tell us how to run their tests on a "custom" compiler binary.

While solution 2 allows us to run these tests on pull requests and both the develop and the breaking branch, it needs tight communication because I suspect the solidity team will find it difficult to make out the reason for test failures.

Also, while running tests on the breaking branch sounds very interesting, it probably requires changes to the tools - but this could also be the case for the develop branch.

I'm fine with either solution, so if you would like to go for solution 2, then I would propose to do it with buidler as a first test. How can we exchange the compiler binary in our test runner?

[franzihei]

As part of the discussion call on #8739, we agreed that as a next step tools can supply us with their custom compiler binary (tests need to be 0.6, or soon 0.7, compatible) and we'll try to incorporate them into the Solidity CI tests.

[Disclaimer: I'm taking this from my call notes, please jump in and correct me if I misunderstood anything!]

[chriseth]

... tools can provide a test runner script and a method to exchange the compiler binary so we can run them on our newly built binary.

[montyly]

This is a great initiative, I would be happy to help for the Trail of Bits tools integration.
For context:

• Slither parses the AST from solc, it supports any solc >=0.4, both legacy and compact AST. I think at this point, we can parse 99% of the ASTs for Solidity <0.6, and we are probably closed for >=0.6 (through the support for some new features were only added recently, like the top-level structure/enums)
• We started to parse the YUL's AST too ([WIP] Add YUL support crytic/slither#502). We have a custom parser for YUL for Solidity <0.6 (the inline assembly was a block of text instead of an AST until 0.6). We will open source this parser soon.
• We also parse additional information through crytic-compile, which is a library that acts as a proxy between the compilation frameworks <-> our tools. Typically this will parse the source mapping, the bytecode, and the natspec

For testing, we can either run Slither directly, or I can write a slither-test-parsing util that will only perform the parsing part (so if there is a bug in other places, your tests would not be affected)

[alcuadrado]

Hey @chriseth,

This is now ready on our side. All you need to do to run our test suite with a custom compiler is:

git clone git@github.com:nomiclabs/buidler.git
cd buidler
git checkout development
BUIDLER_TESTS_SOLC_VERSION="0.7.0" BUIDLER_TESTS_SOLC_PATH="/some/path/soljson-v0.7.0+commit.9e61f92b.js" scripts/run-tests-with-custom-solc.sh

(Note the version and path in the last command)

This will install all the necessary dependencies and run the relevant tests. The only requirement is node.js. Any of the currently actively maintained versions would do it.

If the tests fail, the script will exit with a non-zero status number.

[cameel]

Now that I'm done with solc-bin binaries, I might be able to take this on in the near future. For now I have created a separate issue for hooking up Buidler's suite (#10854) because that's the part that can go into the Implementation Backlog already (but does not resolve this issue as a whole).

This may mean creating a dedicated communication channel for direct consumers of solc. The current channels are too broad for tool developers to keep up to date

@franzihei Would the new forum be a good place for that?

[fvictorio]

@cameel notice that all "buidler" usages have to be replaced with "hardhat" now:

git clone git@github.com:nomiclabs/hardhat.git
cd hardhat
git checkout development
HARDHAT_TESTS_SOLC_VERSION="0.7.0" HARDHAT_TESTS_SOLC_PATH="/some/path/soljson-v0.7.0+commit.9e61f92b.js" scripts/run-tests-with-custom-solc.sh

[cameel]

Sure. Thanks for the heads up!

[hrkrshnn]

@cameel can this be closed?

[cameel]

Well, no :) What I did was just #10854. That was meant to be the first step of this broader issue. The point here is to have faster feedback for external tools and Hardhat was meant to be the first one to try out how it works.

As @chriseth said above, it would probably be better if each tooling project had a test run in its own CI using our latest nightly because its team definitely better understands their own tests than us. But it's also OK to provide a simple script (or just a list of commands to run if it's simple) that runs the part of the test suite that tests integration with the compiler and we'll plug it into our CI.

At the very least I think we should have the major frameworks (Hardhat, Truffle, Brownie, dapp-tools) but preferably more.

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.