alloy-rs / core

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

fix: correctly resolve duplicate struct names in json abi #690

Closed klkvr closed 1 month ago

klkvr commented 1 month ago

Motivation

Closes https://github.com/alloy-rs/core/issues/660

Solution

I've added second abi preprocessing method JsonAbi::rename_internal_structs, which goes over ABI, resolves structs that have a same name but different components sets, and renames them into <struct name>0, <struct name>1, etc

It should be also possible to key them by contract instead, however, there are some edge cases related to aliases with such approach. E.g. import {Struct as Struct1} from "..."; import {Struct as Struct2} from "..."; will result in two different structs referenced as Struct in ABI. Same is possible with Lib.Struct if different Libs are aliased.

This edge case is kind of weird, so I am open to changing impl if we consider performance improvements worth it.

Also not sure if we want this to happen on the level of JsonAbi::to_sol instead of only for sol! used code

PR Checklist

klkvr commented 1 month ago

I have a branch open where I make namespaced output work, but got stuck on weird compile errors https://github.com/alloy-rs/core/tree/dani/to-sol-contract-specifiers

yeah, such approach indeed is more flexible

current branch compiles fine for me, do you have more fixes for sol! as wip?

from diff and a quick test it seems that updated Type::Custom branch in rec_expand_type does most of the job on the sol! side? would be happy to finish this if you'll point out what's left/not working yet

DaniPopes commented 1 month ago

It fails to compile in tests in crates/sol-types/tests/macros/sol/json.rs, but expanding the output and compiling it separately is fine

It only handles the ToSol side, expanding libraries, but namespaces are not handled in the sol! internal representation, so function signatures are wrong

Probably best to just continue that branch

klkvr commented 1 month ago

It fails to compile in tests in crates/sol-types/tests/macros/sol/json.rs, but expanding the output and compiling it separately is fine

fyi moving sol! invocation out of the test function scope fixes compilation, will look into why it's happening

klkvr commented 1 month ago

one of the issues seems to be a limitation of function-level modules

e.g. this code doesn't work

fn test_fn() {
    pub mod inner1 {
        pub struct InnerStruct {}
    }

    pub mod inner2 {
        use super::*;
        fn some_fn() {
            inner1::InnerStruct {};
        }
    }
}

and it seems that there's no way to reach other modules defined inside of a function from inside of another module

ref https://github.com/rust-lang/rust/issues/493

DaniPopes commented 1 month ago

Superseded by https://github.com/alloy-rs/core/pull/693, #694