Handle duplicate ethscription prevention at the contract level#106
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements duplicate ethscription prevention at the contract level by refactoring how ethscription transactions are tracked and validated. The changes move away from application-level duplicate prevention to a more robust contract-based approach.
- Refactored ethscription transaction tracking to use unified source metadata (source_type and source_index)
- Removed application-level duplicate prevention logic in favor of contract-level validation
- Added support for multiple ethscription transfers in a single transaction
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| contracts/test/EthscriptionsWithContent.t.sol | New test file for testing the getEthscriptionWithContent method |
| app/services/eth_block_importer.rb | Removed initialization of seen creates tracking |
| app/models/ethscription_transaction_builder.rb | Updated to use new source tracking and removed duplicate prevention logic |
| app/models/ethscription_transaction.rb | Major refactor to use unified source tracking and support multiple transfers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| raise "Unknown ethscription operation: #{ethscription_operation}" | ||
| end | ||
| payload = ByteString.from_bin( | ||
| eth_transaction.block_hash.to_bin + |
There was a problem hiding this comment.
The method uses eth_transaction.block_hash but the EthTransaction model likely has block_hash as a Hash32 object, not a method that returns binary data. This should probably be eth_transaction.block_hash.to_bin.
| transaction = EthscriptionTransaction.transfer_multiple_ethscriptions( | ||
| eth_transaction: @eth_tx, | ||
| from_address: normalize_address(@eth_tx.from_address), | ||
| to_address: normalize_address(@eth_tx.to_address), | ||
| ethscription_ids: ids, | ||
| source_type: :input, | ||
| source_index: @eth_tx.transaction_index | ||
| ) | ||
|
|
||
| @transactions << transaction | ||
| end | ||
| @transactions << transaction |
There was a problem hiding this comment.
The method creates a single transaction object for multiple ethscription transfers, but the logic doesn't handle the case where some transfers might be invalid. This could cause valid transfers to be rejected due to one invalid transfer in the batch.
| case ethscription_operation | ||
| when 'transfer' | ||
| if transfer_ids | ||
| # Multiple transfer (input-based) | ||
| transfer_ids.is_a?(Array) && transfer_ids.any? | ||
| else | ||
| # Single transfer (event-based) | ||
| ethscription_id.present? | ||
| end |
There was a problem hiding this comment.
[nitpick] The validation logic mixes different transfer types in a single case branch, making it unclear when each branch applies. Consider separating validation for single transfers and multiple transfers into distinct methods for better clarity.
|
bugbot run |
![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 4 out of 4 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| source_tag = operation_source.to_s # "input" or "event" | ||
| source_tag_hash = Eth::Util.keccak256(source_tag.bytes.pack('C*')) # Hash for constant width |
There was a problem hiding this comment.
Converting symbol to string and then hashing on every call is inefficient. Consider using a constant lookup table or memoization for the source tag hashes since there are only two possible values.
| case ethscription_operation | ||
| when 'transfer' | ||
| if transfer_ids | ||
| # Multiple transfer (input-based) | ||
| transfer_ids.is_a?(Array) && transfer_ids.any? | ||
| else | ||
| # Single transfer (event-based) | ||
| ethscription_id.present? | ||
| end |
There was a problem hiding this comment.
The validation logic is inconsistent - it checks transfer_ids.is_a?(Array) but the property is already typed as T.nilable(T::Array[String]). This redundant type check should be removed since Sorbet already enforces the type.
No description provided.