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

Update ERC-7821: Remove Call Struct from Specification #883

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

McOso
Copy link

@McOso McOso commented Feb 3, 2025

Update (Feb 7, 2025): Based on convo, this PR just removes the Call struct from the Specification section since it is not needed to standardize the execution interface. The Reference Implementation has been left unchanged.


ERC-7821 implies using a Call struct. ERC-7579 already leverages an Execution struct in their reference implementation.

This is to change the Call struct to the referenced ERC-7579 Execution struct.


Another option is to remove the struct from this standard altogether since you are really trying to standardize the execute interface

@jxom
Copy link

jxom commented Feb 6, 2025

Why does it need to match the 7579 struct? It's just a naming thing, no?

@McOso
Copy link
Author

McOso commented Feb 6, 2025

Why does it need to match the 7579 struct? It's just a naming thing, no?

I just think it is helpful to match the same struct since the ERC is already referencing 7579. Really, call struct could just be removed from this ERC since the goal isn't to standardize the call struct, it is to standardize the execution interface.

@jxom
Copy link

jxom commented Feb 6, 2025

It references 7579 on the execute & supportsExecutionMode signatures specifically – I don't think other aspects of this ERC necessarily need to reference/be completely aligned with 7579 semantics.

@Vectorized
Copy link
Contributor

Vectorized commented Feb 7, 2025

I prefer Call, since it is shorter.

You can always rename it to Execution tho. We just need to call it something.

Solady’s LibERC7579 doesn’t have a Call struct, so you can use it to build with your own preferred names.

We can add a note saying that this is the Execution struct, and users are free to rename it, as long as the type and order of the fields stay the same.

@McOso
Copy link
Author

McOso commented Feb 7, 2025

I don't think other aspects of this ERC necessarily need to reference/be completely aligned with 7579 semantics.
Yeah, I'm leaning to it being just unnecessary to define any struct in this standard.

@Vectorized what if we remove the Call struct from the specification section, since you are trying to standardize the execution interface and not the call struct. But then you can use the Call struct in the reference implementation?

@jxom
Copy link

jxom commented Feb 7, 2025

Yeah, don't think it would hurt to remove L41-45 as it's not being used in the Overview. We can leave the Reference Implementation as-is.

@McOso McOso force-pushed the fix/7821-call-type branch from fb7a2c8 to 3666ca3 Compare February 7, 2025 13:44
@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Feb 7, 2025

File ERCS/erc-7821.md

Requires 1 more reviewers from @Amxx, @jxom, @Vectorized

@McOso
Copy link
Author

McOso commented Feb 7, 2025

Updated the PR based on the convo. The change now just removes the Call struct from the Specification section but keeps the reference implementation as is.

@McOso McOso changed the title Update ERC-7821: Replace Call struct with 7579 Execution struct Update ERC-7821: Remove Call Struct from Specification Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants