Introducing Protocol Interfaces and Implementations of IPAccount, IPAccountRegistry #1
Conversation
- IPAccount - IPAccountRegistry
kingster-will
requested review from
Ramarti,
LeoHChen,
leeren,
jdubpark and
Spablob
kingster-will
changed the title
| require(accessController_ != address(0), "Invalid access controller"); | ||
| require(accessController == address(0), "Already initialized"); |
There was a problem hiding this comment.
I think we should standardize use of custom errors throughout our codebase.
Also I think to replace line 37 it's more semantically clear to do
if (address(this).code.length > 0) {
revert IPAccountImpl_AlreadyInitialized();
}
There was a problem hiding this comment.
I agree, we should move to custom universal errors instead of literal strings.
| /// @param to_ The recipient of the transaction | ||
| /// @param data_ The calldata to check against | ||
| /// @return True if the signer is valid, false otherwise | ||
| function _isValidSigner(address signer_, address to_, bytes calldata data_) internal view returns (bool) { |
There was a problem hiding this comment.
What is the meaning of to_? Is it supposed to be the module address? Given we are not using it right now, I think it would be simpler to just authenticate based on the caller (signer) and function selector.
| /// @param signer_ The signer to check | ||
| /// @param data_ The data to check against | ||
| /// @return The function selector if the signer is valid, 0 otherwise | ||
| function isValidSigner(address signer_, bytes calldata data_) external view returns (bytes4) { |
There was a problem hiding this comment.
I think the semantics of this function are slightly confusing, since for execute the internal _isValidSigner function is used for checking who is authorized to call a specific module.
Whereas for this we are checking if a signer is authorized to act on behalf of the account overall. Unless you are implying that address(0) represents "all modules"? If so could make that clearer
| address internal immutable ERC6551_PUBLIC_REGISTRY; | ||
| address internal immutable ACCESS_CONTROLLER; | ||
|
|
||
| error NonExistIpAccountImpl(); |
There was a problem hiding this comment.
I think we should use the custom errors library again as before
|
|
||
| import { IERC721Receiver } from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; | ||
| import { IERC1155Receiver } from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; | ||
| import { IERC6551Account } from "contracts/interfaces/erc6551/IERC6551Account.sol"; |
There was a problem hiding this comment.
do you want to use the standard reference here? https://github.com/erc6551/reference/tree/main/src/interfaces
This PR introduces interfaces for components of the beta protocol, including IPAccount, IPAccountRegistry, IModuleRegistry, and AccessController.
The PR includes implementations of IPAccount and IPAccountRegistry,
Changes
IPAccount: The IPAccount interface has been introduced. The IPAccountImpl contract provides an implementation of this interface. IPAccount is a token-bound account that adopts the EIP-6551 standard. These accounts are deployed at deterministic addresses through the official 6551 account registry. IPAccount act as core identity for actions.
IPAccountRegistry: The IPAccountRegistry interface and its implementation have been introduced. This contract is responsible for managing the registration and tracking of IP Accounts.
AccessController: The AccessController interface has been introduced, which includes methods for setting and checking access permissions. The implementation will be added by the next PR.
IModuleRegistry: The IModuleRegistry interface has been introduced, which includes methods for registering and getting modules.
Also update the GitHub action workflow and remove unused sample code.
Test
Test cases have been added to ensure the correct functionality of the newly introduced code.