feat: implement confirmation blocks logic for event listening#17
Closed
feat: implement confirmation blocks logic for event listening#17
Conversation
📝 WalkthroughWalkthroughThese changes introduce blockchain confirmation block depth configuration throughout the custody listener system. The system now accepts and propagates a Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service
participant Listener as Listener
participant Backend as Backend (Client)
participant Handler as Handler/Reconciliation
Service->>Listener: NewListener(confirmationBlocks=N)
Listener->>Listener: Store confirmationBlocks field
Listener->>Backend: SubscribeNewHead(ctx, headerCh)
Backend-->>Listener: Subscription + header channel
loop On New Block Header
Backend->>Listener: Send header on channel
Listener->>Listener: Calculate targetBlock = currentHead - N
Listener->>Handler: reconcileBlockRange(targetBlock)
Handler->>Handler: Process logs up to targetBlock
Listener->>Listener: Update lastBlock & lastIndex
end
Listener->>Listener: Handle subscription errors with backoff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
🧹 Nitpick comments (2)
service/integration_test.go (1)
125-126: Remove stale inline TODO-style comment in send path.These lines imply missing quit/context handling, but the
selectdirectly below already handles both. Keeping this comment can mislead future edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration_test.go` around lines 125 - 126, Remove the stale inline TODO-style comment that suggests missing quit/context handling in the send path; the select immediately below already handles both channel send and quit, so delete the two comment lines near the send logic (the lines referencing "Check if channel is closed or full?" and "Ideally we should select on quit as well while sending") to avoid misleading future readers and keep the send path in the function where the select is implemented (the send/send path surrounding the select block).custody/listener.go (1)
229-232: Extract target-block calculation to one helper.The same
currentHead -> targetBlocklogic appears twice; pulling it into a helper reduces drift risk and makes confirmation semantics easier to audit.Also applies to: 259-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custody/listener.go` around lines 229 - 232, Extract the repeated calculation of targetBlock into a small helper (e.g., computeTargetBlock or targetBlockFor) that takes currentHead and confirmationBlocks (both uint64) and returns uint64: if currentHead > confirmationBlocks return currentHead - confirmationBlocks else return 0; replace both occurrences that set targetBlock from currentHead and confirmationBlocks (the blocks that currently assign targetBlock using currentHead/confirmationBlocks) with a call to this helper so confirmation semantics live in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 23-27: BlockchainConfig.Validate() currently doesn't check
ConfirmationBlocks, so a missing/zero value silently disables confirmation
depth; update the BlockchainConfig.Validate() method to reject zero (and
negative if applicable) by returning an error when ConfirmationBlocks == 0 (or <
1), and include a clear error message referencing ConfirmationBlocks and
expected minimum (e.g., "ConfirmationBlocks must be >= 1"); ensure any callers
that load/parse config propagate/handle that validation error.
In `@custody/listener.go`:
- Around line 247-249: The bug is that after a full reconciliation the code
resets lastIndex to 0 which causes logs in the same lastBlock to be reprocessed;
instead, when advancing the cursor (around variables lastBlock, lastIndex and
targetBlock) set lastIndex to the position after the last processed log in that
block (e.g., lastIndex = lastProcessedIndex + 1 or lastIndex = targetIndex + 1)
so the next pass skips already-delivered entries; apply the same fix to the
analogous block at the other occurrence (the section around lines 277-279) so
both advancement paths move the index forward rather than back to 0.
---
Nitpick comments:
In `@custody/listener.go`:
- Around line 229-232: Extract the repeated calculation of targetBlock into a
small helper (e.g., computeTargetBlock or targetBlockFor) that takes currentHead
and confirmationBlocks (both uint64) and returns uint64: if currentHead >
confirmationBlocks return currentHead - confirmationBlocks else return 0;
replace both occurrences that set targetBlock from currentHead and
confirmationBlocks (the blocks that currently assign targetBlock using
currentHead/confirmationBlocks) with a call to this helper so confirmation
semantics live in one place.
In `@service/integration_test.go`:
- Around line 125-126: Remove the stale inline TODO-style comment that suggests
missing quit/context handling in the send path; the select immediately below
already handles both channel send and quit, so delete the two comment lines near
the send logic (the lines referencing "Check if channel is closed or full?" and
"Ideally we should select on quit as well while sending") to avoid misleading
future readers and keep the send path in the function where the select is
implemented (the send/send path surrounding the select block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e276ef93-6918-4781-9ef7-c98941ea913b
📒 Files selected for processing (4)
config/config.gocustody/listener.goservice/integration_test.goservice/service.go
Comment on lines
+23
to
27
| RPCURL string `yaml:"rpc_url"` | ||
| ContractAddr string `yaml:"contract_address"` | ||
| PrivateKey string `yaml:"private_key"` | ||
| ConfirmationBlocks uint64 `yaml:"confirmation_blocks"` | ||
| } |
There was a problem hiding this comment.
Validate confirmation_blocks so zero doesn’t silently disable confirmation depth.
Line 26 introduces ConfirmationBlocks, but BlockchainConfig.Validate() does not guard it. With an omitted config value, this defaults to 0 and the listener processes unconfirmed head blocks.
🔧 Suggested fix
func (c BlockchainConfig) Validate() error {
if c.RPCURL == "" {
return errors.New("missing blockchain RPC URL")
}
...
if c.PrivateKey == "" {
return errors.New("missing private key")
}
+ if c.ConfirmationBlocks == 0 {
+ return errors.New("confirmation_blocks must be greater than 0")
+ }
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/config.go` around lines 23 - 27, BlockchainConfig.Validate() currently
doesn't check ConfirmationBlocks, so a missing/zero value silently disables
confirmation depth; update the BlockchainConfig.Validate() method to reject zero
(and negative if applicable) by returning an error when ConfirmationBlocks == 0
(or < 1), and include a clear error message referencing ConfirmationBlocks and
expected minimum (e.g., "ConfirmationBlocks must be >= 1"); ensure any callers
that load/parse config propagate/handle that validation error.
Comment on lines
+247
to
249
| lastBlock = targetBlock | ||
| lastIndex = 0 | ||
| } |
There was a problem hiding this comment.
Cursor advancement replays logs from the previously reconciled block.
After a full reconciliation, resetting lastIndex to 0 means the next pass can re-handle same-block logs with index > 0, causing duplicate deliveries.
🔧 Suggested fix
- lastBlock = targetBlock
- lastIndex = 0
+ lastBlock = targetBlock
+ // Full block has been reconciled; skip it entirely on next pass.
+ lastIndex = ^uint32(0)
...
- lastBlock = targetBlock
- lastIndex = 0
+ lastBlock = targetBlock
+ // Full block has been reconciled; skip it entirely on next pass.
+ lastIndex = ^uint32(0)Also applies to: 277-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@custody/listener.go` around lines 247 - 249, The bug is that after a full
reconciliation the code resets lastIndex to 0 which causes logs in the same
lastBlock to be reprocessed; instead, when advancing the cursor (around
variables lastBlock, lastIndex and targetBlock) set lastIndex to the position
after the last processed log in that block (e.g., lastIndex = lastProcessedIndex
+ 1 or lastIndex = targetIndex + 1) so the next pass skips already-delivered
entries; apply the same fix to the analogous block at the other occurrence (the
section around lines 277-279) so both advancement paths move the index forward
rather than back to 0.
Rework the event listener to subscribe to new block headers (SubscribeNewHead) instead of SubscribeFilterLogs, then reconcile logs per block range. This makes the listener resilient to WebSocket disconnections (e.g. Infura error 1006). The service goroutine now loops: on first run it uses the existing client; on subsequent iterations it re-dials via RPCURL, re-binds the contract, and resumes from the persisted cursor. When RPCURL is empty (simulated backend in tests) it exits cleanly instead of attempting to dial. Also fixes two bugs in waitForBackOffTimeout: - return false (not true) when max retries exceeded - use 1<<n (bit-shift) instead of 2^n (XOR) for exponential backoff
calj
force-pushed
the
feat/confirmation-blocks
branch
from
62f3ac4 to
d15b864
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements