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

Add functionStaticCall and functionDelegateCall methods to Address library #2333

Merged
merged 15 commits into from
Sep 17, 2020

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Aug 20, 2020

Consider having functionStaticCall and functionDelegateCall methods in Address library.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @k06a! Did you experiment with ways to avoid repeating the code? It seems the only thing that changes is the line with the actuall call. I wonder if Solidity's function types could be used here, and how gas would be affected.

contracts/utils/Address.sol Outdated Show resolved Hide resolved
contracts/utils/Address.sol Outdated Show resolved Hide resolved
k06a and others added 2 commits August 24, 2020 22:00
Co-authored-by: Francisco Giordano <[email protected]>
Co-authored-by: Francisco Giordano <[email protected]>
@frangio
Copy link
Contributor

frangio commented Aug 24, 2020

@k06a Do you have any comments on the topic of avoiding the repeated code?

@k06a
Copy link
Contributor Author

k06a commented Aug 24, 2020

@frangio we could aggregate only call and delegatecall since they are both non-view methods, else view modifier will not work :(

@frangio
Copy link
Contributor

frangio commented Aug 24, 2020

Ah that's a good point.

I think we should at least be able to put the if conditional inside a separate private function that can be reused though. That would be a nice improvement. Could you try that out in this PR?

@k06a
Copy link
Contributor Author

k06a commented Aug 26, 2020

@frangio did, can't make it work with function pointers because of view modifiers.

@frangio
Copy link
Contributor

frangio commented Sep 2, 2020

@k06a This PR is great but you seem to have deleted the natspec documentation. 😄 Let me know if you can add it back, along with similar docs for the new functions, or if you want me to take care of it.

Thank you!

@k06a
Copy link
Contributor Author

k06a commented Sep 3, 2020

@frangio sorry about this, could someone help to restore it? You could simply push your own commits to my branch since I opened PR - it is allowed by github default settings :)

@frangio
Copy link
Contributor

frangio commented Sep 7, 2020

No problem, I'll take care of it.

@frangio frangio merged commit 87326f7 into OpenZeppelin:master Sep 17, 2020
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