fix(sol-macro-expander): propagate all_derives and extra_derives to periphery SC structs
#1011
Conversation
squadgazzz
requested review from
DaniPopes,
mattsse and
yash-atreya
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where all_derives and extra_derives attributes weren't propagating to periphery Solidity contract structs like events and errors. The solution delays the cleanup of context attributes until after generating the event and error enums, ensuring they inherit the contract-level derive configurations.
Key changes:
- Move context attributes cleanup (
cx.attrs = prev_cx_attrs) after error and event enum generation - Create separate enum expanders for errors and events while context still has derive attributes
- Move function enum generation after cleanup since function enums don't need derive propagation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/sol-types/tests/derives.rs | Comprehensive test cases validating that all_derives and extra_derives propagate to events and errors |
| crates/sol-macro-expander/src/expand/contract.rs | Implementation changes to delay context cleanup and ensure derive propagation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
squadgazzz
changed the title
all_derives and extra_derives to periphery SC structsall_derives and extra_derives to periphery SC structs
There was a problem hiding this comment.
makes sense, lgtm
this could be problematic for users that currently implement these manually as a workaround but imo we should just derive these
pending @DaniPopes
| let functions_enum = (!functions.is_empty()).then(|| { | ||
| let mut attrs = enum_attrs; | ||
| let doc_str = format!("Container for all the [`{name}`](self) function calls."); | ||
| attrs.push(parse_quote!(#[doc = #doc_str])); | ||
| attrs.push(parse_quote!(#[derive(Clone)])); | ||
| let enum_expander = CallLikeExpander { cx, contract_name: name.clone(), extra_methods }; | ||
| enum_expander.expand(ToExpand::Functions(&functions), attrs) | ||
| }); |
There was a problem hiding this comment.
I see, this just moves the location and add thes derives
There was a problem hiding this comment.
Yeah, it uses the flushed cx.
|
@DaniPopes could you please review this small PR? 🙏 |
|
Why not for the functions enum? |
This gets ignored anyway, I already don't remember where exactly, sorry. And I can't think of any use case for debugging or comparing function enums. |
|
@mattsse may i ask you to merge it? |
…erives` to periphery SC structs (alloy-rs#1011)" This reverts commit fad2642.
3 participants
Motivation
Fixes #1009, which shows that currently
all_derivesandextra_derivesdon't propagate to periphery SC structs, such as errors and events.Solution
Do not clean the context attributes before configuring bindings for periphery structs, and do that only afterward. Function enum is not affected, since it doesn't make much sense to propagate derives there.
PR Checklist