fix: update indexing logic for Starknet and Voyager services #626
Conversation
sundayonah
requested review from
5ran6,
chibie and
onahprosper
as code owners
WalkthroughAdded structured logging around extraction and validation of StarkNet transfer event fields and wired construction of a transferEvent to invoke the existing ProcessTransfers flow; no public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
services/indexer/starknet.go (2)
251-255: Refactor: Reduce redundant parameter logging.These extracted parameters are logged again at lines 274-278 (same three fields) and comprehensively at lines 314-323. This creates excessive log volume where the same values appear 2-3 times per transfer event.
Consider removing this log statement and relying on the more comprehensive logging at lines 314-323, or consolidate all parameter logging into a single location.
274-278: Refactor: Remove redundant logging.This log duplicates the parameter logging at lines 251-255 and is superseded by the comprehensive logging at lines 314-323. This redundancy will create excessive log volume.
🔎 Proposed fix to remove redundant log
- logger.WithFields(logger.Fields{ - "valueStr": valueStr, - "fromStr": fromStr, - "toStr": toStr, - }).Infof("Processing transfer event details") - // Parse transfer value
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/indexer/starknet.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/indexer/starknet.go (3)
utils/logger/logger.go (3)
WithFields(76-109)Fields(73-73)Infof(125-130)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)ent/network/where.go (1)
GatewayContractAddress(85-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (7)
services/indexer/starknet.go (7)
202-202: LGTM: Appropriate error logging for malformed events.These error logs help surface malformed transfer events early in the processing pipeline, improving debuggability.
Also applies to: 207-207
220-220: LGTM: Comprehensive parameter validation logging.These error logs catch empty parameters after type validation, ensuring robust detection of invalid transfer events.
Also applies to: 232-232, 248-248
259-262: LGTM: Useful filtering transparency.This informational log provides clear visibility into address-based filtering decisions, which aids in debugging missed transfers.
268-271: LGTM: Clear gateway filtering observability.This log statement makes the gateway contract filtering logic transparent and aids in debugging the transfer processing flow.
290-290: LGTM: Essential metadata validation logging.These error logs appropriately flag missing required event metadata (block number and transaction hash).
Also applies to: 297-297
338-343: LGTM: Clean helper function for uint64 extraction.The implementation correctly handles uint64 type assertion and formatting. However, this function is called at lines 490, 500, 823, and 833 (not marked as changed), suggesting these call sites already existed. This raises a question about whether the function was moved from elsewhere or if there's an annotation inconsistency.
Can you confirm whether this function is newly added or relocated from another location? The existing call sites suggest it may have been defined elsewhere previously.
1-50: Critical: PR objectives don't match actual changes.The PR is linked to issue #407 which requires implementing priority queue logic to save and check previous provider rates in Redis. However, this PR only adds logging statements with no changes to:
- Priority queue logic (
CreatePriorityQueueForBucket)- Order assignment (
AssignLockPaymentOrder)- Redis operations for storing previous rates (
_prevsuffix)- Rate matching logic
Please clarify:
- Is this PR incorrectly linked to issue #407?
- Is this partial implementation with logic changes coming in a follow-up PR?
- Should the PR description and linked issue be updated to reflect that this only adds observability enhancements?
The current state creates confusion about what this PR is intended to accomplish versus what it actually delivers.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/indexer/starknet.go (1)
236-260: Performance concern: Excessive Info-level logging in hot path.These Info-level logs fire for every transfer event processed. In high-throughput scenarios, this will:
- Generate massive log volumes
- Degrade indexing performance
- Make log analysis difficult
Additionally, lines 236-239 and 256-260 are redundant—they log the same
fromStrandtoStrvalues twice within a few lines.Recommendations:
- Use
Debuglevel for detailed extraction/validation logs (lines 236-239, 256-260, 264-267, 273-276, 279-283)- Reserve
Infolevel for significant events (e.g., line 328 is appropriate)- Remove redundant logging at lines 236-239 since lines 256-260 already capture all three fields
🔎 Proposed refactor to use appropriate log levels
- logger.WithFields(logger.Fields{ - "fromStr": fromStr, - "toStr": toStr, - }).Infof("Extracted transfer event addresses") - var valueStr string if bigIntVal, ok := nonIndexedParams["value"].(*big.Int); ok { valueStr = bigIntVal.String() } else if feltVal, ok := nonIndexedParams["value"].(*felt.Felt); ok { valueStr = feltVal.BigInt(big.NewInt(0)).String() } else if strVal, ok := nonIndexedParams["value"].(string); ok { valueStr = strVal } else { logger.Errorf("Unexpected type for 'value' parameter in transfer event") continue } if valueStr == "" { logger.Errorf("Empty 'value' parameter in transfer event") continue } logger.WithFields(logger.Fields{ "fromStr": fromStr, "toStr": toStr, "valueStr": valueStr, - }).Infof("Extracted transfer event parameters") + }).Debugf("Extracted transfer event parameters") // Filter by userAccountAddress - only process transfers to the specified address if userAccountAddress != "" && !strings.EqualFold(cryptoUtils.NormalizeStarknetAddress(toStr), cryptoUtils.NormalizeStarknetAddress(userAccountAddress)) { logger.WithFields(logger.Fields{ "toStr": cryptoUtils.NormalizeStarknetAddress(toStr), "userAccountAddress": cryptoUtils.NormalizeStarknetAddress(userAccountAddress), - }).Infof("Skipping transfer event - 'to' address does not match user account address") + }).Debugf("Skipping transfer event - 'to' address does not match user account address") continue } // Skip if transfer is from gateway contract if strings.EqualFold(cryptoUtils.NormalizeStarknetAddress(fromStr), cryptoUtils.NormalizeStarknetAddress(token.Edges.Network.GatewayContractAddress)) { logger.WithFields(logger.Fields{ "fromStr": cryptoUtils.NormalizeStarknetAddress(fromStr), "GatewayContractAddress": cryptoUtils.NormalizeStarknetAddress(token.Edges.Network.GatewayContractAddress), - }).Infof("Skipping transfer event - 'from' address is gateway contract") + }).Debugf("Skipping transfer event - 'from' address is gateway contract") continue } - logger.WithFields(logger.Fields{ - "valueStr": valueStr, - "fromStr": cryptoUtils.NormalizeStarknetAddress(fromStr), - "toStr": cryptoUtils.NormalizeStarknetAddress(toStr), - }).Infof("Processing transfer event details")
🧹 Nitpick comments (1)
services/indexer/starknet.go (1)
202-207: Error logging is appropriate, but could include more context.The error-level logging for missing/invalid fields is well-placed and uses the correct log level. However, consider including the transaction hash in these error logs to make debugging easier when issues occur.
🔎 Optional enhancement: Add transaction hash to error logs
decoded, ok := eventMap["decoded"].(map[string]interface{}) if !ok || decoded == nil { - logger.Errorf("Missing or invalid 'decoded' field in transfer event") + txHash, _ := eventMap["transaction_hash"].(string) + logger.WithFields(logger.Fields{ + "txHash": txHash, + }).Errorf("Missing or invalid 'decoded' field in transfer event") continue } nonIndexedParams, ok := decoded["non_indexed_params"].(map[string]interface{}) if !ok || nonIndexedParams == nil { - logger.Errorf("Missing or invalid 'non_indexed_params' in transfer event") + txHash, _ := eventMap["transaction_hash"].(string) + logger.WithFields(logger.Fields{ + "txHash": txHash, + }).Errorf("Missing or invalid 'non_indexed_params' in transfer event") continue }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/indexer/starknet.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/indexer/starknet.go (3)
utils/logger/logger.go (4)
Errorf(141-146)WithFields(76-109)Fields(73-73)Infof(125-130)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)ent/network/where.go (1)
GatewayContractAddress(85-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
services/indexer/starknet.go (3)
1-629: Critical: PR objectives don't match implemented changes.The PR description and linked issue #407 focus on priority queue enhancements to save previous provider rates and check both current and previous rates during order assignment. However, the actual code changes only add logging statements to the Starknet indexer.
Expected changes based on PR objectives:
- Modifications to
CreatePriorityQueueForBucketto save previous queues with_prevsuffix- Updates to
AssignLockPaymentOrderto check both current and previous queues- Redis key management for
_prevsuffixed keysNone of these changes are present in this file. Please verify:
- Is this PR addressing issue #407 or a different task?
- Are the priority queue changes in a different file not included in this review?
- Should the PR description be updated to reflect the actual logging improvements?
319-328: LGTM: Well-structured logging for successful transfer processing.This Info-level log is appropriately placed and includes comprehensive context (token, addresses, values, block number, transaction hash). It fires only after validation, making it suitable for tracking successfully processed transfers.
286-290: LGTM: Proper error handling for value parsing.The error handling correctly logs parsing failures with context and continues processing remaining events rather than failing the entire batch.
Description
References
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.