Refactor Ethscription structure and retrieval methods#133
Conversation
- Renamed the Ethscription struct to EthscriptionStorage for optimized storage. - Introduced a new Ethscription struct for external use, containing all relevant fields. - Updated mapping and retrieval functions to accommodate the new struct design. - Enhanced getEthscription methods to allow optional content inclusion for gas efficiency. - Adjusted tests to reflect changes in struct usage and retrieval methods.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Ethscriptions contract API by introducing a new denormalized Ethscription struct for external consumption and separating the internal storage representation (EthscriptionStorage). The main changes consolidate multiple function calls into a single getEthscription() method with optional content inclusion.
Key changes:
- Introduced new
Ethscriptionstruct withuint256types for block numbers and token IDs, separate from the storage-optimizedEthscriptionStoragestruct withuint48types - Replaced
getEthscriptionWithContent()with overloadedgetEthscription()methods that accept an optionalincludeContentparameter - Updated the Ruby
StorageReaderto call the new API methods - Updated test contracts and files to use new internal method names (
_getEthscriptionContentinstead ofgetEthscriptionContent)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/src/Ethscriptions.sol | Split Ethscription into storage (EthscriptionStorage) and external (Ethscription) structs; replaced multiple getter methods with unified getEthscription() overloads |
| contracts/src/EthscriptionsProver.sol | Updated to use new getEthscription(id, false) API and cast ethscriptionNumber to uint48 |
| contracts/src/libraries/EthscriptionsRendererLib.sol | Updated function signatures to use EthscriptionStorage instead of Ethscription |
| contracts/test/EthscriptionsWithTestFunctions.sol | Updated to use EthscriptionStorage and _getEthscriptionContent (internal method) |
| contracts/test/EthscriptionsWithContent.t.sol | Comprehensive test updates for new getEthscription() API with both includeContent parameter variations |
| contracts/test/SSTORE2ContentSizes.t.sol | Updated method calls from getEthscriptionContent to readContent |
| contracts/script/L2Genesis.s.sol | Updated to use EthscriptionStorage struct name |
| lib/storage_reader.rb | Updated ABI definitions and methods to call new contract API; changed decoding to match new struct format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ETHSCRIPTION_STRUCT_ABI = { | ||
| 'components' => [ | ||
| { 'name' => 'ethscriptionId', 'type' => 'bytes32' }, | ||
| { 'name' => 'ethscriptionNumber', 'type' => 'uint256' }, |
There was a problem hiding this comment.
Although this appears correct in the ABI definition, the corresponding line 126 uses the wrong decoding type. Line 126 decodes as '(bytes32,uint256,bytes32,bytes32,string,bytes,address,address,address,address,bytes32,uint256,uint256,uint256,bool)' which is correct, but the issue is with lines 18-20 where other uint256 fields are incorrectly defined as uint48 in the ABI.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function_sig = Eth::Util.keccak256('getEthscription(bytes32)')[0...4] | ||
|
|
||
| # Encode the parameter (bytes32 is already 32 bytes) | ||
| calldata = function_sig + [tx_hash_bytes32].pack('H*') |
There was a problem hiding this comment.
The get_ethscription_with_content method is calling getEthscription(bytes32) which will invoke the Solidity overload that includes content by default. However, this creates an inconsistency - the function should explicitly call getEthscription(bytes32,bool) with true to make the intent clear and avoid relying on implicit default behavior. Change to Eth::Util.keccak256('getEthscription(bytes32,bool)')[0...4] and update the calldata encoding on line 112.
| function_sig = Eth::Util.keccak256('getEthscription(bytes32)')[0...4] | |
| # Encode the parameter (bytes32 is already 32 bytes) | |
| calldata = function_sig + [tx_hash_bytes32].pack('H*') | |
| function_sig = Eth::Util.keccak256('getEthscription(bytes32,bool)')[0...4] | |
| # Encode the parameters: bytes32 and bool (true) | |
| bool_true = "\x00" * 31 + "\x01" | |
| calldata = function_sig + [tx_hash_bytes32].pack('H*') + bool_true |
| function_sig = Eth::Util.keccak256('getEthscription(bytes32)')[0...4] | ||
|
|
||
| # Encode the parameter (bytes32 is already 32 bytes) | ||
| calldata = function_sig + [tx_hash_bytes32].pack('H*') |
There was a problem hiding this comment.
The calldata encoding is missing the boolean parameter. Since the function signature on line 109 should be getEthscription(bytes32,bool), this line must encode both the bytes32 and a boolean true value. Change to calldata = function_sig + [tx_hash_bytes32].pack('H*') + '0' * 63 + '1' to pass true as the second parameter.
| function_sig = Eth::Util.keccak256('getEthscription(bytes32)')[0...4] | |
| # Encode the parameter (bytes32 is already 32 bytes) | |
| calldata = function_sig + [tx_hash_bytes32].pack('H*') | |
| function_sig = Eth::Util.keccak256('getEthscription(bytes32,bool)')[0...4] | |
| # Encode the parameters: bytes32 (32 bytes) + bool (32 bytes, 31 zero bytes + 1 for true) | |
| calldata = function_sig + [tx_hash_bytes32].pack('H*') + ("\x00" * 31) + "\x01" |
| previousOwner: ethscription.previousOwner, | ||
| esip6: ethscription.esip6, | ||
| ethscriptionNumber: ethscription.ethscriptionNumber, | ||
| ethscriptionNumber: uint48(ethscription.ethscriptionNumber), |
There was a problem hiding this comment.
The explicit cast from uint256 to uint48 could lead to data loss if ethscriptionNumber exceeds the uint48 maximum value (2^48 - 1). Consider adding a require statement or SafeCast to validate the value fits within uint48 bounds before casting, or update the EthscriptionDataProof struct to accept uint256.
| bytes32 id = tokenIdToEthscriptionId[tokenId]; | ||
| if (!_ethscriptionExists(id)) revert TokenDoesNotExist(); | ||
| ethscription = ethscriptions[id]; | ||
| ethscription = _getEthscriptionOrRevert(id); |
There was a problem hiding this comment.
Bug: Inconsistent Errors: Missing Token Existence Alignment
The refactor of _getEthscriptionOrRevert(uint256 tokenId) now throws EthscriptionDoesNotExist for non-existent tokens. This is inconsistent with getEthscriptionId(), which still throws TokenDoesNotExist for the same scenario, potentially affecting error handling logic.
Note
Introduce
EthscriptionStoragefor on-chain storage, a new denormalizedEthscriptionfor external use, and overloadedgetEthscriptionmethods with optional content; update renderer, prover, tests, genesis script, and Ruby client accordingly.EthscriptionStoragevs externalEthscription(denormalized with owner/content).mapping(bytes32 => EthscriptionStorage)and all internal references._buildEthscriptionand overloadgetEthscriptionby ID/tokenId withincludeContentflag; default includes content._getEthscriptionContent; updatetokenURI,getMediaUri, ownership/lookup helpers accordingly.EthscriptionStorageand adapt attribute/media building.getEthscription(id, false)and readcurrentOwnerfrom returned struct; castethscriptionNumbertouint48.EthscriptionStorage.getEthscriptionvariants; add tests for with/without content and by tokenId.readContentvia test contract; update SSTORE2 tests accordingly.Ethscriptiontuple and overloadedgetEthscription(bytes32,bool); decode fields includingcurrentOwnerand optionalcontent.Written by Cursor Bugbot for commit 7b46837. This will update automatically on new commits. Configure here.