-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Refactor ERC165 to use function overriding instead of storage. #2505
Conversation
cdcc30e
to
0604e82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great.
test/introspection/ERC165.test.js
Outdated
@@ -9,6 +9,12 @@ contract('ERC165', function (accounts) { | |||
this.mock = await ERC165Mock.new(); | |||
}); | |||
|
|||
it('register interface', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test should be removed since it pertains to ERC165Storage
and not plain ERC165
.
contracts/mocks/ERC165Mock.sol
Outdated
@@ -2,9 +2,9 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this entire file can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then ERC165Storage
should not be abstract, right ?
I've implemented the changes I suggested above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ERC165: Uses virtual/override on the supportsInterface function instead of mapping
This reduces the storage footprint of ERC165 compliant contract, thus reducing the gas cost of deploying and configuring such contracts.
PR Checklist