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

AA-176: Add ERC-165 "supportsInterface" to the EntryPoint #331

Merged
merged 7 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion contracts/core/EntryPoint.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
* Account-Abstraction (EIP-4337) singleton EntryPoint implementation.
* Only one instance required on each chain.
*/
contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard {
contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, OpenZeppelin.ERC165 {

using UserOperationLib for UserOperation;

Expand All @@ -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.

interfaceId == type(IEntryPoint).interfaceId ||
interfaceId == type(IStakeManager).interfaceId ||
interfaceId == type(INonceManager).interfaceId ||
super.supportsInterface(interfaceId);
}

/**
* Compensate the caller's beneficiary address with the collected fees of all UserOperations.
* @param beneficiary - The address to receive the fees.
Expand Down
5 changes: 4 additions & 1 deletion contracts/interfaces/IEntryPoint.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@ pragma solidity ^0.8.12;
/* solhint-disable no-inline-assembly */
/* solhint-disable reason-string */

// we also require '@gnosis.pm/safe-contracts' and both libraries have 'IERC165.sol', leading to conflicts
import "@openzeppelin/contracts/utils/introspection/ERC165.sol" as OpenZeppelin;

import "./UserOperation.sol";
import "./IStakeManager.sol";
import "./IAggregator.sol";
import "./INonceManager.sol";

interface IEntryPoint is IStakeManager, INonceManager {
interface IEntryPoint is IStakeManager, INonceManager, OpenZeppelin.IERC165 {
Copy link
Contributor

Choose a reason for hiding this comment

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

why should the IEntryPoint inherit from ERC165 ? (you later manually remove it, so maybe it should not be in the interface itself...

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, maybe its good, so a bundler can work only with IEntryPoint, not with EntryPoint contract itself.

/***
* An event emitted after each successful request.
* @param userOpHash - Unique identifier for the request (hash its entire content, except signature).
Expand Down
26 changes: 13 additions & 13 deletions reports/gas-checker.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,36 @@
╔══════════════════════════╤════════╗
║ gas estimate "simple" │ 29014 ║
╟──────────────────────────┼────────╢
║ gas estimate "big tx 5k" │ 125260
║ gas estimate "big tx 5k" │ 125248
╚══════════════════════════╧════════╝

╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗
║ 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 │ │ ║
Copy link
Contributor

Choose a reason for hiding this comment

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

cost 40 per op? 250 per 10 ?

╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 2 │ │ 4418615172
║ simple - diff from previous │ 2 │ │ 4420815194
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 10 │ 479702 │ │ ║
║ simple │ 10 │ 479944 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 11 │ │ 4422215208
║ simple - diff from previous │ 11 │ │ 4428015266
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 1 │ 88182 │ │ ║
║ simple paymaster │ 1 │ 88202 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 2 │ │ 4317514161
║ simple paymaster with diff │ 2 │ │ 4319714183
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 10 │ 476938 │ │ ║
║ simple paymaster │ 10 │ 477132 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 11 │ │ 4323414220
║ simple paymaster with diff │ 11 │ │ 4320814194
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 1 │ 182932 │ │ ║
║ big tx 5k │ 1 │ 182976 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 2 │ │ 14472119461
║ big tx - diff from previous │ 2 │ │ 14470719459
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 10 │ 1485370 │ │ ║
║ big tx 5k │ 10 │ 1485552 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 11 │ │ 14471019450
║ big tx - diff from previous │ 11 │ │ 14474419496
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝

17 changes: 17 additions & 0 deletions src/Utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Interface, JsonFragment } from '@ethersproject/abi'

export function getERC165InterfaceID (abi: JsonFragment[]): string {
let interfaceId =
abi
.filter(it => it.type === 'function' && it.name != null)
.map(it => {
const iface = new Interface([it])
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return iface.getSighash(it.name!)
})
.filter(it => it !== '0x01ffc9a7') // remove the IERC165 method itself
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you exclude it? see comment in the contract.

.map((x) => parseInt(x, 16))
.reduce((x, y) => x ^ y)
interfaceId = interfaceId > 0 ? interfaceId : 0xFFFFFFFF + interfaceId + 1
return '0x' + interfaceId.toString(16).padStart(8, '0')
}
43 changes: 43 additions & 0 deletions test/entrypoint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import {
TestSignatureAggregator__factory,
MaliciousAccount__factory,
TestWarmColdAccount__factory,
IEntryPoint__factory,
SimpleAccountFactory__factory,
IStakeManager__factory,
INonceManager__factory,
TestPaymasterRevertCustomError__factory
} from '../typechain'
import {
Expand Down Expand Up @@ -53,6 +57,7 @@ import { arrayify, defaultAbiCoder, hexConcat, hexZeroPad, parseEther } from 'et
import { debugTransaction } from './debugTx'
import { BytesLike } from '@ethersproject/bytes'
import { toChecksumAddress } from 'ethereumjs-util'
import { getERC165InterfaceID } from '../src/Utils'

describe('EntryPoint', function () {
let entryPoint: EntryPoint
Expand Down Expand Up @@ -1362,4 +1367,42 @@ describe('EntryPoint', function () {
})
})
})

describe('ERC-165', function () {
it('should return true for EntryPoint interface IDs', async function () {
const epInterface = IEntryPoint__factory.createInterface()
Copy link
Contributor

Choose a reason for hiding this comment

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

I miss a sample for: how a bundler can check the current entryPoint adddress it has is OK ?

const smInterface = IStakeManager__factory.createInterface()
const nmInterface = INonceManager__factory.createInterface()
// note: manually generating "pure", solidity-like "type(IEntryPoint).interfaceId" without inherited methods
const epPureInterfaceID = getERC165InterfaceID([
...epInterface.fragments.filter(it => [
'handleOps',
'handleAggregatedOps',
'getUserOpHash',
'getSenderAddress',
'simulateValidation',
'simulateHandleOp'
].includes(it.name))
])
const epInterfaceID = getERC165InterfaceID([...epInterface.fragments])
const smInterfaceID = getERC165InterfaceID([...smInterface.fragments])
const nmInterfaceID = getERC165InterfaceID([...nmInterface.fragments])
const res1 = await entryPoint.supportsInterface(epInterfaceID)
const res2 = await entryPoint.supportsInterface(smInterfaceID)
const res3 = await entryPoint.supportsInterface(nmInterfaceID)
const res4 = await entryPoint.supportsInterface(epPureInterfaceID)
Copy link
Contributor

Choose a reason for hiding this comment

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

better:

expect(await entryPoint.supoprtsInterface). to.be.true

expect(res1).to.equal(true)
expect(res2).to.equal(true)
expect(res3).to.equal(true)
expect(res4).to.equal(true)
})

it('should return false for a wrong interface', async function () {
const saInterface = SimpleAccountFactory__factory.createInterface()
const entryPointInterfaceID = getERC165InterfaceID([...saInterface.fragments])
const res = await entryPoint.supportsInterface(entryPointInterfaceID)
expect(res)
.to.equal(false)
})
})
})