refactor(filter): rename chainsync to cardano#575
Conversation
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
📝 WalkthroughWalkthroughThis change replaces the ChainSync filter plugin with a new Cardano filter plugin. The Cardano filter is implemented with event processing and multi-layered filtering capabilities for block and transaction events, supporting address-based, policy ID, asset fingerprint, and pool ID filtering. The implementation includes lifecycle management (Start/Stop), channel-based I/O, and graceful shutdown handling. The ChainSync plugin and its option builders are completely removed, and the plugin registration is updated to load the new Cardano filter instead. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
filter/cardano/cardano_test.go (1)
221-223: Fix "Basic address match" test case to use a real address or certificate.The
mockAddressfunction always returns an emptycommon.Address{}regardless of its input parameter. At line 238-243, the "Basic address match" test expectsmockAddress("addr_test1qqjwq357")to produce an output matching the filter"addr_test1qqjwq357", but since no certificate is provided, the address string comparison at line 162 of the filter logic will fail (the empty address'sString()method won't return"addr_test1qqjwq357").Either provide a certificate for this test case (like the subsequent tests do) or implement
mockAddressto return an address whoseString()method returns the expected value.
🤖 Fix all issues with AI agents
In @filter/cardano/cardano.go:
- Around line 284-301: Stop() currently closes inputChan which can panic if
external goroutines send to InputChan() concurrently; remove closing of
c.inputChan and instead set c.inputChan = nil after acquiring stopOnce (close
only doneChan/outputChan/errorChan as before), ensuring the accessor InputChan()
will return nil post-stop; update Stop() to nil out the channel fields
(c.inputChan, c.outputChan, c.errorChan, c.doneChan) after closing the ones you
do close so TestCardano_Stop still observes nil values and avoid closing
inputChan to prevent sender panics.
🧹 Nitpick comments (4)
filter/cardano/cardano_test.go (1)
155-168: Consider starting the filter before stopping in this test.The test calls
Stop()without first callingStart(). While the current implementation handles this gracefully (channels are nil,stopOnceprevents issues), this doesn't test the expected lifecycle. Consider adding aStart()call first to verify proper shutdown behavior.♻️ Suggested improvement
func TestCardano_Stop(t *testing.T) { c := New() + err := c.Start() + if err != nil { + t.Fatalf("expected no error on start, got %v", err) + } err := c.Stop() if err != nil { t.Fatalf("expected no error, got %v", err) }filter/cardano/option.go (1)
45-59: Consider logging bech32 decoding failures.When bech32 decoding fails (lines 49-52), the error is silently ignored. While this may be intentional (gracefully handling invalid input), logging a warning could help users diagnose configuration issues with malformed stake addresses.
♻️ Optional: Add warning for decode failures
// Pre-decode the bech32 to get credential hash if _, data, err := bech32.Decode(addr); err == nil { if decoded, err := bech32.ConvertBits(data, 5, 8, false); err == nil { c.filterSet.addresses.stakeCredentialHashes[addr] = decoded } + } else if c.logger != nil { + c.logger.Warn("failed to decode stake address", "address", addr, "error", err) }filter/cardano/plugin.go (1)
82-113: Consider trimming whitespace from comma-separated values.When users provide comma-separated values with spaces (e.g.,
"addr1, addr2"), the spaces will be included in the parsed values, potentially causing silent filter failures. Consider trimming whitespace.♻️ Helper function to split and trim
func splitAndTrim(s string) []string { parts := strings.Split(s, ",") result := make([]string, 0, len(parts)) for _, p := range parts { if trimmed := strings.TrimSpace(p); trimmed != "" { result = append(result, trimmed) } } return result }Then use
splitAndTrim(cmdlineOptions.address)instead ofstrings.Split(cmdlineOptions.address, ",").filter/cardano/cardano.go (1)
152-186: Minor: Avoid repeated slice allocation in match functions.
append(te.Outputs, te.ResolvedInputs...)allocates a new slice on each call. SincematchAddressFilter,matchPolicyFilter, andmatchAssetFilterall do this, consider iterating both slices separately to avoid allocations in the hot path.♻️ Alternative: iterate both slices
func (c *Cardano) matchAddressFilter(te event.TransactionEvent) bool { - // Include resolved inputs as outputs for matching - allOutputs := append(te.Outputs, te.ResolvedInputs...) - - // Check outputs against payment and stake addresses - for _, output := range allOutputs { + // Check outputs and resolved inputs against payment and stake addresses + for _, outputs := range [][]ledger.TransactionOutput{te.Outputs, te.ResolvedInputs} { + for _, output := range outputs { addrStr := output.Address().String() // ... rest of logic + } + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
filter/cardano/cardano.gofilter/cardano/cardano_test.gofilter/cardano/filter_types.gofilter/cardano/option.gofilter/cardano/plugin.gofilter/cardano/plugin_test.gofilter/chainsync/chainsync.gofilter/chainsync/option.gofilter/filter.go
💤 Files with no reviewable changes (2)
- filter/chainsync/chainsync.go
- filter/chainsync/option.go
🧰 Additional context used
🧬 Code graph analysis (5)
filter/cardano/plugin_test.go (1)
plugin/plugin.go (1)
Plugin(23-29)
filter/cardano/option.go (2)
filter/cardano/cardano.go (1)
Cardano(28-36)plugin/log.go (1)
Logger(4-13)
filter/cardano/cardano.go (4)
plugin/log.go (1)
Logger(4-13)filter/cardano/option.go (1)
CardanoOptionFunc(25-25)event/block.go (1)
BlockEvent(28-35)event/tx.go (1)
TransactionEvent(30-44)
filter/cardano/cardano_test.go (3)
filter/cardano/cardano.go (1)
New(39-45)filter/cardano/option.go (1)
WithPoolIds(63-100)event/block.go (1)
BlockEvent(28-35)
filter/cardano/plugin.go (3)
plugin/option.go (2)
PluginOption(35-43)PluginOptionTypeString(29-29)filter/cardano/option.go (2)
CardanoOptionFunc(25-25)WithLogger(28-32)internal/logging/logging.go (1)
GetLogger(60-65)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
filter/filter.go (1)
18-21: LGTM!Clean import swap to register the new Cardano filter plugin instead of ChainSync. The side-effect import pattern for auto-registration is preserved.
filter/cardano/plugin_test.go (2)
27-48: LGTM!Plugin registration test correctly updated to look for the "cardano" plugin name. The test logic is sound.
87-121: LGTM!The event processing test correctly verifies that events pass through the filter when no filters are configured. The timeout handling is appropriate.
filter/cardano/filter_types.go (1)
17-52: LGTM!Well-designed internal data structures for O(1) lookups. Using
map[string]struct{}for set semantics is idiomatic Go. The pre-computed credential hashes and dual hex/bech32 pool ID representations enable efficient matching without runtime conversions.filter/cardano/cardano_test.go (1)
426-458: LGTM!Pool ID filter test correctly updated to use hex format, aligning with the Cardano filter's O(1) lookup implementation. The test verifies that block events are filtered based on
IssuerVkeymatching the configured pool ID.filter/cardano/option.go (2)
62-100: LGTM!Pool ID option correctly handles both hex and bech32 formats, pre-computing both representations for O(1) lookups. The bidirectional mapping enables efficient matching regardless of the format used in events.
102-130: LGTM!Simple and efficient set-based storage for policy IDs and asset fingerprints. The lazy initialization pattern (
if c.filterSet.X == nil) is consistently applied.filter/cardano/plugin.go (1)
31-74: LGTM!Plugin registration correctly updated with new name "cardano", updated description, and enhanced option descriptions indicating comma-separated multi-value support. The change maintains backward compatibility with the same option flags.
filter/cardano/cardano.go (3)
119-150: Verify AND filter logic is intentional.The current implementation uses AND logic: a transaction must match all configured filters to pass through. For example, if both address and policy filters are set, the transaction must match both.
If OR semantics are intended (match any filter), the logic would need adjustment. Please confirm this is the expected behavior.
28-56: LGTM!Clean struct design with proper channel types and lifecycle fields. The
sync.Oncefor stop prevents double-close panics. Buffered channels (size 10) provide reasonable backpressure handling.
94-117: LGTM!Block event filtering correctly checks the issuer vkey against all pre-computed pool ID representations with O(1) lookups.
| // Stop the cardano filter | ||
| func (c *Cardano) Stop() error { | ||
| c.stopOnce.Do(func() { | ||
| if c.doneChan != nil { | ||
| close(c.doneChan) | ||
| } | ||
| if c.inputChan != nil { | ||
| close(c.inputChan) | ||
| } | ||
| if c.outputChan != nil { | ||
| close(c.outputChan) | ||
| } | ||
| if c.errorChan != nil { | ||
| close(c.errorChan) | ||
| } | ||
| }) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Closing inputChan may cause sender panic.
If an external goroutine sends to InputChan() while or after Stop() closes the channel, it will panic. Consider either:
- Not closing
inputChaninStop()(let senders manage lifecycle) - Using a mutex or atomic flag to prevent sends after stop
- Documenting that callers must stop sending before calling
Stop()
🔧 Alternative: nil out channels after close to cause panic on accessor instead
A safer pattern is to set channels to nil after closing, so that subsequent calls to InputChan() return nil (which will also panic on send, but is more debuggable):
func (c *Cardano) Stop() error {
c.stopOnce.Do(func() {
if c.doneChan != nil {
close(c.doneChan)
+ c.doneChan = nil
}
if c.inputChan != nil {
close(c.inputChan)
+ c.inputChan = nil
}
if c.outputChan != nil {
close(c.outputChan)
+ c.outputChan = nil
}
if c.errorChan != nil {
close(c.errorChan)
+ c.errorChan = nil
}
})
return nil
}Note: The test TestCardano_Stop already expects channels to be nil after stop, but the current implementation doesn't set them to nil.
📝 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.
| // Stop the cardano filter | |
| func (c *Cardano) Stop() error { | |
| c.stopOnce.Do(func() { | |
| if c.doneChan != nil { | |
| close(c.doneChan) | |
| } | |
| if c.inputChan != nil { | |
| close(c.inputChan) | |
| } | |
| if c.outputChan != nil { | |
| close(c.outputChan) | |
| } | |
| if c.errorChan != nil { | |
| close(c.errorChan) | |
| } | |
| }) | |
| return nil | |
| } | |
| // Stop the cardano filter | |
| func (c *Cardano) Stop() error { | |
| c.stopOnce.Do(func() { | |
| if c.doneChan != nil { | |
| close(c.doneChan) | |
| c.doneChan = nil | |
| } | |
| if c.inputChan != nil { | |
| close(c.inputChan) | |
| c.inputChan = nil | |
| } | |
| if c.outputChan != nil { | |
| close(c.outputChan) | |
| c.outputChan = nil | |
| } | |
| if c.errorChan != nil { | |
| close(c.errorChan) | |
| c.errorChan = nil | |
| } | |
| }) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In @filter/cardano/cardano.go around lines 284 - 301, Stop() currently closes
inputChan which can panic if external goroutines send to InputChan()
concurrently; remove closing of c.inputChan and instead set c.inputChan = nil
after acquiring stopOnce (close only doneChan/outputChan/errorChan as before),
ensuring the accessor InputChan() will return nil post-stop; update Stop() to
nil out the channel fields (c.inputChan, c.outputChan, c.errorChan, c.doneChan)
after closing the ones you do close so TestCardano_Stop still observes nil
values and avoid closing inputChan to prevent sender panics.
2 participants
Summary by cubic
Renamed the chainsync filter to cardano and optimized filtering with O(1) lookups and better hex/bech32 pool ID handling. This makes event filtering faster and clearer across addresses, policies, assets, and pools.
Refactors
Migration
Written for commit d1f58fe. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.