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

PoC: Add initial Revive support #4

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

PoC: Add initial Revive support #4

wants to merge 9 commits into from

Conversation

pkhry
Copy link

@pkhry pkhry commented Feb 10, 2025

Description

See this issue

Re-adds Revive support to foundry-compilers.

This PR exists to simplify the code and remove unnecessary implementations that were initially added.

Also this PR integrates resolc into MultiCompiler struct which is used in foundry to compile sources based on the file extension provided. Now MultiCompiler will use resolc by default instead of solc.

used [email protected]+commit.7893614a.Darwin.appleclang and [email protected]+commit.ee650cf.llvm-18.1.8 for testing.

Issues

  • Imports are not resolved correctly right now, so some of the tests are failing.

  • Liberal use of cloning to not deal with ownership for the PoC.(mostly solc and Compiler::Input handling).

  • ResolcContract has toplevel fields that were never populated during testing, namely abi, userdoc, devdoc. Those fields are inside solc_metadata inside metadata field. ResolcContract is based on this struct.

  • Yul output should in which form? We now fail right now as we can't construct a full contract from one field.

TODOS

  • structure returned errors between solc, yul and resolc error kinds for resolc invocation
  • Error out on contracts with unsupportedsolc versions.
  • find a way to run the integration test-suite for both resolc and solc when using MultiCompiler in the calls without introducing flat out code duplication.

Failing tests

can_checkout_repo, can_apply_libraries, can_apply_libraries_with_remappings, can_handle_nested_test_absolute_imports, can_compile_sparse_with_link_references

example error output from resolc:

"Contract `src/LinkTest.sol:LinkTest` compiling error: The contract `src/LinkTest.sol:LinkTest` LLVM IR generator definition pass error: Library `src/MyLib.sol:MyLib` not found in the projectThe contract `src/LinkTest.sol:LinkTest` LLVM IR generator definition pass error: Library `src/MyLib.sol:MyLib` not found in the project"

see issue

can_compile_configured

No ir field present inside Contract in the compilation output.
Should ir_optimized be used instead then?

can_compile_yul_sample, yul_remappings_ignored

object "SimpleStore" {
  code {
    datacopy(0, dataoffset("Runtime"), datasize("Runtime"))
    return(0, datasize("Runtime"))
  }
  object "Runtime" {
    code {
      calldatacopy(0, 0, 36) // write calldata to memory
    }
  }
}

Given the snippet above. resolc will fail with:

"Yul object `./foundry-revive-compiler/test-data/yul-sample/SimpleStore.yul` parsing error: Syntax error: 6:3 Objects must be named as '<name>' (deploy) and '<name>_deployed' (runtime)"

However the error doesn't happen when we call

solc --strict-assembly ./foundry-revive-compiler/test-data/yul-sample/SimpleStore.yul

The error is happening here: click me

can_handle_nested_absolute_imports

see foundry-rs/foundry#2706

can_parse_doc

solidity snippet used:

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

/// @title Not an ERC20.
/// @author Notadev
/// @notice Do not use this.
/// @dev This is not an ERC20 implementation.
/// @custom:experimental This is an experimental contract.
interface INotERC20 {
    /// @notice Transfer tokens.
    /// @dev Transfer `amount` tokens to account `to`.
    /// @param to Target account.
    /// @param amount Transfer amount.
    /// @return A boolean value indicating whether the operation succeeded.
    function transfer(address to, uint256 amount) external returns (bool);

    /// @notice Transfer some tokens.
    /// @dev Emitted when transfer.
    /// @param from Source account.
    /// @param to Target account.
    /// @param value Transfer amount.
    event Transfer(address indexed from, address indexed to, uint256 value);

    /// @notice Insufficient balance for transfer.
    /// @dev Needed `required` but only `available` available.
    /// @param available Balance available.
    /// @param required Requested amount to transfer.
    error InsufficientBalance(uint256 available, uint256 required);
}

contract NotERC20 is INotERC20 {
    /// @inheritdoc INotERC20
    function transfer(address to, uint256 amount) external returns (bool) {
        return false;
    }
}

The test fails becuase INotERC20 has no userdoc or devdoc for some reason, while NotERC20 has the userdoc and devdoc but not all fields are populated.

consistent_bytecode

Contract.ir is missing, we only have ir_optimized from resolc

@pkhry pkhry self-assigned this Feb 10, 2025
}

#[cfg(test)]
mod tests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these tests

Copy link
Author

Choose a reason for hiding this comment

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

but why most of them were there in the first place, like most of them got removed anyways.
do you think leaving test that check things below are useless?

  • version parsing
  • compiling a small .json example

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had added these just to check if the code i wrote was fine im not sure if they added any value since its just simple assertions, but i suppose if they ok then no need to remove

}

impl Resolc {
/// When creating a new Resolc Compiler instance for now we only care for
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these comments

Copy link
Author

Choose a reason for hiding this comment

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

which "these"? a lot of the comments ended up being outdated or redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ones above new

/// We install but as mentioned this could change as it may not be the best
/// approach since requirements are going to change
pub fn new(revive_path: PathBuf, solc_compiler: SolcCompiler) -> Result<Self> {
Ok(Self { resolc: revive_path, solc: solc_compiler })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we would want to error out if the solc being used is less than 0.8.0. i recall we dont support compiling contracts with versions lower than that as such we need to check during initialization for the version of solc being used

Copy link
Author

Choose a reason for hiding this comment

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

Yup, ResolcRestrictions still need to be integrated into the compilation pipeline

/// We install but as mentioned this could change as it may not be the best
/// approach since requirements are going to change
pub fn new(revive_path: PathBuf, solc_compiler: SolcCompiler) -> Result<Self> {
Ok(Self { resolc: revive_path, solc: solc_compiler })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we not check if the revive path exists to avoid issues down the line?

Copy link
Author

Choose a reason for hiding this comment

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

Yup

output: Output,
}

impl From<ResolcContract> for foundry_compilers_artifacts_solc::Contract {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say instead of using the from we implement custom deserializer to ensure we check for direct abi/userdoc/devdoc and and fields before falling back to metadata values since some revive outputs include these directly.

Copy link
Author

Choose a reason for hiding this comment

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

Can you provide a snippet of solidity code where upon compiling we actually get the docs back on toplevel struct?

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.

2 participants