alloy-rs / core

High-performance, well-tested & documented core libraries for Ethereum, in Rust
https://alloy.rs
Apache License 2.0
763 stars 137 forks source link

[Bug] sol macro can't create definitions for files which import types #602

Closed kayabaNerve closed 3 months ago

kayabaNerve commented 5 months ago

Component

sol! macro

What version of Alloy are you on?

0.7.0

Operating System

Linux

Describe the bug

I have the following:

import "./Sandbox.sol";

contract Router {
  // Nonce is incremented for each batch of transactions executed
  uint256 public nonce;

  // The current session
  uint256 public session;
  // Current public key's x-coordinate
  // This key must always have the parity defined within the Schnorr contract
  bytes32 public seraiKey;

  struct OutInstruction {
    address to;
    Call[] calls;

    uint256 value;
  }

  ...
}

where Sandbox.sol contains, at the top level,

struct Call {
  address to;
  uint256 value;
  bytes data;
}

Despite this being compiling Solidity (per solc), the sol macro fails with "unresolved custom type: Call". My invoation is as follows:

sol!("artifacts/Router.sol");

I would call this a feature-request for import resolution, except AFAICT, there's no possibly way to model this behavior through the current sol macro. I'm perfectly fine with needing to do,

sol!("artifacts/Sandbox.sol", "artifacts/Router.sol");

and tried exactly that (which wouldn't require import resolution, which sounds horrific). Because there's no way to model this behavior when using the file API AFAICT, I'd argue sol is incomplete regarding its intended behavior, making this a bug. Apologies if this should be a feature request, and please, feel free to mark it at one.

My workaround for now is using the JSON ABI, yet per the macro's documentation, this is less functional for some cases. It's only because I don't have such cases at this moment, I can personally accept this workaround.

gakonst commented 5 months ago

The sol macro is not a compiler! It's literally parsing the data as passed to it. Please use the compiled ABI! We can explore whether importing multiple files in such case makes sense.

kayabaNerve commented 5 months ago

I understand it's not a compiler nor a preprocessor, hence my request for multiple imports. My reasoning for not wanting to use the JSON ABI is present in the issue (though again, that is the workaround I used).

prestwich commented 3 months ago

our intention for sol! is that solidity-based generation is a tool for rapid iteration on simple projects (e.g. "I just want to call this function"), while json-based codegen is for complex projects and for long-term project maintenance. As a result, we are choosing not to prioritize import resolution or other high-complexity features. JSON is the officially support path for these contracts

I'm going to close this as won't fix. we may revisit sol! should a rust compiler ever pop up

kayabaNerve commented 3 months ago

while json-based codegen is for complex projects and for long-term project maintenance

This seems in conflict with current documentation.

Prefer using Solidity input when possible, as the JSON ABI format omits some information which is useful to this macro, such as enum variants and visibility modifiers on functions.

I do understand this falls under "when possible", and its solely my feelings on how this is written, and I do ack:

IMPORTANT! This is NOT a Solidity compiler, or a substitute for one! It only parses a Solidity-like syntax to generate Rust types, designed for simple interfaces defined inline with your other Rust code.

I don't say this to ask you change your decision though. I just wanted to note my thoughts and can respect the decision.


Per https://github.com/alloy-rs/core/issues/601#issuecomment-2054424694, if the non-macro variant is made public, I believe I can stitch include_strs on my end to solve this (though only in the build-script case, but that's perfectly fine for me). The non-macro variant seems to be moving forward so I'm happy there :) This I note here in case anyone else is looking for a work-around.