-
Notifications
You must be signed in to change notification settings - Fork 614
Create IHookMetadata #382
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
base: main
Are you sure you want to change the base?
Create IHookMetadata #382
Conversation
This Pull Request proposes adding a new interface, IHookMetadata, to the Uniswap V4 Periphery repository. The IHookMetadata interface is designed to provide standardized metadata for hooks, which include information about the audit details and additional metadata. By integrating IHookMetadata, we can create a consistent and transparent way to describe hooks, enhancing both the security and the user experience for developers interacting with Uniswap hooks. Details The IHookMetadata interface introduces the following key features: Audit Metadata: Provides comprehensive information regarding audits conducted on the smart contract. AuditSummary struct: Contains details such as the auditor's name, the issued timestamp, hashes of the code and audit document, and an audit URI. Auditor struct: Specifies the audit firm's name, additional URI information, and a list of authors involved in the audit. EIP-712 Domain Information: The EIP712Domain struct is included to describe the domain for which signatures are generated. This is critical for verifying off-chain signed messages, ensuring consistency and integrity across the Uniswap ecosystem. Signature Standards: The SignatureType enum defines multiple signature standards supported, including SECP256K1, BLS, ERC1271, and SECP256R1, which allows the flexibility to adopt different signature approaches as needed by various protocols. Functions for Metadata Retrieval: The interface provides several functions for easy retrieval of metadata, such as: name(), repository(), logoURI(), and description(): Functions that provide basic descriptive information about the hook. auditSummary(), eip712Domain(), and signedAuditSummary(): Functions that provide crucial audit and signing information. By adding IHookMetadata, this interface aims to improve trust in the Uniswap ecosystem through more accessible audit data and standardized metadata for hooks. It would ensure a consistent method of obtaining and verifying metadata and signature information for any hooks that integrate with Uniswap V4. Motivation Transparency and Trust: Increasing transparency about hook audits and other metadata contributes to improved trust in the Uniswap ecosystem. Standardization: A standardized interface for audit information reduces friction for developers seeking to integrate their hooks. Security: Leveraging the EIP-712 domain and supporting multiple signature types strengthens the ability to verify transactions and audit reports effectively. Implementation Notes This interface is defined to be implemented by any hook used within Uniswap, providing a framework for uniform metadata collection. Adopting IHookMetadata across hooks will help developers better understand, verify, and use hooks while ensuring they meet established standards. Conclusion This PR aims to improve the metadata handling, transparency, and security of hooks in Uniswap V4 by integrating IHookMetadata. We believe this will benefit developers, users, and auditors by providing a clear, accessible, and standardized set of metadata for hooks. Please let me know if there are any questions or further improvements needed for this interface.
|
@RostyslavBortman can you give it a filename hah |
src/interfaces/IHookMetadata.sol
Outdated
| Auditor auditor; // The auditor who performed the audit | ||
| uint256 issuedAt; // The timestamp at which the audit was issued | ||
| uint256[] ercs; // List of ERC standards that were covered in the audit | ||
| bytes32 codeHash; // Hash of the audited smart contract code |
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 should be more specific in the comment - is this the github commit? the bytecode hash? the initcode hash?
src/interfaces/IHookMetadata.sol
Outdated
| } | ||
|
|
||
| // Struct representing the EIP712 domain, which is used for signatures | ||
| struct EIP712Domain { |
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.
is there a reason not to pre-define the eip712 typehash of what the signature should be of?
src/interfaces/IHookMetadata.sol
Outdated
|
|
||
| function name() external view returns (string memory); // Returns the name of the hook | ||
| function repository() external view returns (string memory); // Returns the repository URI for the smart contract code | ||
| function logoURI() external view returns (string memory); // Returns the URI for the hook's logo |
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.
thoughts on adding a field for the hook dev to provide a URL to a website/docs or something about their hook?
src/interfaces/IHookMetadata.sol
Outdated
| Auditor auditor; | ||
| uint256 issuedAt; | ||
| uint256[] ercs; | ||
| bytes32 bytecodeHash; |
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 do wonder how practical a bytecodeHash is in practice? It changes even you change the optimiser runs, sometimes if you the deploy parameters, and other very small tweaks - and because its not reversible it might become impossible to see what someone actually audited?
If you have a bytecodeHash from an auditor... how are you meant to find the version of the code that they audited?
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.
Just thinking out loud here, it still might be the best option. But just wondering if youve thought this through
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.
So I think we are talking about the next options here:
-
commit hash - I don't this reasonable because someone can actually change the code during the deployment and you have to read an actual contract code anyway. But would love to hear your opinion;
-
bytecodeHash - I think we can also ask devs to deploy with some specific params for optimiser, etc, to be able to allow onchain audit verification. As for deploy parameters - I think we can focus only on the code itself without constructor params;
how are you meant to find the version of the code that they audited?
So, I think the regular flow - they have to have a link to the repo on their website, commit hash, etc. But as for the onchain verification I see bytecodeHash as only one possible option
src/interfaces/IHookMetadata.sol
Outdated
| /// @param signatureType The type of the signature (e.g., SECP256K1, BLS, etc.). | ||
| /// @param data The actual signature data. | ||
| struct Signature { | ||
| SignatureType signatureType; |
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.
What was the motivation for removing the EIP712 stuff? Happy either way just interested :)
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.
So I think we want to provide more flexibility in terms of what should be inside of hash for the signature since we can't define function eip712Domain specifically in the interface
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.
One of the main benefits of EIP712 domain signatures would be for DeFi protocols to prevent replay attacks on their signed limit orders using the chain id, contract address, etc... as parameters. In this case there is nothing anyone can do with a signed audit payload so it doesn't need that extra security. I do think a domain separator could add some specification around what is being signed here but it's not necessary
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.
Yes, I agree. + we removed it to provide more flexibility actually
fix: IHookMetadata auditSummaries + eip domen
HookMetadata
HookMetadata moved to utils
calldata changed to memory
Fixed method signature to match others
| /// @notice An internal method that registers a new signed audit summury and emits an event that may be useful for | ||
| /// external indexing services to discover and display essential information about a Uniswap V4 hook. | ||
| /// @dev This internal method should be called in the child hook contract whenever new audit summary is registered | ||
| /// (for example, in the constructor or in the custom owner/admin/DAO controlled method). | ||
| /// @param signedAuditSummary A new signed audit summury to register. | ||
| /// @return A new signed audit summary ID. | ||
| function _registerAuditSummary(SignedAuditSummary memory signedAuditSummary) internal returns (uint256) { |
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.
not sure if I'm in love with the idea of adding an audit after audits, is there a reason why audit information cannot be provided at the time of deployment (via constructor)?
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.
@saucepoint There are a few reasons why we came out with this solution:
- a contract may be audited a few times and have a few audit results from different auditors.
- we need a
bytecodeHashto store it in the contract with the audit results. To obtain a bytecode we need to deploy a contract.
| * 1. Required Metadata | ||
| * - Every hook must implement: name(), repository(), logoURI(), websiteURI(), | ||
| * description(), version(). | ||
| * - These fields are effectively immutable and indexed at deployment time. |
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.
what does "effectively" immutable mean? When can they be changed?
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.
We assume that after deploy they will not be changed
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.
They are not actually changeable in the contracts though no? So I think you can just declare them to be immutable
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.
@snreynolds Unfortunately, it is impossible to declare string variables as immutable in Solidity at this moment.
From the documentation:
Not all types for constants and immutables are implemented at this time. The only supported types are strings (only for constants) and value types.
However, there is one workaround we can implement instead: put all 6 variables in the HookMetadata storage, mark them as private (not immutable, as this is impossible) and allow them to be set up through the constructor only. No setters will be provided, only getters that will allow reading these state variables from outside or child contracts.
What do you think about this approach?
| * emit the AuditSummaryRegistered event for indexers. | ||
| * | ||
| * 3. EIP-712 Domain Information | ||
| * - The contract must provide an EIP-712 DOMAIN_SEPARATOR to allow auditors |
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.
Is there a standard format you'd want to enforce in the DOMAIN_SEPARATOR? Could give a recommendation for which fields should/must be specified. ie Im guessing the verifyingContract is the hook but maybe worth specifying?
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.
We created a template for IHookMetadata usage - uniswapfoundation/v4-template#58, used https://github.com/uniswap/v4-periphery/blob/ea2bf2e1ba6863bb809fc2ff791744f308c4a26d/src/base/EIP712_v4.sol there
| * summaries and their retrivals using internal counting mechanism. | ||
| */ | ||
| abstract contract HookMetadata is IHookMetadata { | ||
| mapping(uint256 auditId => SignedAuditSummary signedAuditSummary) private signedAuditsSummaries; |
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.
Is there a reason to store on chain? If offchain indexing services are already indexing against the registration, whats the reason to also store on chain?
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.
Good question. The primary reason for storing the audit summary on-chain is to ensure immutability and verifiable proof that an audit actually occurred. By storing the audit data, including the auditor’s signature, directly on-chain, we guarantee that nobody can modify the audit content after submission.
While off-chain indexing services can indeed store a copy of the audit, they can't provide the same cryptographic guarantees of immutability. Additionally, by having an on-chain registry of trusted auditor addresses, anyone can verify directly on-chain whether a hook was audited by a trusted auditor or not.
It's important to note that builders aren't forced to submit audits on-chain—it's an optional but recommended approach if verifiable audit proof is desired directly within the onchain environment.
This Pull Request proposes adding a new interface, IHookMetadata, to the Uniswap V4 Periphery repository. The IHookMetadata interface is designed to provide standardized metadata for hooks, which include information about the audit details and additional metadata. By integrating IHookMetadata, we can create a consistent and transparent way to describe hooks, enhancing both the security and the user experience for developers interacting with Uniswap hooks.
The IHookMetadata interface introduces the following key features:
Audit Metadata: Provides comprehensive information regarding audits conducted on the smart contract.
AuditSummary struct: Contains details such as the auditor's name, the issued timestamp, hashes of the code and audit document, and an audit URI.
Auditor struct: Specifies the audit firm's name, additional URI information, and a list of authors involved in the audit.
EIP-712 Domain Information
The EIP712Domain struct is included to describe the domain for which signatures are generated. This is critical for verifying off-chain signed messages, ensuring consistency and integrity across the Uniswap ecosystem.
Signature Standards:
The SignatureType enum defines multiple signature standards supported, including SECP256K1, BLS, ERC1271, and SECP256R1, which allows the flexibility to adopt different signature approaches as needed by various protocols.
Functions for Metadata Retrieval:
The interface provides several functions for easy retrieval of metadata, such as: name, repository, logoURI, and description: Functions that provide basic descriptive information about the hook.
auditSummary, eip712Domain, and signedAuditSummary: Functions that provide crucial audit and signing information.
By adding IHookMetadata, this interface aims to improve trust in the Uniswap ecosystem through more accessible audit data and standardized metadata for hooks. It would ensure a consistent method of obtaining and verifying metadata and signature information for any hooks that integrate with Uniswap V4.
Motivation
Transparency and Trust: Increasing transparency about hook audits and other metadata contributes to improved trust in the Uniswap ecosystem.
Standardization: A standardized interface for audit information reduces friction for developers seeking to integrate their hooks.
Implementation Notes
This interface is defined to be implemented by any hook used within Uniswap, providing a framework for uniform metadata collection.
Adopting IHookMetadata across hooks will help developers better understand, verify, and use hooks while ensuring they meet established standards.
Conclusion
This PR aims to improve the metadata handling, transparency, and security of hooks in Uniswap V4 by integrating IHookMetadata. We believe this will benefit developers, users, and auditors by providing a clear, accessible, and standardized set of metadata for hooks.
Please let me know if there are any questions or further improvements needed for this interface.