fix hardcoded event#1411
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesMetadata Event Lookup
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant DecryptDdoHandler
participant findMetadataEventInLogs
participant fetchEventFromTransaction
DecryptDdoHandler->>DecryptDdoHandler: validate receipt and logs exist
DecryptDdoHandler->>findMetadataEventInLogs: filter logs by dataNftAddress
findMetadataEventInLogs->>fetchEventFromTransaction: parse MetadataCreated/MetadataUpdated event
fetchEventFromTransaction-->>findMetadataEventInLogs: parsed event or null
findMetadataEventInLogs-->>DecryptDdoHandler: matching event or null
DecryptDdoHandler->>DecryptDdoHandler: throw if null, else extract flags/encryptedDocument/documentHash
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR fixes a bug where the handler blindly assumed the MetadataCreated or MetadataUpdated event would always be the first log in a transaction receipt. By searching through all logs and filtering by the Data NFT address, the code now correctly handles transactions originating from smart wallets (ERC-4337, multisigs) or other complex contracts that emit multiple events. The implementation is robust and well-tested. LGTM!
Comments:
• [INFO][performance] It's a minor optimization, but you could define const abiInterface = new ethers.Interface(ERC721Template.abi) outside of the findMetadataEventInLogs function (e.g., at the module level). This avoids re-instantiating the ethers.Interface on every call, which parses the ABI JSON every time. Given that this function is not called in a tight loop, the performance impact is negligible, but it's a good practice.
• [INFO][style] Since you added a check for !receipt, the error message 'receipt logs 0' might be slightly misleading if the receipt itself is null (which can happen if the transaction hasn't been mined or doesn't exist on the provider). Consider updating it to something more accurate.
- throw new Error('receipt logs 0')
+ throw new Error('Transaction receipt not found or has no logs')• [INFO][other] Excellent unit tests! You clearly mocked the foreign log scenarios (such as those from ERC-4337 entry points) and thoroughly tested case-insensitivity and other edge cases. Great job on the test coverage here.
Fix: don't assume the metadata event is the first log in the transaction
Summary
DecryptDdoHandler.decryptDDO()resolved the encrypted document from atransactionIdbyparsing
receipt.logs[0]and requiring it to beMetadataCreated/MetadataUpdated.That assumption only holds for plain EOA transactions. For transactions sent through
account-abstraction wallets (ERC-4337), multisigs (e.g. Safe) or relayers, other events
(EntryPoint/wallet/token events) precede the metadata event, so decryption failed with
Decrypt DDO: Failed to process transaction ideven though the transaction contains aperfectly valid metadata event.
What changed
findMetadataEventInLogs(logs, dataNftAddress)insrc/components/core/handler/ddoHandler.ts:logs[0],address match) — so batched AA transactions that touch multiple NFTs resolve the event
of the correct contract,
fetchEventFromTransaction()util fromsrc/utils/util.tsfor theactual log parsing/matching (no shared-util changes needed; logs are pre-filtered by
NFT address before delegating),
MetadataCreatedorMetadataUpdatedevent, ornull.decryptDDO()now uses the helper; also guards against anullreceipt. Arg extraction(
args[3]/args[4]/args[5]= flags / encrypted document / document hash) and theerror contract (HTTP 400
Failed to process transaction id, specific reason logged) areunchanged.
Note:
getEventFromTx()fromsrc/utils/util.tscould not be reused here — it matcheson
log.fragment.name, which is only populated on receipts obtained from an ethersContract(tx.wait()).provider.getTransactionReceipt()returns plain logs withoutfragment, so it would never match.Audit: other hard-coded event positions in the codebase
The rest of the codebase was audited for the same class of bug:
src/components/Indexer/ChainIndexer.ts(receipt.logs[reindexTask.eventIndex]) —numeric index, but
eventIndexis optional, never set by its only producer(
reindexTxHandler), and falls back to scanning all logs when undefined. Intentionaltargeted-reindex feature; left as is.
getEventFromTx/fetchEventFromTransactionfilter all logs;BaseProcessor.getEventDataand the mainindexer loop match
topics[0]event-signature hashes across all logs; remainingtopics[N]usages are standard indexed-argument access.Tests
src/test/unit/ddoEventLog.test.ts(6 tests) using synthetic receiptsbuilt with
Interface.encodeEventLog:MetadataCreatedwhen it is not the first log (the AA/multisig scenario),MetadataUpdated,nullwhen no metadata event exists / on an empty logs array.Summary by CodeRabbit