fix service request query #471
Conversation
WalkthroughThe pull request introduces a refactoring of the Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
modules/service/client/utils/query.go (4)
102-103: Add return value documentation.Please document the return values in the function comment to clarify the meaning of -1 and possible error conditions.
-// findMsgIndex will find the message index in the txInfo. +// findMsgIndex finds the message index in the txInfo. +// Returns the message index (>= 0) if found, or -1 if not found. +// Returns an error if the context is unknown or if parsing fails.
111-116: Fix potential integer overflow and typo.The current implementation has two issues:
ParseIntcould overflow on 32-bit systems- Variable name has a typo ('paresIndex')
- paresIndex, err := strconv.ParseInt(attribute.Value, 10, 64) - if err != nil { - return msgIndex, err - } - msgIndex = int(paresIndex) + parsedIndex, err := strconv.Atoi(attribute.Value) + if err != nil { + return msgIndex, err + } + msgIndex = parsedIndex
104-104: Define constant for sentinel value.The magic number
-1is used multiple times as a sentinel value. Consider defining it as a constant for better maintainability.+const msgIndexNotFound = -1 + func findMsgIndex(txInfo *sdk.TxResponse, params types.QueryRequestContextRequest) (int, error) { - msgIndex := -1 + msgIndex := msgIndexNotFound // ... rest of the function - return -1, fmt.Errorf("unknown request context: %s", params.RequestContextId) + return msgIndexNotFound, fmt.Errorf("unknown request context: %s", params.RequestContextId)Also applies to: 130-131, 148-149
133-133: Document version compatibility.The comment "Compatible with older versions" could be more specific about which versions are supported and why this compatibility is needed.
- // Compatible with older versions. + // Handle logs format from pre-v1.x versions where events were stored in transaction logs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/service/client/utils/query.go(2 hunks)
🔇 Additional comments (1)
modules/service/client/utils/query.go (1)
69-72: LGTM! Clean refactoring of message index lookup.The extraction of the complex event searching logic into
findMsgIndeximproves code readability while maintaining the same behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
modules/service/client/utils/query.go (4)
112-113: Fix typo in variable name.The variable name
paresIndexappears to be a typo and should beparseIndex.- paresIndex, err := strconv.ParseInt(attribute.Value, 10, 64) + parseIndex, err := strconv.ParseInt(attribute.Value, 10, 64)
108-129: Simplify nested loops with early returns.The deeply nested loops with multiple conditions can be simplified using early returns. This would improve readability and reduce cognitive complexity.
if txInfo.Logs == nil { - for _, event := range txInfo.Events { - if event.Type == types.EventTypeCreateContext { - for _, attribute := range event.Attributes { - if attribute.Key == types.EventTypeMsgIndex { - parseIndex, err := strconv.ParseInt(attribute.Value, 10, 64) - if err != nil { - return msgIndex, err - } - msgIndex = int(parseIndex) - } - - if attribute.Key == types.AttributeKeyRequestContextID && - attribute.Value == params.RequestContextId { - found = true - } - - if found && msgIndex != -1 { - return msgIndex, nil - } - } - } - } + for _, event := range txInfo.Events { + if event.Type != types.EventTypeCreateContext { + continue + } + + for _, attribute := range event.Attributes { + switch attribute.Key { + case types.EventTypeMsgIndex: + parseIndex, err := strconv.ParseInt(attribute.Value, 10, 64) + if err != nil { + return msgIndex, err + } + msgIndex = int(parseIndex) + if found { + return msgIndex, nil + } + case types.AttributeKeyRequestContextID: + if attribute.Value == params.RequestContextId { + found = true + if msgIndex != -1 { + return msgIndex, nil + } + } + } + } + }
134-147: Extract older version compatibility logic into a separate function.The compatibility logic for older versions should be extracted into a separate function for better maintainability and clarity.
+func findMsgIndexFromLogs(txInfo *sdk.TxResponse, params types.QueryRequestContextRequest) (int, error) { + for i, log := range txInfo.Logs { + for _, event := range log.Events { + if event.Type != types.EventTypeCreateContext { + continue + } + + for _, attribute := range event.Attributes { + if attribute.Key == types.AttributeKeyRequestContextID && + attribute.Value == params.RequestContextId { + return i, nil + } + } + } + } + return -1, fmt.Errorf("unknown request context: %s", params.RequestContextId) +} func findMsgIndex(txInfo *sdk.TxResponse, params types.QueryRequestContextRequest) (int, error) { const errUnknownContext = "unknown request context: %s" if txInfo.Logs == nil { // ... existing direct events logic ... } // Compatible with older versions. - for i, log := range txInfo.Logs { - // ... existing logs logic ... - } - return msgIndex, fmt.Errorf(errUnknownContext, params.RequestContextId) + return findMsgIndexFromLogs(txInfo, params) }
102-150: Add documentation for version compatibility.The function handles two different versions but lacks documentation explaining when each code path is used. Add comments explaining the version differences and when each path is taken.
+// findMsgIndex searches for the message index in transaction information using two methods: +// 1. Direct events (newer versions): When txInfo.Logs is nil, searches through txInfo.Events +// 2. Logs-based events (older versions): When txInfo.Logs is present, searches through the logs +// +// The function returns the found message index or -1 with an error if the context is not found. func findMsgIndex(txInfo *sdk.TxResponse, params types.QueryRequestContextRequest) (int, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/service/client/utils/query.go(2 hunks)
🔇 Additional comments (1)
modules/service/client/utils/query.go (1)
69-72: Clean refactoring of event search logic!Good job extracting the complex event searching logic into a separate function. This improves readability and maintainability.
Summary by CodeRabbit