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

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

Closed
kayabaNerve opened this issue Apr 15, 2024 · 4 comments
Closed
Labels
bug Something isn't working

Comments

@kayabaNerve
Copy link

kayabaNerve commented Apr 15, 2024

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.

@kayabaNerve kayabaNerve added the bug Something isn't working label Apr 15, 2024
@gakonst
Copy link
Member

gakonst commented Apr 16, 2024

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
Copy link
Author

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
Copy link
Member

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

@prestwich prestwich closed this as not planned Won't fix, can't repro, duplicate, stale May 28, 2024
@kayabaNerve
Copy link
Author

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 #601 (comment), 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants