Enhance ERC721 Ethscriptions Collection Parser with Initial Owner Support #145
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…port - Updated the `Erc721EthscriptionsCollectionParser` to include `initial_owner` in the `create_collection` operation schema and ABI type. - Modified the `validate_and_encode` method to accept an `eth_transaction` parameter for determining the initial owner context. - Enhanced metadata handling to support ownership renouncement and validation of optional addresses. - Updated tests to reflect changes in the collection creation process, ensuring proper handling of the new `initial_owner` field.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| it 'handles empty optional fields' do | ||
| json = %(data:,{"p":"erc-721-ethscriptions-collection","op":"create_collection","name":"Test","symbol":"TST","max_supply":"100","description":"","logo_image_uri":"","banner_image_uri":"","background_color":"","website_link":"","twitter_link":"","discord_link":"","merkle_root":"#{zero_merkle_root}"}) | ||
| json = %(data:,{"p":"erc-721-ethscriptions-collection","op":"create_collection","name":"Test","symbol":"TST","max_supply":"100","description":"","logo_image_uri":"","banner_image_uri":"","background_color":"","website_link":"","twitter_link":"","discord_link":"","merkle_root":"#{zero_merkle_root}","initial_owner":"0x0000000000000000000000000000000000000001"}) |
There was a problem hiding this comment.
Bug: ABI Decoder: Outdated Schema Causes Errors
The ABI decoder uses an outdated type signature missing the address field for initial_owner. The encoded data now includes 12 fields (with initial_owner at the end), but the decoder expects only 11 fields, causing the decoded tuple to have incorrect values or throw a decoding error.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the ERC721 Ethscriptions Collection Parser to support explicit ownership assignment during collection creation through the addition of an initial_owner field. Previously, the owner was implicitly determined from the transaction context; now it can be explicitly specified, including support for immediate ownership renouncement via address(0).
Key Changes
- Added
initialOwnerfield to theCollectionParamsstruct in both Solidity contracts and Ruby parsers - Updated ABI types across the codebase to include the new
addressparameter (12th field in create_collection operations) - Enhanced the Ruby parser to derive
initial_ownerfrom transaction context when not explicitly specified, with support for theshould_renounceflag - Implemented special handling in Solidity for
address(0)ownership via a workaround pattern to support immediate renouncement - Updated all test fixtures to include the new
initial_ownerfield - Modified
protocol_parser.rbto accept and passeth_transactionparameter throughout the call chain
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/src/ERC721EthscriptionsCollectionManager.sol | Added initialOwner field to CollectionParams; refactored _createCollection into _initializeCollection and _storeCollectionData for better separation of concerns |
| contracts/src/ERC721EthscriptionsCollection.sol | Implemented ownership renouncement pattern for address(0) initialization using temporary address(1) assignment |
| app/models/erc721_ethscriptions_collection_parser.rb | Added validation and encoding for initial_owner; enhanced build_metadata_object to support transaction-based owner derivation and should_renounce flag |
| app/models/protocol_parser.rb | Added eth_transaction parameter to method signatures for passing transaction context to parsers |
| app/models/ethscription_transaction.rb | Updated to pass eth_transaction instead of just ethscription_id to protocol parser |
| spec/models/protocol_parser_spec.rb | Updated test fixtures to include initial_owner field |
| spec/models/erc721_ethscriptions_collection_parser_spec.rb | Updated test fixtures and round-trip test to include new initial_owner field with correct ABI type |
| spec/models/erc721_collections_import_fallback_spec.rb | Added mock transaction objects with from_address for testing import fallback scenarios |
| spec/integration/collections_protocol_spec.rb | Updated integration test fixtures to include initial_owner field |
| spec/integration/collections_protocol_e2e_spec.rb | Updated end-to-end test fixtures to include initial_owner field |
| contracts/test/*.t.sol | Updated Solidity test fixtures to include initialOwner in collection creation parameters with descriptive comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (initialOwner_ == address(0)) { | ||
| __Ownable_init(address(1)); | ||
| _transferOwnership(address(0)); |
There was a problem hiding this comment.
[nitpick] This workaround pattern for renouncing ownership at initialization may be fragile and could fail if OpenZeppelin's OwnableUpgradeable implementation changes. The contract first initializes with address(1) then immediately transfers to address(0).
Consider documenting this pattern with a comment explaining why this workaround is necessary, or consider using OpenZeppelin's renounceOwnership() function after initialization with a non-zero address if the intended behavior is to have no owner.
Suggested change
| _transferOwnership(address(0)); | |
| renounceOwnership(); |
| string twitterLink; | ||
| string discordLink; | ||
| bytes32 merkleRoot; | ||
| address initialOwner; |
There was a problem hiding this comment.
The new initialOwner field in the CollectionParams struct lacks documentation. Consider adding a comment to explain:
- Its purpose (setting the initial owner of the collection)
- Special behavior when set to
address(0)(ownership renouncement) - The expected value (typically the transaction sender)
For example:
address initialOwner; // Initial owner of the collection; use address(0) to renounce ownership immediately
Suggested change
| address initialOwner; | |
| address initialOwner; // Initial owner of the collection; use address(0) to renounce ownership immediately. Typically set to the transaction sender. |
Comment on lines
+593
to
+598
| def validate_optional_address(value, field_name) | ||
| unless value.is_a?(String) && value.match?(/\A0x[0-9a-f]{40}\z/i) | ||
| raise ValidationError, "Invalid address for #{field_name}: #{value}" | ||
| end | ||
| # Allow address(0) for renouncement | ||
| value.downcase |
There was a problem hiding this comment.
[nitpick] The regex pattern /\A0x[0-9a-f]{40}\z/i uses the case-insensitive flag i, but then forces lowercase with .downcase. This means uppercase addresses (including checksummed addresses) are accepted and normalized to lowercase.
However, the regex should either:
- Remove the
iflag to strictly require lowercase hex (matching the pattern used for bytes32), or - Keep the
iflag if checksummed addresses need to be supported
Consider aligning with the validate_bytes32 approach which uses /\A0x[0-9a-f]{64}\z/ (no i flag) for consistency, unless there's a specific reason to accept checksummed addresses.
- Updated the `Erc721EthscriptionsCollectionParser` to convert `from_address` to a hexadecimal format using `to_hex` method for consistency. - Modified test cases to utilize `Address20.from_hex` for mock transactions, ensuring proper address handling in the context of collection creation. - Adjusted ABI encoding in tests to include the address type, reflecting the updated structure for collection operations.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
spec/models/erc721_collections_import_fallback_spec.rb:185
- The test should verify that the
initial_ownerfield (metadata[11]) is correctly set from theeth_transaction.from_address. Consider adding an assertion likeexpect(metadata[11]).to eq('0x0000000000000000000000000000000000000001')to validate the import fallback correctly derives the initial owner.
it 'builds create_collection_and_add_self for specific ethscription using live JSON files' do
# This ethscription should be the leader of a collection in the live JSON files
specific_id = '0x05aac415994e0e01e66c4970133a51a4cdcea1f3a967743b87e6eb08f2f4d9f9'
# Create a mock eth_transaction with from_address
mock_tx = double('eth_transaction', from_address: Address20.from_hex('0x0000000000000000000000000000000000000001'))
protocol, operation, encoded = ProtocolParser.for_calldata(
'data:,{}',
eth_transaction: mock_tx,
ethscription_id: ByteString.from_hex(specific_id)
)
expect(protocol).to eq('erc-721-ethscriptions-collection'.b)
expect(operation).to eq('create_collection_and_add_self'.b)
# Decode to verify it's properly formed
decoded = Eth::Abi.decode([
'((string,string,uint256,string,string,string,string,string,string,string,bytes32,address),(bytes32,uint256,string,string,string,(string,string)[],bytes32[]))'
], encoded)[0]
metadata = decoded[0]
item = decoded[1]
# Should have valid metadata
expect(metadata[0]).to be_a(String) # name
expect(metadata[0].length).to be > 0
expect(metadata[1]).to be_a(String) # symbol
expect(metadata[2]).to be_a(Integer) # maxSupply
# Should have valid item data (contentHash is first field now)
expect(item[0]).to be_a(String) # content_hash (packed bytes)
expect(item[1]).to be_a(Integer) # item_index
expect(item[2]).to be_a(String) # name
expect(item[3]).to be_a(String) # background_color
expect(item[4]).to be_a(String) # description
expect(item[5]).to be_an(Array) # attributes
expect(item[6]).to be_an(Array) # merkle_proof
end
spec/models/erc721_ethscriptions_collection_parser_spec.rb:142
- The test validates the encoded data but doesn't verify the
initial_ownerfield (which would be at index 11 in the decoded tuple). Consider adding an assertion to ensure the field is properly encoded and decoded.
it 'handles empty optional fields' do
json = %(data:,{"p":"erc-721-ethscriptions-collection","op":"create_collection","name":"Test","symbol":"TST","max_supply":"100","description":"","logo_image_uri":"","banner_image_uri":"","background_color":"","website_link":"","twitter_link":"","discord_link":"","merkle_root":"#{zero_merkle_root}","initial_owner":"0x0000000000000000000000000000000000000001"})
result = ProtocolParser.for_calldata(json)
expect(result[0]).to eq('erc-721-ethscriptions-collection'.b)
expect(result[1]).to eq('create_collection'.b)
decoded = Eth::Abi.decode(
['(string,string,uint256,string,string,string,string,string,string,string,bytes32,address)'],
result[2]
)[0]
expect(decoded[0]).to eq("Test")
expect(decoded[1]).to eq("TST")
expect(decoded[2]).to eq(100)
expect(decoded[3]).to eq("")
expect(decoded[10]).to eq([zero_merkle_root[2..]].pack('H*'))
end
spec/models/erc721_collections_import_fallback_spec.rb:88
- The test should verify that the
initial_ownerfield (metadata[11]) is correctly set from theeth_transaction.from_address. Consider adding an assertion likeexpect(metadata[11]).to eq('0x0000000000000000000000000000000000000001')to validate the import fallback correctly derives the initial owner from the transaction context.
it 'builds create_collection_and_add_self for the leader via import fallback' do
# Create a mock eth_transaction with from_address
mock_tx = double('eth_transaction', from_address: Address20.from_hex('0x0000000000000000000000000000000000000001'))
protocol, operation, encoded = ProtocolParser.for_calldata(
'data:,{}',
eth_transaction: mock_tx,
ethscription_id: ByteString.from_hex(leader_id)
)
expect(protocol).to eq('erc-721-ethscriptions-collection'.b)
expect(operation).to eq('create_collection_and_add_self'.b)
decoded = Eth::Abi.decode([
'((string,string,uint256,string,string,string,string,string,string,string,bytes32,address),(bytes32,uint256,string,string,string,(string,string)[],bytes32[]))'
], encoded)[0]
metadata = decoded[0]
item = decoded[1]
expect(metadata[0]).to eq('Test Collection') # name
expect(metadata[1]).to eq('TEST') # symbol (from slug)
expect(metadata[2]).to eq(2) # maxSupply from total_supply
# Item now has contentHash as first field
expect(item[1]).to eq(0) # item index (now at position 1)
expect(item[2]).to eq('Item Zero') # name (now at position 2)
expect(item[3]).to eq('') # background_color (now at position 3)
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| it "accepts GZIP input post-ESIP-7" do | ||
| compressed_data_uri = Zlib.gzip("data:text/plain;charset=utf-8,Hello World") # Placeholder for GZIP data | ||
| compressed_data_uri = Zlib.gzip("data:text/plain;charset=utf-8,Hello Worldaaa") # Placeholder for GZIP data |
There was a problem hiding this comment.
This change appears to be unrelated to the PR's purpose of adding initial_owner support. The test string was changed from "Hello World" to "Hello Worldaaa", which seems accidental. This should be reverted to maintain the original test case.
Suggested change
| compressed_data_uri = Zlib.gzip("data:text/plain;charset=utf-8,Hello Worldaaa") # Placeholder for GZIP data | |
| compressed_data_uri = Zlib.gzip("data:text/plain;charset=utf-8,Hello World") # Placeholder for GZIP data |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Erc721EthscriptionsCollectionParserto includeinitial_ownerin thecreate_collectionoperation schema and ABI type.validate_and_encodemethod to accept aneth_transactionparameter for determining the initial owner context.initial_ownerfield.Note
Add
initial_ownerto ERC-721 collection params/ABI and wire it through parsing, import fallback, L2 calldata, and contracts (including zero-address renounce), with tests updated accordingly.initial_ownertocreate_collectionschema and tocreate_collection_and_add_self/legacy tuple ABIs; introducevalidate_optional_address.initial_ownerinbuild_metadata_objectviashould_renounce, explicit field, oreth_transaction.from_address; raise when unavailable.eth_transactionand includesinitial_ownerin encoded metadata.ProtocolParser/EthscriptionTransactionnow passeth_transactionto parsers and support derivingethscription_idfrom it.CollectionParamsaddsaddress initialOwner.ERC721EthscriptionsCollection.initializehandles zero address by initializing then transferring ownership toaddress(0)._initializeCollectionand_storeCollectionData; initialize withmetadata.initialOwnerand record contract address accordingly.initial_owner/addressin tuples.eth_transactioncontext.Written by Cursor Bugbot for commit 76710a3. This will update automatically on new commits. Configure here.