Conversation
WalkthroughReplaced plugin.Plugin with plugin.Spec across server and plugins, added stub Suggest methods to plugins, updated tx_indexer import paths and changed Chains() to return (chains, error) with callers handling errors, and bumped some go.mod dependencies and cron from indirect to direct. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as cmd/* (main)
participant TXI as plugin/tx_indexer
participant Svc as txIndexerService
Main->>TXI: Chains()
alt error
TXI-->>Main: (nil, err)
Main->>Main: panic(err)
else success
TXI-->>Main: (chains, nil)
Main->>Svc: New(..., chains, ...)
Svc-->>Main: service instance
end
note right of TXI: Chains() now returns (chains, error)
sequenceDiagram
autonumber
participant Caller as Server/Runtime
participant Plug as plugin.Spec (fees/payroll)
participant Types as recipes/types
Caller->>Plug: Suggest(configuration)
alt current (unimplemented)
Plug-->>Caller: (nil, error("unimplemented"))
else implemented
Plug->>Types: construct PolicySuggest
Types-->>Plug: *PolicySuggest
Plug-->>Caller: *PolicySuggest, nil
end
note right of Plug: Plugins now implement Suggest(...) per plugin.Spec
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/fees/fees.go (2)
139-150: Goroutine captures loop variable incorrectly; use short declaration to avoid racesInside the loop,
feePolicy = feePolicydoes not create a new variable; closures will capture the loop variable and may all observe the last item. This can load or operate on the wrong policy.Fix by shadowing with short declaration:
- feePolicy = feePolicy + feePolicy := feePolicy eg.Go(func() error { defer wg.Done() if err := sem.Acquire(ctx, 1); err != nil { return fmt.Errorf("failed to acquire semaphore: %w", err) } defer sem.Release(1) return fp.executeFeeLoading(ctx, feePolicy) })
265-272: Same closure-capture bug for run variable in HandleTransactions
run = rundoes not rebind; closures can see the wrong run. Shadow the loop variable.- run = run + run := run eg.Go(func() error { //TODO also check failed runs if run.Status != types.FeeRunStateDraft { return nil }
🧹 Nitpick comments (9)
cmd/payroll/worker/main.go (2)
13-14: Prefer an explicit alias for tx_indexer storage to avoid confusion with plugin/storageTo improve readability and avoid mixing with plugin/storage packages, alias the tx_indexer storage import like in the server main.
Apply:
- "github.com/vultisig/verifier/plugin/tx_indexer" - "github.com/vultisig/verifier/plugin/tx_indexer/pkg/storage" + "github.com/vultisig/verifier/plugin/tx_indexer" + tx_indexer_storage "github.com/vultisig/verifier/plugin/tx_indexer/pkg/storage"Also update usage accordingly (outside this hunk):
// line 59 currently: txIndexerStore, err := storage.NewPostgresTxIndexStore(ctx, cfg.Database.DSN) // change to: txIndexerStore, err := tx_indexer_storage.NewPostgresTxIndexStore(ctx, cfg.Database.DSN)
64-69: Fail fast with structured logging instead of panicUse logger.Fatalf to emit a structured message and exit with non-zero status.
- chains, err := tx_indexer.Chains() - if err != nil { - // Handle the error appropriately - either return it or log it - panic(fmt.Errorf("failed to initialize tx indexer chains: %w", err)) - } + chains, err := tx_indexer.Chains() + if err != nil { + logger.Fatalf("failed to initialize tx indexer chains: %v", err) + }cmd/payroll/server/main.go (1)
65-70: Use logger.Fatalf over panic for init failureConsistent structured logging on startup issues improves observability and exits cleanly.
- chains, err := tx_indexer.Chains() - if err != nil { - // Handle the error appropriately - either return it or log it - panic(fmt.Errorf("failed to initialize tx indexer chains: %w", err)) - } + chains, err := tx_indexer.Chains() + if err != nil { + logger.Fatalf("failed to initialize tx indexer chains: %v", err) + }cmd/fees/worker/main.go (3)
116-121: Initialize chains with error handling — remove leftover commentThe error handling is fine (consistent with the rest of main), but the inline comment looks like a leftover TODO.
Apply this minimal cleanup:
- // Handle the error appropriately - either return it or log it panic(fmt.Errorf("failed to initialize tx indexer chains: %w", err))
188-191: Check cron AddFunc errors to avoid silently misconfigured schedulesrobfig/cron AddFunc returns an EntryID and an error; ignoring the error can leave jobs unscheduled at runtime if cronexpr is invalid.
Example adjustments:
- loadFees.AddFunc(feePluginConfig.Jobs.Load.Cronexpr, func() { + if _, err := loadFees.AddFunc(feePluginConfig.Jobs.Load.Cronexpr, func() { startLoadingFees(asynqClient, logger) - }) + }); err != nil { + logger.WithError(err).Fatal("failed to schedule LoadFees cron") + }- transactFees.AddFunc(feePluginConfig.Jobs.Transact.Cronexpr, func() { + if _, err := transactFees.AddFunc(feePluginConfig.Jobs.Transact.Cronexpr, func() { startTransactingFees(asynqClient, logger) - }) + }); err != nil { + logger.WithError(err).Fatal("failed to schedule TransactFees cron") + }- updateVerifier.AddFunc(feePluginConfig.Jobs.Post.Cronexpr, func() { + if _, err := updateVerifier.AddFunc(feePluginConfig.Jobs.Post.Cronexpr, func() { startPostTx(asynqClient, logger) - }) + }); err != nil { + logger.WithError(err).Fatal("failed to schedule PostTx cron") + }Also applies to: 195-198, 202-205
106-109: Nit: variable name typo “postgressDB” → “postgresDB”Minor naming polish for readability and consistency.
- postgressDB, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) + postgresDB, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)- postgressDB, + postgresDB,Also applies to: 158-169
plugin/payroll/payroll.go (1)
32-35: Avoid panicking in Suggest; return a sentinel error insteadA panic here will crash the process if Suggest is invoked. Prefer returning a not-implemented error to degrade gracefully.
Apply this change:
-func (p *Plugin) Suggest(configuration map[string]any) (*types.PolicySuggest, error) { - panic("unimplemented") -} +func (p *Plugin) Suggest(configuration map[string]any) (*types.PolicySuggest, error) { + return nil, errors.New("Suggest not implemented") +}And add the import (outside the selected range):
import ( "errors" // existing imports... )plugin/fees/fees.go (2)
54-57: Avoid panicking in Suggest; return a not-implemented errorLike payroll, panic would bring down the worker if Suggest is called. Prefer a graceful error.
Apply this change:
-func (fp *FeePlugin) Suggest(configuration map[string]any) (*rtypes.PolicySuggest, error) { - panic("unimplemented") -} +func (fp *FeePlugin) Suggest(configuration map[string]any) (*rtypes.PolicySuggest, error) { + return nil, errors.New("Suggest not implemented") +}And add the import (outside the selected range):
import ( "errors" // existing imports... )
135-154: Redundant WaitGroup alongside errgroupYou're using both errgroup and a separate WaitGroup in these flows. errgroup already waits for all spawned goroutines via
eg.Wait(). Keeping both adds noise and increases maintenance cost with little benefit.
- Remove wg.Add/Done/Wait and rely solely on errgroup.
- Keep the semaphore as-is to cap concurrency.
Also applies to: 286-306
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
api/server.go(2 hunks)cmd/fees/worker/main.go(2 hunks)cmd/payroll/server/main.go(2 hunks)cmd/payroll/worker/main.go(2 hunks)go.mod(2 hunks)plugin/fees/fees.go(4 hunks)plugin/payroll/payroll.go(2 hunks)plugin/payroll/transaction.go(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
📚 Learning: 2025-07-24T19:34:47.441Z
Learnt from: webpiratt
PR: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
Applied to files:
plugin/payroll/transaction.gocmd/payroll/worker/main.gocmd/payroll/server/main.goplugin/payroll/payroll.go
📚 Learning: 2025-05-07T08:23:45.882Z
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Applied to files:
plugin/payroll/transaction.gocmd/payroll/worker/main.gogo.modcmd/payroll/server/main.gocmd/fees/worker/main.goplugin/fees/fees.goplugin/payroll/payroll.go
📚 Learning: 2025-07-02T04:55:36.331Z
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Applied to files:
go.mod
📚 Learning: 2025-07-31T15:43:42.631Z
Learnt from: garry-sharp
PR: vultisig/plugin#146
File: plugin/fees/transaction.go:158-160
Timestamp: 2025-07-31T15:43:42.631Z
Learning: In the vultisig/plugin fees plugin, the public ProposeTransactions method intentionally returns "not implemented" because the fees plugin v2 uses a run-based architecture instead of the standard plugin template pattern. The actual transaction proposal logic is implemented in the private proposeTransactions method that accepts a types.FeeRun parameter, allowing fees to be batched and tracked through fee runs rather than individual policy-based transactions.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-04T10:47:47.927Z
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-10T20:41:44.025Z
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.025Z
Learning: The constant `vtypes.PluginVultisigFees_feee` in the vultisig/plugin project is correctly spelled with "feee" (including the extra 'e'). This is the actual constant name defined in the external dependency github.com/vultisig/verifier/types and should not be changed to "fee" singular, as this is how it's defined in the external package.
Applied to files:
plugin/fees/fees.go
📚 Learning: 2025-07-09T22:23:17.348Z
Learnt from: webpiratt
PR: vultisig/plugin#125
File: plugin/payroll/policy.go:160-160
Timestamp: 2025-07-09T22:23:17.348Z
Learning: In plugin/payroll/policy.go, the "token" case in the checkRule method intentionally does not call validateToken() as this is a temporary implementation that will be replaced with generic schema validation. The missing validation call is expected behavior during this transition period.
Applied to files:
plugin/payroll/payroll.go
🔇 Additional comments (15)
plugin/payroll/transaction.go (1)
27-27: Import path migrated to plugin/tx_indexer storage: LGTMThe storage import switch to github.com/vultisig/verifier/plugin/tx_indexer/pkg/storage aligns with the PR objectives and centralization. CreateTxDto usage remains intact. Also consistent with prior learning that verifier/plugin is the correct external source of plugin infrastructure.
api/server.go (1)
39-39: plugin.Plugin references removed and NewServer call sites updatedNo leftovers of the old
plugin.Plugininterface and all production call sites now pass aplugin.Specimplementer.
- api/server.go’s
NewServersignature expectsplugin.Spec(formerlyplugin.Plugin)rgconfirms zero matches forplugin.Plugin- cmd/fees/server/main.go passes
feePlugin(fees.FeePlugin{})- cmd/payroll/server/main.go passes
p(payroll plugin)- scripts/dev/create_fee_policy/dummy_server.go uses
nilas a deliberate placeholdergo.mod (2)
180-180: Promoted cron/v3 to a direct dependency: LGTMDirect require is appropriate given runtime usage by scheduling components. Remember to keep versions aligned across services; v3.0.1 is consistent with prior usage.
21-22: Deps bumped to match verifier/plugin migration—verify no stale imports & error handling
- All imports of tx_indexer now point to github.com/vultisig/verifier/tx_indexer. No leftover imports of the old local plugin tx_indexer path were found.
- Calls to tx_indexer.Chains() in payroll/worker and fees/worker correctly assign and handle the returned error.
- In cmd/fees/server/main.go, tx_indexer.Chains() is passed directly into a function call. Please confirm that the caller handles or propagates the error return instead of silently dropping it.
cmd/payroll/worker/main.go (1)
73-74: Passing pre-fetched chains into NewService: LGTMMatches the new Chains() API and avoids re-resolving chains in constructors.
cmd/payroll/server/main.go (2)
12-13: Import path updates to plugin/tx_indexer: LGTMConsistent with the migration. Using tx_indexer_storage alias here improves clarity.
74-75: Passing resolved chains to tx_indexer.NewService: LGTMAligns with the new constructor pattern across the PR.
cmd/fees/worker/main.go (2)
15-16: Correct import path migration to plugin/tx_indexerImport path updated to verifier/plugin/tx_indexer and its storage subpackage. This aligns with the refactor and retrieved learnings. LGTM.
122-126: Passing pre-fetched chains into NewService is correctUsing the pre-fetched chains makes initialization explicit and avoids hidden errors in constructors. LGTM.
plugin/payroll/payroll.go (3)
11-11: Importing recipes/types for Suggest return typeBringing in recipes/types to reference PolicySuggest is correct for the new Suggest API. LGTM.
15-16: tx_indexer import path updated to plugin/tx_indexerMatches the migration plan. LGTM.
19-19: Interface assertion updated to plugin.SpecThis compile-time check is useful to ensure the Plugin satisfies the new interface. LGTM.
plugin/fees/fees.go (3)
15-15: tx_indexer import path migrated to plugin/tx_indexerMatches the verifier/plugin refactor. LGTM.
26-26: Importing recipes/types as rtypes for SuggestConsistent with the new Spec API. LGTM.
35-35: Interface assertion updated to plugin.SpecGood compile-time guard to ensure FeePlugin implements the new interface. LGTM.
webpiratt
left a comment
webpiratt
left a comment
There was a problem hiding this comment.
LGTM, please fix err handling and let's merge
3 participants
Update verifier dependency and fix tx_indexer imports
This PR aligns with the ongoing refactoring effort where shared plugin infrastructure has been moved from individual plugin repositories to the centralized
verifier/pluginpackage. This eliminates code duplication and provides a unified plugin development experience.Take note the verifier version is still not the newest currently, theres also refactoring efforts to vultisig-go (see #316), these changes will be handled in another PR. The current PR just handles the verifier/plugin to simplify the PR
Also the "Suggest" methods in the plugins are placeholders for now.
Changes Made
📦 Import Path Updates
github.com/vultisig/verifier/tx_indexertogithub.com/vultisig/verifier/plugin/tx_indexerplugin/path prefix🔧 Code Changes
tx_indexer.Chains()signature change: Function now returns(SupportedChains, error)instead of justSupportedChainstx_indexer.Chains()calls to handle the new error return value🔄 Interface Updates
plugin.Plugintoplugin.SpecinterfaceSuggest()method with placeholder implementation to satisfy the new interface requirements⬆️ Dependency Updates
v0.0.0-20250814230939-fd9719226c8f(right before vultisig-go refactoring)Related
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Notes