Conversation
WalkthroughUpdates default rate-limit values, switches queue broadcasts to synchronous, adds parsing/retry for account-sequence-mismatch errors, introduces tests for the parser, and refactors stray-manager pagination and internal types (including changing lastSize to uint64 and safer slice population). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Queue.BroadcastPending as BroadcastPending
participant Extract as extractExpectedSequence
participant RetryLoop as Retry
Caller->>BroadcastPending: BroadcastPending(tx, account)
rect rgb(220,240,255)
Note over BroadcastPending: Send via BroadcastTxSync
BroadcastPending->>BroadcastPending: Call RPC BroadcastTxSync
end
alt RPC returns sequence-mismatch error
BroadcastPending->>Extract: extractExpectedSequence(err.Error())
alt extraction success
Extract-->>BroadcastPending: expectedSequence (uint64), true
BroadcastPending->>BroadcastPending: set account sequence = expectedSequence
else extraction failed
Extract-->>BroadcastPending: 0, false
BroadcastPending->>BroadcastPending: increment sequence (fallback)
end
BroadcastPending->>RetryLoop: retry broadcast with updated sequence
else RPC success
BroadcastPending->>BroadcastPending: check res.RawLog for "account sequence mismatch"
alt RawLog contains mismatch
BroadcastPending->>Extract: extractExpectedSequence(res.RawLog)
alt extraction success
Extract-->>BroadcastPending: expectedSequence
BroadcastPending->>BroadcastPending: set sequence = expectedSequence
else
Extract-->>BroadcastPending: 0,false
BroadcastPending->>BroadcastPending: increment sequence
end
end
BroadcastPending-->>Caller: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
config/types.go(1 hunks)queue/queue.go(5 hunks)queue/queue_test.go(1 hunks)
🔇 Additional comments (5)
config/types.go (1)
70-72: Verify rate limit changes and clarify incomplete broadcast migration.The rate limiting defaults have been significantly relaxed (4x faster token generation, 3x larger burst capacity). Additionally, the codebase shows mixed broadcast implementations:
queue/queue.gousesBroadcastTxSync, butcore/app.gostill usesBroadcastTxCommitin three locations (lines 133, 155, 177), suggesting an incomplete migration.The sequence mismatch handling mentioned in the summary is already implemented in
queue/queue.go(lines 246–248, 263–265).Please verify:
- Are these rate limit values (100ms per token, burst of 30) appropriate for the expected load and tested under realistic conditions?
- Is the incomplete
BroadcastTxCommitusage incore/app.gointentional or does it need migration toBroadcastTxSyncfor consistency?queue/queue_test.go (2)
110-131: LGTM! Comprehensive edge case coverage.The edge case tests appropriately verify that the extraction handles whitespace variations, newlines, and long messages correctly.
84-88: No issues found—the comma-formatted test case is documented edge-case handling.The regex pattern
expected\s+(\d+)(queue.go:30) intentionally matches only contiguous digits. All real Cosmos SDK v0.45.17 error messages in the test suite (lines 19, 25, 112) use non-comma format ("expected 1471614"). The test case at lines 84–88 with comma formatting is explicitly documented as a known limitation: "Regex will match the first digit sequence '1'". This shows developers are aware of the edge case and it does not represent a production concern.queue/queue.go (2)
146-147: LGTM! Simplified polling logic.The 100ms sleep interval is reasonable for queue polling, and the simplified time check improves readability.
234-234: Verify the broadcast method change aligns with system requirements.The switch from
BroadcastTxCommittoBroadcastTxSyncis a significant behavioral change.BroadcastTxSyncreturns after passingCheckTxwithout waiting for block inclusion, making it faster but providing less certainty about transaction finality.Please confirm:
- The system correctly handles the asynchronous nature of
BroadcastTxSync- There are appropriate mechanisms to track transaction inclusion in blocks
- Error handling accounts for transactions that pass
CheckTxbut fail in block executionYou may also want to verify that downstream systems don't depend on the finality guarantees that
BroadcastTxCommitprovided.
| // extractExpectedSequence extracts the expected sequence number from an account sequence mismatch error message. | ||
| // It looks for the pattern "expected <number>" in the error message, allowing for optional whitespace. | ||
| // Returns the expected sequence number and true if found, or 0 and false if not found. | ||
| func extractExpectedSequence(errorMsg string) (uint64, bool) { | ||
| re := regexp.MustCompile(`expected\s+(\d+)`) | ||
| matches := re.FindStringSubmatch(errorMsg) | ||
| if len(matches) > 1 { | ||
| if expectedSeq, parseErr := strconv.ParseUint(matches[1], 10, 64); parseErr == nil { | ||
| return expectedSeq, true | ||
| } | ||
| } | ||
| return 0, false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optimize regex compilation for better performance.
The regex is compiled on every function call, which is inefficient. Consider moving the compilation to a package-level variable.
Apply this diff to optimize the regex compilation:
+var (
+ sequenceRegex = regexp.MustCompile(`expected\s+(\d+)`)
+)
+
// extractExpectedSequence extracts the expected sequence number from an account sequence mismatch error message.
// It looks for the pattern "expected <number>" in the error message, allowing for optional whitespace.
// Returns the expected sequence number and true if found, or 0 and false if not found.
func extractExpectedSequence(errorMsg string) (uint64, bool) {
- re := regexp.MustCompile(`expected\s+(\d+)`)
- matches := re.FindStringSubmatch(errorMsg)
+ matches := sequenceRegex.FindStringSubmatch(errorMsg)
if len(matches) > 1 {
if expectedSeq, parseErr := strconv.ParseUint(matches[1], 10, 64); parseErr == nil {
return expectedSeq, true
}
}
return 0, false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // extractExpectedSequence extracts the expected sequence number from an account sequence mismatch error message. | |
| // It looks for the pattern "expected <number>" in the error message, allowing for optional whitespace. | |
| // Returns the expected sequence number and true if found, or 0 and false if not found. | |
| func extractExpectedSequence(errorMsg string) (uint64, bool) { | |
| re := regexp.MustCompile(`expected\s+(\d+)`) | |
| matches := re.FindStringSubmatch(errorMsg) | |
| if len(matches) > 1 { | |
| if expectedSeq, parseErr := strconv.ParseUint(matches[1], 10, 64); parseErr == nil { | |
| return expectedSeq, true | |
| } | |
| } | |
| return 0, false | |
| } | |
| var ( | |
| sequenceRegex = regexp.MustCompile(`expected\s+(\d+)`) | |
| ) | |
| // extractExpectedSequence extracts the expected sequence number from an account sequence mismatch error message. | |
| // It looks for the pattern "expected <number>" in the error message, allowing for optional whitespace. | |
| // Returns the expected sequence number and true if found, or 0 and false if not found. | |
| func extractExpectedSequence(errorMsg string) (uint64, bool) { | |
| matches := sequenceRegex.FindStringSubmatch(errorMsg) | |
| if len(matches) > 1 { | |
| if expectedSeq, parseErr := strconv.ParseUint(matches[1], 10, 64); parseErr == nil { | |
| return expectedSeq, true | |
| } | |
| } | |
| return 0, false | |
| } |
🤖 Prompt for AI Agents
In queue/queue.go around lines 26 to 38, the regular expression is being
compiled on every call to extractExpectedSequence which hurts performance; fix
it by moving regexp.MustCompile(`expected\s+(\d+)`) to a package-level variable
(e.g., var expectedSeqRe = regexp.MustCompile(...)) with a brief comment, then
update extractExpectedSequence to use that precompiled
expectedSeqRe.FindStringSubmatch(errorMsg) and keep the same parsing and return
logic.
| if strings.Contains(err.Error(), "account sequence mismatch") { | ||
| if expectedSeq, found := extractExpectedSequence(err.Error()); found { | ||
| data = data.WithSequence(expectedSeq) | ||
| continue | ||
| } | ||
| // Fallback to incrementing if extraction fails | ||
| if data.Sequence != nil { | ||
| data = data.WithSequence(*data.Sequence + 1) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting duplicated sequence mismatch handling logic.
The sequence mismatch handling logic is duplicated between the error path (lines 246-256) and the response path (lines 263-273). This reduces maintainability and increases the risk of inconsistencies if the logic needs to be updated.
Consider refactoring into a helper function:
// handleSequenceMismatch attempts to recover from a sequence mismatch by extracting
// the expected sequence from the error message or incrementing the current sequence.
// Returns updated TransactionData and true if recovery was attempted, or original data and false otherwise.
func handleSequenceMismatch(data walletTypes.TransactionData, errorMsg string) (walletTypes.TransactionData, bool) {
if expectedSeq, found := extractExpectedSequence(errorMsg); found {
return data.WithSequence(expectedSeq), true
}
// Fallback to incrementing if extraction fails
if data.Sequence != nil {
return data.WithSequence(*data.Sequence + 1), true
}
return data, false
}Then use it in both places:
// In error path:
if strings.Contains(err.Error(), "account sequence mismatch") {
if newData, handled := handleSequenceMismatch(data, err.Error()); handled {
data = newData
continue
}
}
// In response path:
if strings.Contains(res.RawLog, "account sequence mismatch") {
if newData, handled := handleSequenceMismatch(data, res.RawLog); handled {
data = newData
continue
}
}Also applies to: 263-273
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
strays/manager.go(3 hunks)strays/types.go(1 hunks)
⏰ 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: lint
1 participant
Switching to sync to make things better again.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests