Refactor Protocol Parsing and Introduce Word Domains Support#140
Conversation
- Updated `ProtocolParser` to streamline extraction of parameters for various protocols, including ERC-20 and ERC-721. - Introduced `WordDomainsParser` for handling legacy word-domain registrations, supporting operations like `register` and `set_primary`. - Enhanced the `Erc20FixedDenominationParser` and `Erc721EthscriptionsCollectionParser` to utilize the new unified parsing approach. - Adjusted ABI encoding for operations to accommodate changes in parameter structures. - Improved test coverage for new parsing functionalities and ensured compatibility across different protocols.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a unified protocol parsing system with support for word-domains (legacy word-only domain inscriptions), refactors the existing ERC-20 and ERC-721 parsers to use a common interface, and adds ERC-721 collection support for fixed denomination tokens. The changes also include updates to contract event signatures, pagination limits, and test coverage.
Key Changes:
- Unified protocol parser interface with consistent
validate_and_encodeAPI across all parsers - New word-domains protocol parser for dotless domain registrations
- ERC-20 fixed denomination tokens now have associated ERC-721 collections for individual notes
- Updated event signatures to include
mintIdparameter for better tracking - Added
contentHashfield to collection items for content verification
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
spec/models/word_domains_parser_spec.rb |
New test suite for word-domains protocol parser |
spec/models/ethscription_transaction_builder_spec.rb |
Updated to use unified ProtocolParser interface |
spec/models/erc721_ethscriptions_collection_parser_spec.rb |
Updated tests for new collection item structure with contentHash |
spec/models/erc721_collections_import_fallback_spec.rb |
Updated tests for import fallback with contentHash support |
spec/models/erc20_fixed_denomination_parser_spec.rb |
Refactored tests to use ProtocolParser interface |
spec/integration/tokens_protocol_spec.rb |
Updated integration test for new parser interface |
lib/protocol_event_reader.rb |
Added ERC721CollectionDeployed event and updated event signatures with mintId |
contracts/test/TokenURIGas.t.sol |
New gas benchmark test for tokenURI scaling |
contracts/test/NameRegistry.t.sol |
New test suite for name registry functionality |
contracts/test/EthscriptionsToken.t.sol |
Added tests for ERC-721 collection support in fixed denomination tokens |
contracts/test/CollectionsManager.t.sol |
Updated tests to include contentHash in item data |
contracts/src/libraries/Predeploys.sol |
Added NAME_REGISTRY predeploy address |
contracts/src/NameRegistry.sol |
New contract implementing word-domains protocol |
contracts/src/Ethscriptions.sol |
Reduced pagination limits for performance |
contracts/src/ERC721EthscriptionsCollectionManager.sol |
Added contentHash field to ItemData and verification logic |
contracts/src/ERC721EthscriptionsCollection.sol |
Updated initialization to store collectionId locally and handle zero-address minting |
contracts/src/ERC20FixedDenominationManager.sol |
Added ERC-721 collection deployment and management for fixed denomination tokens |
contracts/script/L2Genesis.s.sol |
Added NameRegistry deployment and protocol registration |
app/models/word_domains_parser.rb |
New Ruby parser for word-domains protocol |
app/models/protocol_parser.rb |
Refactored to unified protocol parsing system with consistent interface |
app/models/erc721_ethscriptions_collection_parser.rb |
Updated to new interface with contentHash support |
app/models/erc20_fixed_denomination_parser.rb |
Refactored to simplified interface with tuple encoding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| REGISTER_OP = 'register'.b | ||
| SET_PRIMARY_OP = 'set_primary'.b | ||
|
|
||
| NAME_REGEX = /\A[a-z0-9_]{1,30}\z/.freeze |
There was a problem hiding this comment.
The comment on line 21 in contracts/src/NameRegistry.sol defines MAX_LENGTH = 31, but the Ruby parser regex allows only 1-30 characters ({1,30}). This inconsistency between the contract constant (31) and the parser validation (30) could lead to rejected valid inscriptions. Either the contract constant should be 30, or the regex should be {1,31}.
| NAME_REGEX = /\A[a-z0-9_]{1,30}\z/.freeze | |
| NAME_REGEX = /\A[a-z0-9_]{1,31}\z/.freeze |
| CollectionRecord storage collection = collectionStore[collectionId]; | ||
| Ethscriptions.Ethscription memory ethscription = ethscriptions.getEthscription(ethscriptionId, false); | ||
|
|
||
| require(ethscription.contentHash == item.contentHash, "Content hash mismatch"); |
There was a problem hiding this comment.
The error message 'Content hash mismatch' is unclear about what failed. Consider providing more context such as 'Item content hash does not match ethscription content' to help users understand that the hash they provided in the item data must match the actual ethscription content hash.
| require(ethscription.contentHash == item.contentHash, "Content hash mismatch"); | |
| require(ethscription.contentHash == item.contentHash, "Item content hash does not match ethscription content"); |
| function _getContractSalt(string memory tick, string memory contractType) private pure returns (bytes32) { | ||
| return keccak256(abi.encode(tick, contractType)); |
There was a problem hiding this comment.
The salt computation uses only tick and contractType without including any protocol-specific identifier. This could potentially allow collisions if other protocols or contracts use the same tick names with CREATE2. Consider including the protocol name or contract address in the salt computation: keccak256(abi.encode(protocolName, tick, contractType)).
| function _getContractSalt(string memory tick, string memory contractType) private pure returns (bytes32) { | |
| return keccak256(abi.encode(tick, contractType)); | |
| function _getContractSalt(string memory tick, string memory contractType) private view returns (bytes32) { | |
| return keccak256(abi.encode(address(this), tick, contractType)); |
| hash = Eth::Util.keccak256(decoded_content).unpack1('H*') | ||
| content_hash = '0x' + hash |
There was a problem hiding this comment.
The content hash computation is duplicated in three places: lines 156-157, lines 219-220, and used again in the build_item_object method. Consider extracting this into a private helper method like compute_content_hash(decoded_content) to reduce duplication and ensure consistency.
| mapping(string => TokenInfo) internal tokensByTick; | ||
| mapping(bytes32 => string) internal deployToTick; // deployEthscriptionId => tick |
There was a problem hiding this comment.
Using string as a mapping key in tokensByTick can be gas-inefficient for lookups. Consider using bytes32 (via keccak256(bytes(tick))) as the key instead, similar to how the old implementation used _getTickKey. This would make lookups more gas-efficient while maintaining uniqueness.
| @@ -115,8 +115,8 @@ contract Ethscriptions is ERC721EthscriptionsSequentialEnumerableUpgradeable { | |||
| // ============================================================= | |||
|
|
|||
| /// @dev Maximum page sizes for pagination helpers | |||
There was a problem hiding this comment.
The pagination limits were reduced from 50/1000 to 20/50 without any explanatory comment. Consider adding a comment explaining why these limits were reduced (e.g., 'Reduced to prevent excessive gas consumption in view functions' or 'Optimized based on gas benchmarks').
| /// @dev Maximum page sizes for pagination helpers | |
| /// @dev Maximum page sizes for pagination helpers | |
| // Reduced from 50/1000 to 20/50 to prevent excessive gas consumption in view functions and optimize for gas usage based on benchmarks. |
|
|
||
| # For plain word-domains: must have blank mimetype and no parameters | ||
| # Also reject empty content (like 'data:,') | ||
| return nil if decoded_content.strip.empty? |
There was a problem hiding this comment.
Using strip.empty? for validation could incorrectly reject valid word domains that consist only of whitespace characters if they passed through the regex validation. However, the NAME_REGEX in WordDomainsParser only allows [a-z0-9_], so whitespace-only names would fail anyway. Consider using empty? directly instead of strip.empty? for clarity, or add a comment explaining the intent.
| return nil if decoded_content.strip.empty? | |
| return nil if decoded_content.empty? |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… to simplify domain length handling.
|
bugbot run |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 22 out of 22 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.
| item.description, | ||
| item.attributes | ||
| )); | ||
|
|
There was a problem hiding this comment.
Bug: Merkle Proofs: Backwards Incompatibility
The Merkle proof verification now includes contentHash as the first field in the leaf computation, but this breaks compatibility with existing Merkle trees. Any Merkle proofs generated before this change (which didn't include contentHash) will fail validation even if they were valid before. This creates a backwards incompatibility where previously valid proofs become invalid after deployment.
| function _getTickKey(string memory tick) private pure returns (bytes32) { | ||
| return keccak256(abi.encode(protocolName, tick)); | ||
| function _getContractSalt(string memory tick, string memory contractType) private pure returns (bytes32) { | ||
| return keccak256(abi.encode(tick, contractType)); |
There was a problem hiding this comment.
Bug: CREATE2 Addresses: Predictability Compromised
The CREATE2 salt generation changed from keccak256(abi.encode(protocolName, tick)) to keccak256(abi.encode(tick, contractType)), breaking deterministic address generation. Any tokens deployed before this change will have different addresses than what predictTokenAddressByTick now computes, and attempting to deploy the same tick will create a contract at a different address, potentially allowing duplicate ticks or breaking address predictability guarantees.
ProtocolParserto streamline extraction of parameters for various protocols, including ERC-20 and ERC-721.WordDomainsParserfor handling legacy word-domain registrations, supporting operations likeregisterandset_primary.Erc20FixedDenominationParserandErc721EthscriptionsCollectionParserto utilize the new unified parsing approach.Note
Unifies protocol parsing, adds word-domain support, enforces content hashes for collections, and links fixed-denomination ERC-20 mints to ERC-721 collections with updated contracts, events, and tests.
ProtocolParser: Parses JSON body/headers and plain content; routes to parsers;for_calldatareturns[protocol, op, encoded]; import fallback byethscription_idfor collections.WordDomainsParser: Supportsregister(plain text or JSON) andset_primaryoperations.[protocol, op, encoded]via strict JSON regex; removed legacy structured outputs.content_hashtoitem, computes/injects hash, strict key order, updated ABI encodings and import fallback.mintId) alongside ERC-20; new mappings and views; newERC721CollectionDeployedevent; updated mint/transfer events to includemintId.collectionId, handles mint-to-zero, uses manager for metadata/URIs.ItemDataincludescontentHash; validates against ethscription content; Merkle leaf updated; create/init updated to passcollectionId.word-domainsmirroring ownership as ERC-721; exposes primary name helpers.NameRegistry; added predeploy address; registerword-domainsprotocol.ProtocolParser.for_calldata; update ABI expectations (includingcontent_hash); add gas test fortokenURI; extensive new tests for collections and name registry.Written by Cursor Bugbot for commit 1aa1898. This will update automatically on new commits. Configure here.