-
Notifications
You must be signed in to change notification settings - Fork 25
Enhance ERC721 Contracts with Base64 Metadata Encoding #142
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
Conversation
- Updated `ERC721EthscriptionsCollection` to return Base64-encoded JSON for metadata. - Refactored `NameRegistry` to include a new `contractURI` method for OpenSea collection-level metadata. - Implemented transfer and approval functions to revert transfers, enhancing security. - Updated tests to validate the new Base64 encoding and ensure correct URI handling.
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.
Pull Request Overview
This PR enhances ERC721 contracts by implementing Base64-encoded JSON for metadata URIs, improving compatibility with NFT marketplaces like OpenSea. The changes also strengthen security by adding explicit transfer and approval blocking functions.
- Updated both
ERC721EthscriptionsCollectionandNameRegistryto return Base64-encoded data URIs for token metadata - Added
contractURI()methods for OpenSea collection-level metadata support - Implemented comprehensive transfer and approval blocking to prevent unauthorized token movements
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| contracts/test/CollectionURIResolution.t.sol | Updated tests to validate Base64-encoded contractURI, including proper decoding and content verification |
| contracts/src/NameRegistry.sol | Added contractURI method, implemented transfer/approval blocking, and switched from Strings to LibString; changed protocolName from function to constant |
| contracts/src/ERC721EthscriptionsCollection.sol | Modified tokenURI to return Base64-encoded JSON instead of plain JSON |
Comments suppressed due to low confidence (1)
contracts/src/ERC721EthscriptionsCollection.sol:194
- Missing override for
safeTransferFrom(address, address, uint256)(the overload without thebytes memoryparameter). The IERC721 interface defines two safeTransferFrom functions, and both should be overridden to maintain consistency with the NameRegistry implementation and ensure proper interface conformance.
Add:
function safeTransferFrom(address, address, uint256)
public
pure
override(ERC721EthscriptionsUpgradeable, IERC721)
{
revert TransferNotAllowed();
} function safeTransferFrom(address, address, uint256, bytes memory)
public
pure
override(ERC721EthscriptionsUpgradeable, IERC721)
{
revert TransferNotAllowed();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Ethscriptions public constant ethscriptions = Ethscriptions(Predeploys.ETHSCRIPTIONS); | ||
|
|
||
| string public constant PROTOCOL_NAME = "word-domains"; | ||
| string public constant protocolName = "word-domains"; |
Copilot
AI
Nov 10, 2025
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.
Changing protocolName from a function to a public constant breaks the IProtocolHandler interface requirement. The interface defines protocolName() as a function that must be implemented. This will cause the contract to fail compilation or break interface compatibility.
Recommendation: Keep protocolName() as a function returning the constant string, or modify the interface if changing it is intentional.
| string public constant protocolName = "word-domains"; | |
| function protocolName() public pure override returns (string memory) { | |
| return "word-domains"; | |
| } |
- Added `transfer_ownership` and `renounce_ownership` operations to the `Erc721EthscriptionsCollectionParser` for managing collection ownership. - Introduced corresponding methods in the `ERC721EthscriptionsCollectionManager` to handle ownership transfers and renouncements, including validation for zero addresses. - Updated tests to cover new ownership functionalities, ensuring correct encoding and behavior for both operations. - Enhanced error handling for invalid ownership transfer attempts.
| end | ||
|
|
||
| value.downcase | ||
| end |
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.
Bug: Case Sensitivity Breaks Address Validation
The validate_address method rejects valid EIP-55 checksummed Ethereum addresses containing uppercase hex characters. The regex pattern /\A0x[0-9a-f]{40}\z/ only accepts lowercase hex, but Ethereum addresses are case-insensitive and often use mixed-case checksums. Users providing checksummed addresses (e.g., 0xAbCd...) will encounter validation errors even though these addresses are valid. The method already downcases the result, so accepting mixed case input wouldn't affect the output.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ',"', | ||
| mediaType, | ||
| '":"', | ||
| mediaUri, |
Copilot
AI
Nov 12, 2025
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.
The mediaUri should be escaped with .escapeJSON() to prevent JSON injection attacks. Unlike other URIs that are resolved/processed, this is directly embedded in the JSON output without escaping. If the media URI contains characters like quotes or backslashes, it could break the JSON structure.
| mediaUri, | |
| mediaUri.escapeJSON(), |
| mediaType, | ||
| '":"', | ||
| mediaUri.escapeJSON(), | ||
| mediaUri, |
Copilot
AI
Nov 12, 2025
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.
The mediaUri is not being escaped with .escapeJSON() despite being inserted into a JSON string. This is inconsistent with line 127 (removed) which had proper escaping. This could lead to JSON injection if the media URI contains special characters like quotes or backslashes.
| mediaUri, | |
| mediaUri.escapeJSON(), |
ERC721EthscriptionsCollectionto return Base64-encoded JSON for metadata.NameRegistryto include a newcontractURImethod for OpenSea collection-level metadata.Note
Adds collection ownership transfer/renounce operations, serves Base64-encoded metadata (with URI resolution), refactors NameRegistry metadata and transfer blocking, and updates parser/tests accordingly.
transfer_ownershipandrenounce_ownershipops,_transferCollectionOwnership, andOwnershipTransferredevent inERC721EthscriptionsCollectionManager; remove implicit owner change on leader ethscription transfer.address, non-zero), value builders, and encoding paths.ERC721EthscriptionsCollection.contractURI()now returns Base64-encoded JSON and resolvesesc://ethscriptions/{id}/data;tokenURIuses rawmediaUri.tokenURI, newcontractURI(), escaped JSON fields, andprotocolNameconstant.ERC721EthscriptionsCollectionandNameRegistry.esc://handling).ProtocolParserspecs and expanded collection parser specs for new ops/validation.Written by Cursor Bugbot for commit 64230ae. This will update automatically on new commits. Configure here.