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

Rename _msgSender #280

Merged
merged 2 commits into from
May 14, 2022
Merged

Conversation

Vectorized
Copy link
Collaborator

@Vectorized Vectorized commented May 14, 2022

_msgSender() is used for writing GSN compatible contracts.

When users import OZ and inherit from their contracts (e.g. Ownable), our _msgSender() will conflict with their definition.

I suggest to keep an overrideable function, with a different name, such that it won't conflict.

So now, when writing GSN compatible contracts with OZ, one just have to override new function to return _msgSender().

Vectorized referenced this pull request May 14, 2022
* Remove OpenZeppelin

* Add tests

* Add suggested changes
@Vectorized Vectorized requested a review from cygaar May 14, 2022 08:52
@Vectorized
Copy link
Collaborator Author

Vectorized commented May 14, 2022

On the same note, I'm also thinking of renaming _toString() to something else, since it is a very common name that may cause conflict, and not really related to the core logic of ERC721A.

@Vectorized
Copy link
Collaborator Author

Vectorized commented May 14, 2022

I merge this first in case people are using forge to install (which doesn't pull from npm, but rather github).

Most people will use Ownable but not GSN, so this PR is quite urgent for developer Quality of Life.

Meanwhile, there is a poll on the name at https://twitter.com/optimizoor/status/1525402330900086784

@Vectorized Vectorized merged commit 45454d6 into chiru-labs:main May 14, 2022
@Vectorized Vectorized deleted the feature/removeContext branch May 17, 2022 04:38
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.

1 participant