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(cast): cast interface generates unexpected library instead of interface #9960

Open
2 tasks done
sakulstra opened this issue Feb 26, 2025 · 7 comments
Open
2 tasks done
Labels
C-cast Command: cast T-bug Type: bug

Comments

@sakulstra
Copy link
Contributor

sakulstra commented Feb 26, 2025

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

9bcfbaa

What version of Foundryup are you on?

No response

What command(s) is the bug in?

cast interface

Operating System

None

Describe the bug

Hey, i think there is a regression in cast interface (or at least an unexpected change).

I have a:

interface ITestContract {
  struct TestStruct {
    address asset;
  }
}

contract TestContract is ITestContract {
  function test(TestStruct memory) external {}
}

Now cast interface ./src/Test.sol:TestContract will yield:

pragma solidity ^0.8.4;

library ITestContract {
    struct TestStruct {
        address asset;
    }
}

interface TestContract {
    function test(ITestContract.TestStruct memory) external;
}

Which I think different to previous versions as i'm pretty sure that before structs were included in the generated interface. Is this change intentional?, as ux wise it's a bit weird.


More of a question out of curiosity: Would it be possible to include the natspec on the generated interface?
In most smart contracts i'm reading i have to constantly jump between interface and contract to fully understand the code. Therefore i'm wondering if it wouldn't be a better approach to just have all the natspec on the actual contract (not the interface) and then simply generate the interface.

@sakulstra sakulstra added T-bug Type: bug T-needs-triage Type: this issue needs to be labelled labels Feb 26, 2025
@github-project-automation github-project-automation bot moved this to Todo in Foundry Feb 26, 2025
@zerosnacks
Copy link
Member

This is likely related to us moving from Solang to Solar

/// Converts a vector of tuples containing the ABI and contract name into a vector of
/// `InterfaceSource` objects.
fn get_interfaces(abis: Vec<(JsonAbi, String)>) -> Result<Vec<InterfaceSource>> {
abis.into_iter()
.map(|(contract_abi, name)| {
let source = match foundry_cli::utils::abi_to_solidity(&contract_abi, &name) {
Ok(generated_source) => generated_source,
Err(e) => {
warn!("Failed to format interface for {name}: {e}");
contract_abi.to_sol(&name, None)
}
};
Ok(InterfaceSource { json_abi: serde_json::to_string_pretty(&contract_abi)?, source })
})
.collect()
}

@zerosnacks zerosnacks added the C-cast Command: cast label Feb 26, 2025
@zerosnacks zerosnacks changed the title cast interface generates unexpected library bug(cast): cast interface generates unexpected library instead of interface Feb 26, 2025
@zerosnacks zerosnacks added T-to-reproduce Type: requires reproduction and removed T-needs-triage Type: this issue needs to be labelled labels Feb 26, 2025
@grandizzy
Copy link
Collaborator

@sakulstra I just checked with an (really) old build and there is no regression introduced here, how do you think the output should look?

CC @zerosnacks

@zerosnacks
Copy link
Member

Interesting, I would expect it to yield an output like this where ITestContract is an interface, not a library

interface ITestContract {
    struct TestStruct {
        address asset;
    }
}

interface TestContract is ITestContract {
    function test(ITestContract.TestStruct memory) external;
}

@sakulstra
Copy link
Contributor Author

Trying to figure out which version we were on, but seems like in fact we manually patched the interface because in the version there was some problem with the scoping and i simply forgot.

how do you think the output should look

I would have expected:

pragma solidity ^0.8.4;

interface TestContract {
    struct TestStruct {
        address asset;
    }
    function test(TestStruct memory) external;
}

At least how we currently use cast interface is to build an aggregated interface for contracts with complex inheritance structure. So that you can access Ownable, or AccessControl which might not be defined on the contract itself.
Generating multiple interfaces / libraries kinda defeats the purpose a bit.

@grandizzy grandizzy removed the T-to-reproduce Type: requires reproduction label Feb 28, 2025
@grandizzy
Copy link
Collaborator

thanks. ah, I think I was making a mistake and could be a regression at the end, anyways, will look into solving it

@grandizzy
Copy link
Collaborator

grandizzy commented Feb 28, 2025

confirmed it's an regression introduced in d8f6631, most probably in alloy here https://github.com/alloy-rs/core/blob/5b29eb74afb26417704833a0985a2e35bd218ee6/crates/json-abi/src/to_sol.rs#L154-L175

@grandizzy
Copy link
Collaborator

after checking the alloy code I think that's something desired and not an issue, @DaniPopes could you pls chime in? thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cast Command: cast T-bug Type: bug
Projects
Status: Todo
Development

No branches or pull requests

3 participants