Enhance Ethscriptions Metadata Handling and URI Resolution#141
Conversation
- Updated `L2Genesis` to include `NAME_REGISTRY` as a proxied contract. - Enhanced `ERC721EthscriptionsCollection` and `NameRegistry` to include ethscription ID and number in JSON metadata. - Improved URI resolution in tests to ensure consistent hex string formatting for ethscription IDs. - Added new methods in `EthscriptionsApiClient` for fetching single ethscription data and handling errors more gracefully. - Refactored content hash calculation in `EthscriptionsApiClient` for improved accuracy.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR enhances metadata handling for Ethscriptions by adding ethscription ID and number fields to JSON metadata, improving URI resolution consistency, and refining error handling in both contract and API layers.
- Added ethscription ID and number as top-level fields in collection and name registry metadata
- Updated hex string formatting to use explicit 32-byte length for consistent 0x-prefixed output
- Improved error handling for non-existent ethscriptions and API failures
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/storage_reader.rb | Changed error handling to specifically catch contract revert errors and return nil for non-existent ethscriptions instead of generic error handling |
| lib/ethscriptions_api_client.rb | Added fetch_single_ethscription method, changed content hash calculation from SHA256 to keccak256, added l1_block_number field |
| contracts/test/CollectionsManager.t.sol | Added comprehensive merkle proof validation tests for collection access control including non-owner scenarios |
| contracts/test/CollectionURIResolution.t.sol | Fixed hex string conversion to use explicit 32-byte length parameter for consistent formatting |
| contracts/src/libraries/EthscriptionsRendererLib.sol | Updated toHexString calls to use explicit 32-byte length for ethscription IDs and content hashes |
| contracts/src/NameRegistry.sol | Added ethscription_id and ethscription_number as top-level metadata fields, removed _bytes32ToHex helper function |
| contracts/src/ERC721EthscriptionsCollection.sol | Added ethscription_id and ethscription_number as top-level metadata fields in tokenURI |
| contracts/script/L2Genesis.s.sol | Updated to mark NAME_REGISTRY as a proxied contract and adjusted deployment comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Templates and direct deploys live directly at their addresses (no proxy wrapping) | ||
| _setCodeAt(Predeploys.ERC20_FIXED_DENOMINATION_IMPLEMENTATION, "ERC20FixedDenomination"); | ||
| _setCodeAt(Predeploys.ERC721_ETHSCRIPTIONS_COLLECTION_IMPLEMENTATION, "ERC721EthscriptionsCollection"); | ||
| _setCodeAt(Predeploys.NAME_REGISTRY, "NameRegistry"); |
There was a problem hiding this comment.
The NAME_REGISTRY is marked as a proxied contract on line 216, but its implementation code is being set directly at the proxy address using _setCodeAt on line 230. This is inconsistent with other proxied contracts (ERC20_FIXED_DENOMINATION_MANAGER, ERC721_ETHSCRIPTIONS_COLLECTION_MANAGER, ETHSCRIPTIONS_PROVER) which use _setImplementationCodeNamed to set their implementation code at the implementation address (lines 224-226).
This should be changed to:
_setImplementationCodeNamed(Predeploys.NAME_REGISTRY, "NameRegistry");And moved to line 226 (after the ETHSCRIPTIONS_PROVER line) instead of line 230, to group it with the other proxied contracts.
| // Templates and direct deploys live directly at their addresses (no proxy wrapping) | |
| _setCodeAt(Predeploys.ERC20_FIXED_DENOMINATION_IMPLEMENTATION, "ERC20FixedDenomination"); | |
| _setCodeAt(Predeploys.ERC721_ETHSCRIPTIONS_COLLECTION_IMPLEMENTATION, "ERC721EthscriptionsCollection"); | |
| _setCodeAt(Predeploys.NAME_REGISTRY, "NameRegistry"); | |
| _setImplementationCodeNamed(Predeploys.NAME_REGISTRY, "NameRegistry"); | |
| // Templates and direct deploys live directly at their addresses (no proxy wrapping) | |
| _setCodeAt(Predeploys.ERC20_FIXED_DENOMINATION_IMPLEMENTATION, "ERC20FixedDenomination"); | |
| _setCodeAt(Predeploys.ERC721_ETHSCRIPTIONS_COLLECTION_IMPLEMENTATION, "ERC721EthscriptionsCollection"); |
| begin | ||
| data = fetch_json(path) | ||
| # The show endpoint returns the ethscription directly | ||
| normalize_creations([data['result']]).first | ||
| rescue HttpError => e | ||
| if e.code == 404 | ||
| # Ethscription doesn't exist | ||
| nil | ||
| else | ||
| # Re-raise other HTTP errors for retry handling | ||
| raise | ||
| end | ||
| end |
There was a problem hiding this comment.
The 404 error handling may cause unnecessary retries. When fetch_json returns a 404 HttpError, the Retriable.retriable block will retry it multiple times (since HttpError is in the retry list), and only after exhausting retries will it be caught on line 59-62 and return nil.
For 404 errors (which indicate the ethscription doesn't exist), retrying is unnecessary and wasteful. Consider handling 404s before they trigger the retry logic, or excluding 404 errors from the retry mechanism in fetch_json.
- Added `NAME_REGISTRY` to the implementation code in `L2Genesis`. - Adjusted comments for clarity regarding template deployments and proxy usage. - Ensured consistency in the handling of contract implementations within the genesis setup.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
L2Genesisto includeNAME_REGISTRYas a proxied contract.ERC721EthscriptionsCollectionandNameRegistryto include ethscription ID and number in JSON metadata.EthscriptionsApiClientfor fetching single ethscription data and handling errors more gracefully.EthscriptionsApiClientfor improved accuracy.Note
Adds proxied
NAME_REGISTRY, includes ethscription ID/number in token metadata with fixed 32-byte hex, introduces Merkle allowlist tests, and improves API client/storage reader behavior.Predeploys.NAME_REGISTRYas a proxied contract; set implementation via_setImplementationCodeNamed.ERC721EthscriptionsCollection.tokenURIandNameRegistry.tokenURInow includeethscription_id(0x + 32-byte hex) and numericethscription_number.EthscriptionsRendererLibforethscriptionIdandcontentHash.esc://references.CollectionsManager.t.sol.EthscriptionsApiClient: addfetch_single_ethscription; computecontent_hashvia keccak256; exposel1_block_number; robust error handling.StorageReader: return nil on contract reverts for missing ethscription; use includeContent=false path with graceful handling.Written by Cursor Bugbot for commit 9e9b9c8. This will update automatically on new commits. Configure here.