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

fix: use method signature to allow using functional polymorphism #40

Closed
wants to merge 1 commit into from

Conversation

bmateus
Copy link
Contributor

@bmateus bmateus commented Aug 2, 2024

issue discussed here

@hiddentao
Copy link
Contributor

Thanks for the submission!

Just tested this against the https://github.com/gemstation/contracts-foundry repo and got test failures. I think the code might need a bit of tweaking.

@hiddentao
Copy link
Contributor

hiddentao commented Aug 12, 2024

The issue is that functions which take structs as parameters won't have the correct selectors generated. Doing so would require deconstructing the struct and nested structs and then doing something like:

struct InnerStruct {
    uint256 x;
    bool y;
}

struct OuterStruct {
    uint256 a;
    address b;
    InnerStruct c;
}

function complexFunction(OuterStruct memory param) public {
}

// selector => complexFunction((uint256,address,(uint256,bool)))

Getting this working requires much larger parsing work, not to mention finding where structs are located. So for now I propose doing the following:

  • Generate the raw bytes4 selector using your new code whenever possible
  • Fallback to using functionName.selector when the function contains a struct and issuing a warning to the gemforge log output of this.

hiddentao added a commit that referenced this pull request Aug 12, 2024
hiddentao added a commit that referenced this pull request Aug 12, 2024
* fix: use method signature instead of just the name to allow using functional polymorphism
* fix: comprehensive fix for #40

---------

Co-authored-by: Bruno Mateus <bmateus@gmail.com>
@hiddentao
Copy link
Contributor

Merged in #42

@hiddentao hiddentao closed this Aug 12, 2024
@bmateus
Copy link
Contributor Author

bmateus commented Aug 13, 2024

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants