Refactor ERC721 Ethscriptions Collection Management#135
Conversation
- Updated the collection creation process to use `max_supply` instead of `total_supply`. - Introduced a new operation `create_and_add_self` for creating collections with associated items. - Enhanced validation methods for collection metadata and item attributes, including optional merkle proof handling. - Adjusted ABI types and internal data structures to accommodate the new parameters. - Updated tests to reflect changes in the collection management logic and ensure compatibility with the new structure.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the collections protocol to use max_supply instead of total_supply and adds support for merkle-based allowlisting. The changes modernize the data structures by consolidating metadata into a single CollectionRecord struct and implementing merkle proof validation for item additions.
- Renamed
total_supplytomax_supplythroughout the codebase for better semantic clarity - Added merkle tree support with optional
merkleRootfield andmerkleProofarrays for allowlisting - Refactored Solidity contract to consolidate
CollectionStateandCollectionMetadataintoCollectionRecord
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/models/erc721_ethscriptions_collection_parser_spec.rb | Updates test fixtures to use max_supply instead of total_supply and adds merkle proof support in ABI type definitions |
| spec/integration/collections_protocol_spec.rb | Updates integration tests with renamed field and corrects comment from CollectionMetadata to CollectionParams |
| spec/integration/collections_protocol_e2e_spec.rb | Updates end-to-end tests to use max_supply field |
| lib/collections_reader.rb | Refactors to use unified getCollection contract call and adds merkleRoot field support |
| contracts/test/CollectionsProtocol.t.sol | Updates test cases to use CollectionParams struct with max_supply and merkleRoot fields |
| contracts/test/CollectionsManager.t.sol | Updates unit tests with new struct names and adds merkleProof parameter to item operations |
| contracts/test/AddressPrediction.t.sol | Updates test to use CollectionRecord and maxSupply field |
| contracts/src/ERC721EthscriptionsCollectionManager.sol | Major refactoring: consolidates structs, adds merkle proof validation, and implements ownership syncing via onTransfer |
| contracts/src/ERC721EthscriptionsCollection.sol | Simplifies collection contract to thin wrapper with ownership management and removes sync functionality |
| app/models/erc721_ethscriptions_collection_parser.rb | Adds create_and_add_self operation, merkle proof validation, and updates all operations to include merkle root |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data['discord_link'] | ||
| data['discord_link'], | ||
| # Append zero merkle root to satisfy contract struct shape | ||
| ["".ljust(64, '0')].pack('H*') |
There was a problem hiding this comment.
The empty string padded to 64 zeros creates a zero bytes32 value. Consider extracting this magic value to a named constant (e.g., ZERO_BYTES32) for better readability and maintainability.
| ] | ||
|
|
||
| # Append zero merkle root if not provided in payload (parser schema omits it) | ||
| values << ["".ljust(64, '0')].pack('H*') |
There was a problem hiding this comment.
This duplicates the zero bytes32 value from line 367. Consider extracting this to a named constant (e.g., ZERO_BYTES32) to avoid duplication and improve maintainability.
| string memory mediaField = string.concat( | ||
| ',"', | ||
| mediaType, | ||
| mediaType.escapeJSON(), |
There was a problem hiding this comment.
The mediaType field should not be escaped with escapeJSON() when it's used as a JSON key name. The media type values ('image' or 'animation_url') are controlled outputs and escaping them could break the JSON structure by incorrectly escaping characters in the key name.
| mediaType.escapeJSON(), | |
| mediaType, |
| mediaType.escapeJSON(), | ||
| '":"', | ||
| mediaUri.escapeJSON(), | ||
| mediaUri, |
There was a problem hiding this comment.
The mediaUri should be escaped using escapeJSON() for security. URI strings can contain characters that need escaping in JSON (e.g., quotes, backslashes). The line should be mediaUri.escapeJSON().
| mediaUri, | |
| mediaUri.escapeJSON(), |
| end No newline at end of file | ||
|
|
||
| def self.format_bytes32_hex(value) | ||
| hex = value.to_s.delete_prefix('0x') |
There was a problem hiding this comment.
The slicing operation [0,64] after rjust(64, '0') is redundant when the hex is shorter than 64 chars, since rjust already ensures it's exactly 64 chars. Only values longer than 64 chars would be affected by the slice. Consider clarifying if this is intentional truncation for overly-long inputs or removing the redundant slice.
| hex = value.to_s.delete_prefix('0x') | |
| hex = value.to_s.delete_prefix('0x') | |
| # Truncate to 64 hex characters to ensure bytes32 length; pad with zeros if shorter |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Emit Transfer to address(0) for indexers to track removal | ||
| emit Transfer(currentOwner, address(0), tokenId); | ||
| // Emit burn-style transfer for indexers | ||
| emit Transfer(ownerOf(tokenId), address(0), tokenId); |
There was a problem hiding this comment.
Bug: Reorder: Fetch Owner Before Marking Nonexistent Token
In removeMember, the token's existence flag is set to false (line 65) before calling ownerOf(tokenId) (line 68). The ownerOf function calls _requireOwned, which checks the existence flag and reverts with ERC721NonexistentToken if the token doesn't exist. This causes the function to always revert when trying to emit the Transfer event. The owner should be retrieved before marking the token as non-existent.
| mediaType.escapeJSON(), | ||
| '":"', | ||
| mediaUri.escapeJSON(), | ||
| mediaUri, |
There was a problem hiding this comment.
Bug: JSON Injection Risk from Unescaped mediaUri
The mediaUri value is not escaped with escapeJSON() when constructing the JSON tokenURI, while mediaType on line 106 is properly escaped. This inconsistency could allow JSON injection if the mediaUri contains special characters like quotes or backslashes. The old code properly escaped mediaUri with escapeJSON(), but this was removed in the refactor.
- Introduced new operations `create_collection_and_add_self` and `add_self_to_collection` for streamlined collection and item management. - Updated the `extract` method to support `ethscription_id` and improved import fallback handling. - Refactored item data structures to remove `ethscription_id` from item data during batch operations. - Enhanced validation and encoding processes for collection metadata and item attributes. - Added comprehensive tests for new functionalities, ensuring proper integration and behavior in the collections protocol.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
contracts/src/ERC721EthscriptionsCollectionManager.sol:1
- The slice operation
[0,64]afterrjustsuggests potential issues with input validation. Ifhex.length > 64before padding, this silently truncates, which could mask bugs. Consider adding validation to ensure the hex string is at most 64 characters before padding.
// SPDX-License-Identifier: MIT
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| name: data[:name], | ||
| symbol: data[:symbol], | ||
| maxSupply: data[:maxSupply], | ||
| totalSupply: data[:maxSupply], |
There was a problem hiding this comment.
The totalSupply field is set to maxSupply, which represents the maximum allowed items, not the current number of minted items. This should be set to the actual current supply retrieved from the collection contract (like currentSize in get_collection_state). This will cause APIs to report incorrect supply counts.
| { | |
| name: data[:name], | |
| symbol: data[:symbol], | |
| maxSupply: data[:maxSupply], | |
| totalSupply: data[:maxSupply], | |
| state = get_collection_state(collection_id, block_tag: block_tag) | |
| { | |
| name: data[:name], | |
| symbol: data[:symbol], | |
| maxSupply: data[:maxSupply], | |
| totalSupply: state ? state[:currentSize] : 0, |
| unless value.keys == expected_min_keys || value.keys == (expected_min_keys + ['merkle_root']) | ||
| raise ValidationError, "Invalid metadata keys or order" |
There was a problem hiding this comment.
The array concatenation expected_min_keys + ['merkle_root'] creates a new array each time this validation runs, but doesn't enforce that merkle_root must be the last key. If other keys appear after merkle_root, validation will fail incorrectly. Consider using a more explicit key order check or documenting that merkle_root must be last.
| unless value.keys == expected_min_keys || value.keys == (expected_min_keys + ['merkle_root']) | |
| raise ValidationError, "Invalid metadata keys or order" | |
| keys = value.keys | |
| if keys == expected_min_keys | |
| # ok | |
| elsif keys.length == expected_min_keys.length + 1 && keys[0, expected_min_keys.length] == expected_min_keys && keys.last == 'merkle_root' | |
| # ok | |
| else | |
| raise ValidationError, "Invalid metadata keys or order (merkle_root must be last if present)" |
| items = JSON.parse(File.read(items_path)) | ||
| collections = JSON.parse(File.read(collections_path)) |
There was a problem hiding this comment.
File I/O operations can raise exceptions (e.g., Errno::ENOENT, JSON::ParserError) but are not wrapped in error handling. This could cause unexpected crashes during import fallback. Consider adding rescue clauses to gracefully handle missing or malformed JSON files.
| items = JSON.parse(File.read(items_path)) | |
| collections = JSON.parse(File.read(collections_path)) | |
| begin | |
| items = JSON.parse(File.read(items_path)) | |
| collections = JSON.parse(File.read(collections_path)) | |
| rescue Errno::ENOENT => e | |
| warn "File not found: #{e.message}" | |
| items = {} | |
| collections = {} | |
| rescue JSON::ParserError => e | |
| warn "JSON parse error: #{e.message}" | |
| items = {} | |
| collections = {} | |
| end |
| if (!senderIsCollectionOwner && !_inImportMode()) { | ||
| _verifyItemMerkleProof(item, collection.merkleRoot); | ||
| } |
There was a problem hiding this comment.
The Merkle proof verification is skipped when merkleRoot is bytes32(0) (checked inside _verifyItemMerkleProof), but this creates two distinct permission models: when merkleRoot is zero, anyone can add items; when non-zero, only those with valid proofs can add. This dual behavior should be clearly documented, and consider whether unrestricted addition when merkleRoot is zero is the intended design.
| end | ||
|
|
||
| def to_bytes32_hex(val) | ||
| h = safe_string(val).downcase |
There was a problem hiding this comment.
The validation regex allows only lowercase hex characters [0-9a-f], but the code lowercases the input before checking. If the original validation should preserve case sensitivity or reject uppercase, the current logic contradicts that. If case-insensitive validation is intended, document this behavior clearly.
| h = safe_string(val).downcase | |
| h = safe_string(val) |
| item2_results = import_l1_block([item2_spec], esip_overrides: { esip6_is_enabled: true }) | ||
| item2_id = item2_results[:ethscription_ids].first | ||
| expect(item2_id).to be_present, "Item 2 should be created" | ||
| batch_results = item2_results # Use the second item's results for validation |
There was a problem hiding this comment.
This assignment to batch_results is useless, since its value is never read.
| batch_results = item2_results # Use the second item's results for validation |
- Removed `ethscription_id` from item data structures in the `Erc721EthscriptionsCollectionParser` to streamline item management. - Updated ABI types for various operations to reflect changes in item data handling. - Enhanced error logging for ABI encoding issues in both `Erc721EthscriptionsCollectionParser` and `EthscriptionTransaction`. - Simplified the `ProtocolExtractor` by removing unnecessary validations and directly extracting protocol parameters. - Improved test coverage for new item handling and collection operations, ensuring robust integration with the collections protocol.
|
bugbot run |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| function _inImportMode() private view returns (bool) { | ||
| return block.timestamp < Constants.historicalBackfillApproxDoneAt; |
There was a problem hiding this comment.
Missing import for Constants library. The code references Constants.historicalBackfillApproxDoneAt but there is no import statement for ./libraries/Constants.sol. This will cause a compilation error.
| def load_import_data(items_path:, collections_path:) | ||
| items = JSON.parse(File.read(items_path)) | ||
| collections = JSON.parse(File.read(collections_path)) |
There was a problem hiding this comment.
Missing error handling for file I/O operations. If the JSON files don't exist or contain invalid JSON, this will raise unhandled exceptions. Consider wrapping in a rescue block and returning a sensible default or nil to allow graceful degradation when import files are unavailable.
| if (encoded = build_import_encoded_params(ethscription_id.to_hex)) | ||
| return encoded | ||
| end | ||
| end |
There was a problem hiding this comment.
Calling .to_hex on ethscription_id without verifying it responds to that method. If ethscription_id is already a string or doesn't have a to_hex method, this will raise NoMethodError. Add a guard to check if ethscription_id is a ByteString or similar type before calling .to_hex.
| if (encoded = build_import_encoded_params(ethscription_id.to_hex)) | |
| return encoded | |
| end | |
| end | |
| hex_id = ethscription_id.respond_to?(:to_hex) ? ethscription_id.to_hex : ethscription_id | |
| if (encoded = build_import_encoded_params(hex_id)) | |
| return encoded | |
| end |
| # begin | ||
| # payload = DataUri.new(content_uri).decoded_data | ||
| # rescue StandardError | ||
| # return nil | ||
| # end | ||
| # return nil unless payload.start_with?('{') |
There was a problem hiding this comment.
Commented-out validation code for data URI and JSON payload checking should be removed. If this validation is no longer needed, delete it; if it's still being evaluated, add a TODO comment explaining why it's temporarily disabled.
| # begin | |
| # payload = DataUri.new(content_uri).decoded_data | |
| # rescue StandardError | |
| # return nil | |
| # end | |
| # return nil unless payload.start_with?('{') | |
| rescue => e | ||
| Rails.logger.error "Import error: #{e.class} - #{e.message}" | ||
| Rails.logger.error e.backtrace.first(20).join("\n") | ||
| Rails.logger.error e.backtrace.join("\n") |
There was a problem hiding this comment.
[nitpick] Logging the entire backtrace without truncation could produce very large log entries. Consider limiting to first N frames (e.g., e.backtrace.first(50).join('\n')) to balance debugging needs with log size management.
| Rails.logger.error e.backtrace.join("\n") | |
| Rails.logger.error e.backtrace.first(50).join("\n") |
| explicit = pairs.map { |(iid, _)| [iid, items_by_id[iid]['index']] } | ||
| explicit_indices = explicit.filter_map { |_iid, idx| idx if idx.is_a?(Integer) } | ||
| if explicit_indices.size == pairs.size | ||
| min_idx = explicit_indices.min |
There was a problem hiding this comment.
The offset logic (min_idx == 0) ? 0 : 1 is unclear without context. Add a comment explaining why items with min_idx=0 get offset=0 while others get offset=1, as this affects zero-based index normalization.
| min_idx = explicit_indices.min | |
| min_idx = explicit_indices.min | |
| # If the minimum index is 0, indices are already zero-based; otherwise, offset by 1 to normalize to zero-based indexing. |
| bytes32 id = manager.collectionIdForAddress(address(this)); | ||
| if (id == bytes32(0)) revert UnknownCollection(); | ||
| return id; |
There was a problem hiding this comment.
The collectionId() function performs an external call to the manager contract on every invocation. Since this value never changes after initialization, consider caching it as a storage variable during initialize() to avoid repeated external calls.
max_supplyinstead oftotal_supply.create_and_add_selffor creating collections with associated items.Note
Reworks ERC-721 collections to use max_supply + merkleRoot with new self-referential ops, adds import fallback, auto-ownership sync, and updates parser, contracts, reader, and tests.
app/models/erc721_ethscriptions_collection_parser.rb):total_supplywithmax_supply; extend ABI to includebytes32 merkle_root.create_collection_and_add_self(plus legacy alias) andadd_self_to_collection; remove legacyadd_items_batch/sync_ownershippaths.merkle_proofon items; improved type validators; binary-safe encoding and error logging.ethscription_idis provided, auto-build params from JSON (items_by_ethscription.json,collections_by_name.json).ProtocolExtractor/EthscriptionTransaction: plumbethscription_idinto extractors; enhanced ABI error logs.ERC721EthscriptionsCollectionnowOwnableUpgradeable; manager-driven mint/burn,forceTransfer, and owner sync via factory; tokenURI generation unchanged.ERC721EthscriptionsCollectionManagerstoresCollectionRecord(incl.maxSupply,merkleRoot),Membershipmapping, deterministic CREATE2 deployment, on-transfer auto-sync, single-item add with optional merkle verification, edit/remove/lock ops; exposesgetCollection,getEthscriptionTokenId, etc.L2Genesis.s.sol: always registers collections protocol handler.CollectionsReader: switch togetCollection(bytes32)and derivecurrentSizeviatotalSupply(); surfacemerkleRoot.derive_ethscriptions_blocks.rb/genesis_generator.rb: logging tweak; removeENABLE_COLLECTIONSflag usage.max_supply, new ops, and new getters; add import-fallback specs and end-to-end flows; address prediction adjusted.Written by Cursor Bugbot for commit 466e4fc. This will update automatically on new commits. Configure here.