AA-176: Add ERC-165 "supportsInterface" to the EntryPoint #331
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note: due to 'StakeManager', 'NonceManager' being 'abstract' and only parts of the EntryPoint, and due to IEntryPoint inheriting from all interfaces, the implementation looks a little weird. I this this is fine, though.
drortirosh
reviewed
Aug 21, 2023
contracts/interfaces/IEntryPoint.sol
Outdated
| import "./UserOperation.sol"; | ||
| import "./IStakeManager.sol"; | ||
| import "./IAggregator.sol"; | ||
| import "./INonceManager.sol"; | ||
|
|
||
| interface IEntryPoint is IStakeManager, INonceManager { | ||
| interface IEntryPoint is IStakeManager, INonceManager, OpenZeppelin.IERC165 { |
There was a problem hiding this comment.
why should the IEntryPoint inherit from ERC165 ? (you later manually remove it, so maybe it should not be in the interface itself...
There was a problem hiding this comment.
actually, maybe its good, so a bundler can work only with IEntryPoint, not with EntryPoint contract itself.
drortirosh
reviewed
Aug 21, 2023
| ╚══════════════════════════╧════════╝ | ||
|
|
||
| ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗ | ||
| ║ handleOps description │ count │ total gasUsed │ per UserOp gas │ per UserOp overhead ║ | ||
| ║ │ │ │ (delta for │ (compared to ║ | ||
| ║ │ │ │ one UserOp) │ account.exec()) ║ | ||
| ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ | ||
| ║ simple │ 1 │ 81899 │ │ ║ | ||
| ║ simple │ 1 │ 81943 │ │ ║ |
There was a problem hiding this comment.
cost 40 per op? 250 per 10 ?
drortirosh
reviewed
Aug 21, 2023
test/entrypoint.test.ts
Outdated
|
|
||
| describe('ERC-165', function () { | ||
| it('should return true for EntryPoint interface IDs', async function () { | ||
| const epInterface = IEntryPoint__factory.createInterface() |
There was a problem hiding this comment.
I miss a sample for: how a bundler can check the current entryPoint adddress it has is OK ?
drortirosh
reviewed
Aug 21, 2023
test/entrypoint.test.ts
Outdated
| const res1 = await entryPoint.supportsInterface(epInterfaceID) | ||
| const res2 = await entryPoint.supportsInterface(smInterfaceID) | ||
| const res3 = await entryPoint.supportsInterface(nmInterfaceID) | ||
| const res4 = await entryPoint.supportsInterface(epPureInterfaceID) |
There was a problem hiding this comment.
better:
expect(await entryPoint.supoprtsInterface). to.be.true
drortirosh
reviewed
Aug 21, 2023
| @@ -39,6 +39,15 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard | |||
| */ | |||
| uint256 public constant SIG_VALIDATION_FAILED = 1; | |||
|
|
|||
| /// @inheritdoc OpenZeppelin.IERC165 | |||
| function supportsInterface(bytes4 interfaceId) public view virtual override(OpenZeppelin.ERC165, OpenZeppelin.IERC165) returns (bool) { | |||
| return interfaceId == (type(IEntryPoint).interfaceId ^ type(IStakeManager).interfaceId ^ type(INonceManager).interfaceId) || | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
drortirosh
reviewed
Aug 21, 2023
src/Utils.ts
Outdated
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| return iface.getSighash(it.name!) | ||
| }) | ||
| .filter(it => it !== '0x01ffc9a7') // remove the IERC165 method itself |
There was a problem hiding this comment.
why do you exclude it? see comment in the contract.
drortirosh
approved these changes
Aug 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: due to 'StakeManager', 'NonceManager' being 'abstract' and only parts of the EntryPoint, and due to IEntryPoint inheriting from all interfaces, the implementation looks a little weird.
I think this this is fine, though.