payroll scheduler refactoring#134
Conversation
|
Warning Rate limit exceeded@webpiratt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change set introduces a new unified scheduling system, replacing the previous time trigger mechanism with a streamlined scheduler table, interfaces, and worker logic. It removes legacy time trigger code, updates database schemas and migrations, adds new configuration and Docker support for a dedicated payroll scheduler service, and refactors server and plugin code to use the new scheduler interfaces and storage abstractions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PayrollScheduler (New Service)
participant Postgres
participant Redis (Asynq)
participant Plugin
User->>PayrollScheduler: Start service (main.go)
PayrollScheduler->>PayrollScheduler: Load config, connect to Postgres & Redis
PayrollScheduler->>PayrollScheduler: Run Worker
loop Every poll interval
PayrollScheduler->>Postgres: Fetch pending scheduler entries
PayrollScheduler->>Postgres: For each, fetch policy
PayrollScheduler->>PayrollScheduler: Calculate next execution time
PayrollScheduler->>Redis: Enqueue task to Asynq
PayrollScheduler->>Postgres: Update next_execution in scheduler table
end
Redis->>Plugin: Deliver scheduled task for execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ 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. 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 (
|
# Conflicts: # internal/scheduler/scheduler.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
storage/postgres/schema/schema.sql (2)
1-179: Resolve pipeline failure by updating schema.sql.The pipeline failure indicates that the schema file is out of sync with the actual database schema after migrations.
Run the following command to fix the pipeline failure:
make dump-schema CONFIG=config-plugin.yaml
82-93: Fix Remaining References Before Droppingtime_triggersTableThe
time_triggerstable is still referenced in the codebase and cannot be dropped until those references are removed or updated:• storage/postgres/policy.go
// line showing DELETE FROM time_triggers DELETE FROM time_triggersPlease:
- Remove or replace the logic in
storage/postgres/policy.gothat mentionstime_triggers.- Once all code references are removed, drop the
time_triggerstable and thetrigger_statusenum fromstorage/postgres/schema/schema.sql.
🧹 Nitpick comments (4)
plugin/payroll/interval.go (1)
29-29: Consider making time dependency injectable for better testability.The direct use of
time.Now()makes this code non-deterministic and difficult to test. Consider accepting a time provider interface or making the current time injectable.-func (i *Interval) FromNowWhenNext(policy types.PluginPolicy) (time.Time, error) { +func (i *Interval) FromNowWhenNext(policy types.PluginPolicy) (time.Time, error) { + return i.fromTimeWhenNext(policy, time.Now()) +} + +func (i *Interval) fromTimeWhenNext(policy types.PluginPolicy, now time.Time) (time.Time, error) { recipe, err := policy.GetRecipe() if err != nil { return time.Time{}, fmt.Errorf("failed to unpack recipe: %w", err) @@ -26,13 +30,13 @@ freq := cfg[frequency].GetStringValue() switch freq { case daily: - next = time.Now().AddDate(0, 0, 1) + next = now.AddDate(0, 0, 1) case weekly: - next = time.Now().AddDate(0, 0, 7) + next = now.AddDate(0, 0, 7) case biWeekly: - next = time.Now().AddDate(0, 0, 14) + next = now.AddDate(0, 0, 14) case monthly: - next = time.Now().AddDate(0, 1, 0) + next = now.AddDate(0, 1, 0) default: return time.Time{}, fmt.Errorf("unknown frequency: %s", freq) }Also applies to: 31-31, 33-33, 35-35
Dockerfile.Payroll.scheduler (1)
38-38: Consider making configuration more flexible.The current approach copies the example configuration file and renames it to
config.json. While this works for development, consider making the configuration source more flexible for production deployments, perhaps through environment variables or volume mounts.You could modify the approach to support external configuration:
-COPY payroll.scheduler.example.json config.json +COPY payroll.scheduler.example.json ./Then update the application to check for mounted config files or use the example as a fallback.
internal/scheduler/storage_pg.go (2)
23-32: Consider handling duplicate key errorsThe current implementation doesn't distinguish between duplicate key errors and other database errors. Consider checking for duplicate key violations to provide more specific error messages.
func (s *PostgresStorage) CreateWithTx(ctx context.Context, tx pgx.Tx, policyID uuid.UUID, next time.Time) error { _, err := tx.Exec(ctx, ` INSERT INTO scheduler (policy_id, next_execution) VALUES ($1, $2) `, policyID, next) if err != nil { + // Check if it's a unique constraint violation + if pgErr, ok := err.(*pgconn.PgError); ok && pgErr.Code == "23505" { + return fmt.Errorf("scheduler entry already exists for policy %s: %w", policyID, err) + } return fmt.Errorf("failed to create scheduler entry: %w", err) } return nil }
77-99: Consider reducing code duplicationBoth
SetNextandSetNextWithTxexecute the same SQL query. Consider extracting the common update logic.+func (s *PostgresStorage) setNextExecution(ctx context.Context, db interface{ Exec(context.Context, string, ...interface{}) (pgconn.CommandTag, error) }, policyID uuid.UUID, next time.Time) error { + _, err := db.Exec(ctx, ` + UPDATE scheduler + SET next_execution = $2 + WHERE policy_id = $1 + `, policyID, next) + if err != nil { + return fmt.Errorf("failed to update next execution time: %w", err) + } + return nil +} + func (s *PostgresStorage) SetNext(ctx context.Context, policyID uuid.UUID, next time.Time) error { - _, err := s.pool.Exec(ctx, ` - UPDATE scheduler - SET next_execution = $2 - WHERE policy_id = $1 - `, policyID, next) - if err != nil { - return fmt.Errorf("failed to update next execution time: %w", err) - } - return nil + return s.setNextExecution(ctx, s.pool, policyID, next) } func (s *PostgresStorage) SetNextWithTx(ctx context.Context, tx pgx.Tx, policyID uuid.UUID, next time.Time) error { - _, err := tx.Exec(ctx, ` - UPDATE scheduler - SET next_execution = $2 - WHERE policy_id = $1 - `, policyID, next) - if err != nil { - return fmt.Errorf("failed to update next execution time: %w", err) - } - return nil + return s.setNextExecution(ctx, tx, policyID, next) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
.run/[git] payroll worker.run.xml(1 hunks).run/[git] plugin stack.run.xml(1 hunks)Dockerfile.Payroll.scheduler(1 hunks)api/server.go(1 hunks)cmd/dca/server/main.go(2 hunks)cmd/dca/worker/main.go(1 hunks)cmd/fees/server/main.go(2 hunks)cmd/fees/worker/main.go(1 hunks)cmd/payroll/scheduler/config.go(1 hunks)cmd/payroll/scheduler/main.go(1 hunks)cmd/payroll/server/main.go(2 hunks)cmd/payroll/worker/main.go(1 hunks)docker-compose.yaml(6 hunks)go.mod(1 hunks)internal/scheduler/interval.go(1 hunks)internal/scheduler/policy.go(1 hunks)internal/scheduler/scheduler.go(0 hunks)internal/scheduler/scheduler_test.go(0 hunks)internal/scheduler/service.go(1 hunks)internal/scheduler/service_nil.go(1 hunks)internal/scheduler/storage.go(1 hunks)internal/scheduler/storage_pg.go(1 hunks)internal/scheduler/worker.go(1 hunks)internal/tasks/tasks.go(0 hunks)internal/types/time_trigger.go(0 hunks)payroll.scheduler.example.json(1 hunks)payroll.server.example.json(0 hunks)plugin/dca/dca.go(0 hunks)plugin/fees/transaction.go(0 hunks)plugin/payroll/interval.go(1 hunks)plugin/payroll/scheduler.go(1 hunks)plugin/payroll/transaction.go(1 hunks)scripts/dev/create_fee_policy/dummy_server.go(2 hunks)service/policy.go(4 hunks)storage/db.go(0 hunks)storage/postgres/migrations/plugin/20241211182120_add_time_triggers.sql(0 hunks)storage/postgres/migrations/plugin/20250721165241_scheduler.sql(1 hunks)storage/postgres/schema/schema.sql(3 hunks)storage/postgres/time_trigger.go(0 hunks)storage/postgres/time_trigger_test.go(0 hunks)
💤 Files with no reviewable changes (11)
- internal/tasks/tasks.go
- payroll.server.example.json
- plugin/dca/dca.go
- internal/scheduler/scheduler_test.go
- plugin/fees/transaction.go
- storage/postgres/time_trigger_test.go
- storage/db.go
- storage/postgres/migrations/plugin/20241211182120_add_time_triggers.sql
- internal/types/time_trigger.go
- storage/postgres/time_trigger.go
- internal/scheduler/scheduler.go
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
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.
.run/[git] payroll worker.run.xml (1)
Learnt from: webpiratt
PR: #114
File: .run/[git] payroll worker.run.xml:5-8
Timestamp: 2025-07-03T16:08:11.917Z
Learning: GoLand run configuration files (.run/*.run.xml) are auto-generated by the IDE and should not be manually edited. Environment variables and other settings should be modified through GoLand's Run/Debug Configurations UI instead.
cmd/fees/server/main.go (3)
Learnt from: RaghavSood
PR: #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: johnnyluo
PR: #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.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
go.mod (2)
Learnt from: RaghavSood
PR: #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: johnnyluo
PR: #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.
cmd/payroll/server/main.go (3)
Learnt from: RaghavSood
PR: #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: johnnyluo
PR: #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.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
scripts/dev/create_fee_policy/dummy_server.go (3)
Learnt from: RaghavSood
PR: #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: RaghavSood
PR: #75
File: storage/postgres/db_plugin.go:25-40
Timestamp: 2025-05-30T02:44:31.711Z
Learning: The embed.FS directive //go:embed migrations/plugin/*.sql in storage/postgres/db_plugin.go correctly embeds plugin migration files that exist in the storage/postgres/migrations/plugin/ directory. The path "migrations/plugin" passed to goose.Up() references the embedded filesystem structure, not the physical file system path.
Learnt from: johnnyluo
PR: #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.
.run/[git] plugin stack.run.xml (1)
Learnt from: webpiratt
PR: #114
File: .run/[git] payroll worker.run.xml:5-8
Timestamp: 2025-07-03T16:08:11.917Z
Learning: GoLand run configuration files (.run/*.run.xml) are auto-generated by the IDE and should not be manually edited. Environment variables and other settings should be modified through GoLand's Run/Debug Configurations UI instead.
cmd/dca/server/main.go (3)
Learnt from: RaghavSood
PR: #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: johnnyluo
PR: #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.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
internal/scheduler/policy.go (1)
Learnt from: RaghavSood
PR: #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.
api/server.go (2)
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). It contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
plugin/payroll/interval.go (2)
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:510-514
Timestamp: 2025-06-18T18:20:59.510Z
Learning: The erc20ABI constant is defined in plugin/payroll/constants.go within the payroll package, making it accessible to other files in the same package like transaction.go.
internal/scheduler/service.go (2)
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: RaghavSood
PR: #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.
storage/postgres/schema/schema.sql (1)
Learnt from: webpiratt
PR: #89
File: storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql:13-13
Timestamp: 2025-06-11T18:42:54.241Z
Learning: In the tx_indexer table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the policy_id column is intentionally not defined with a foreign-key constraint to plugin_policies(id).
payroll.scheduler.example.json (2)
Learnt from: garry-sharp
PR: #113
File: docker-compose.yaml:80-81
Timestamp: 2025-07-03T15:55:04.605Z
Learning: For docker-compose.yaml files used for local development builds, hardcoded database credentials like "myuser:mypassword" in DATABASE_DSN environment variables are acceptable since they're only used locally and not exposed to external networks.
Learnt from: RaghavSood
PR: #70
File: .github/workflows/migration-test.yml:14-17
Timestamp: 2025-05-27T04:28:20.414Z
Learning: In GitHub Actions workflows for testing, hardcoded database credentials are acceptable when using ephemeral test services (like PostgreSQL containers) that only exist during the CI run, as there's no security risk with temporary test environments.
cmd/payroll/worker/main.go (6)
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: RaghavSood
PR: #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: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). It contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:510-514
Timestamp: 2025-06-18T18:20:59.510Z
Learning: The erc20ABI constant is defined in plugin/payroll/constants.go within the payroll package, making it accessible to other files in the same package like transaction.go.
Learnt from: webpiratt
PR: #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.
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:0-0
Timestamp: 2025-06-18T18:23:20.077Z
Learning: In the payroll plugin (plugin/payroll/transaction.go), signRequest.Transaction is stored with the "0x" prefix, making it compatible with gcommon.FromHex which requires 0x-prefixed hex strings.
internal/scheduler/interval.go (1)
Learnt from: RaghavSood
PR: #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.
internal/scheduler/storage.go (1)
Learnt from: RaghavSood
PR: #75
File: storage/postgres/db_plugin.go:25-40
Timestamp: 2025-05-30T02:44:31.711Z
Learning: The embed.FS directive //go:embed migrations/plugin/*.sql in storage/postgres/db_plugin.go correctly embeds plugin migration files that exist in the storage/postgres/migrations/plugin/ directory. The path "migrations/plugin" passed to goose.Up() references the embedded filesystem structure, not the physical file system path.
storage/postgres/migrations/plugin/20250721165241_scheduler.sql (2)
Learnt from: webpiratt
PR: #89
File: storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql:13-13
Timestamp: 2025-06-11T18:42:54.241Z
Learning: In the tx_indexer table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the policy_id column is intentionally not defined with a foreign-key constraint to plugin_policies(id).
Learnt from: RaghavSood
PR: #75
File: storage/postgres/db_plugin.go:25-40
Timestamp: 2025-05-30T02:44:31.711Z
Learning: The embed.FS directive //go:embed migrations/plugin/*.sql in storage/postgres/db_plugin.go correctly embeds plugin migration files that exist in the storage/postgres/migrations/plugin/ directory. The path "migrations/plugin" passed to goose.Up() references the embedded filesystem structure, not the physical file system path.
cmd/payroll/scheduler/config.go (2)
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). It contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
plugin/payroll/transaction.go (5)
Learnt from: webpiratt
PR: #96
File: plugin/payroll/transaction.go:0-0
Timestamp: 2025-06-18T18:23:20.077Z
Learning: In the payroll plugin (plugin/payroll/transaction.go), signRequest.Transaction is stored with the "0x" prefix, making it compatible with gcommon.FromHex which requires 0x-prefixed hex strings.
Learnt from: webpiratt
PR: #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.
Learnt from: RaghavSood
PR: #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: #96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Learnt from: webpiratt
PR: #125
File: plugin/payroll/transaction.go:186-197
Timestamp: 2025-07-09T22:22:36.651Z
Learning: In plugin/payroll/transaction.go, the getTokenID function is designed to return evm.ZeroAddress.Hex() when no "token" parameter constraint is found, rather than returning an error. This is expected behavior according to the team's design intent, even though the token parameter is marked as required in the recipe specification.
plugin/payroll/scheduler.go (2)
Learnt from: webpiratt
PR: #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.
Learnt from: johnnyluo
PR: #108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
internal/scheduler/storage_pg.go (1)
Learnt from: RaghavSood
PR: #75
File: storage/postgres/db_plugin.go:25-40
Timestamp: 2025-05-30T02:44:31.711Z
Learning: The embed.FS directive //go:embed migrations/plugin/*.sql in storage/postgres/db_plugin.go correctly embeds plugin migration files that exist in the storage/postgres/migrations/plugin/ directory. The path "migrations/plugin" passed to goose.Up() references the embedded filesystem structure, not the physical file system path.
🧬 Code Graph Analysis (12)
cmd/fees/worker/main.go (1)
internal/tasks/tasks.go (1)
QUEUE_NAME(9-9)
cmd/dca/worker/main.go (1)
internal/tasks/tasks.go (1)
QUEUE_NAME(9-9)
cmd/fees/server/main.go (1)
internal/scheduler/service_nil.go (1)
NewNilService(15-17)
scripts/dev/create_fee_policy/dummy_server.go (1)
internal/scheduler/service_nil.go (1)
NewNilService(15-17)
cmd/dca/server/main.go (1)
internal/scheduler/service_nil.go (1)
NewNilService(15-17)
api/server.go (2)
internal/scheduler/service.go (1)
Service(11-15)service/policy.go (1)
NewPolicyService(33-42)
plugin/payroll/interval.go (1)
internal/scheduler/interval.go (1)
Interval(9-11)
cmd/payroll/worker/main.go (1)
internal/tasks/tasks.go (1)
QUEUE_NAME(9-9)
cmd/payroll/scheduler/config.go (1)
storage/redis_storage.go (1)
RedisConfig(18-24)
plugin/payroll/transaction.go (1)
internal/scheduler/storage.go (1)
Scheduler(11-14)
plugin/payroll/scheduler.go (1)
internal/scheduler/storage.go (1)
Storage(16-23)
internal/scheduler/storage_pg.go (1)
internal/scheduler/storage.go (1)
Scheduler(11-14)
🪛 GitHub Actions: Migration Test
storage/postgres/schema/schema.sql
[error] 1-1: Schema has changed but schema.sql was not updated! Please run 'make dump-schema CONFIG=config-plugin.yaml' to update the schema file.
🪛 Checkov (3.2.334)
payroll.scheduler.example.json
[MEDIUM] 8-9: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (54)
internal/scheduler/interval.go (1)
9-11: Well-designed interface for scheduler modularity.The
Intervalinterface provides a clean abstraction for calculating next execution times based on plugin policies. The method signature is clear and the interface follows the single responsibility principle, making it easily testable and mockable.go.mod (1)
23-23: Dependency update supports scheduler refactoring.The version bump of
github.com/vultisig/recipesaligns with the scheduler refactoring changes in this PR. The update likely includes supporting changes for the new scheduler abstractions.cmd/fees/server/main.go (2)
11-11: Added scheduler import for interface injection.
108-108: Proper dependency injection of scheduler service.Replacing the direct Redis options with
scheduler.NewNilService()follows good architectural practices by injecting a scheduler service interface. The use of NilService is appropriate for the fees server, which doesn't appear to require active scheduling capabilities..run/[git] payroll worker.run.xml (1)
5-8: Run configuration updated to reflect architectural changes.The removal of
BASE_FILE_PATHaligns with the separation of scheduling responsibilities from the payroll worker. However, remember that GoLand run configuration files are auto-generated and should be modified through the IDE's Run/Debug Configurations UI rather than manual editing.cmd/fees/worker/main.go (1)
59-61: Simplified queue configuration aligns with architectural separation.Removing the "scheduled_plugin_queue" from the fees worker is appropriate given the refactoring that moves scheduling responsibilities to dedicated scheduler workers. This simplifies the worker to focus on its core fees-related tasks while maintaining the necessary
tasks.QUEUE_NAMEqueue for plugin operations.cmd/dca/worker/main.go (1)
47-49: LGTM! Queue configuration updated correctly.The removal of the "scheduled_plugin_queue" aligns with the architectural refactoring that moves scheduling responsibilities to a dedicated scheduler service. The remaining configuration is correct and maintains the original plugin_queue functionality.
scripts/dev/create_fee_policy/dummy_server.go (2)
13-13: LGTM! Scheduler import added correctly.The import of the internal scheduler package is properly added to support the new scheduler service dependency injection pattern.
124-124: LGTM! Scheduler service dependency injection implemented correctly.The use of
scheduler.NewNilService()is appropriate for a dummy server, providing a no-op scheduler implementation. This change aligns with the broader architectural refactoring that replaces direct Redis options with scheduler service interface injection.cmd/dca/server/main.go (2)
11-11: LGTM! Scheduler import added correctly.The import addition supports the new scheduler service dependency injection pattern implemented across the codebase.
88-88: LGTM! Scheduler service dependency injection implemented correctly.The use of
scheduler.NewNilService()is appropriate for the DCA server, providing a no-op scheduler implementation. This change is consistent with the architectural refactoring that moves from direct Redis configuration to scheduler service interface injection.payroll.scheduler.example.json (1)
1-10: LGTM! Configuration structure is correct.The JSON configuration file follows standard patterns for Redis and database connections. The hardcoded credentials are acceptable since this is an example configuration file for local development, as indicated by the
.example.jsonsuffix. The structure aligns with the new payroll scheduler service requirements..run/[git] plugin stack.run.xml (1)
3-3: LGTM! Payroll scheduler added to compound run configuration.The addition of the "[git] payroll scheduler" configuration entry aligns with the new scheduler service introduced in this PR. The change appears to have been made through GoLand's Run/Debug Configurations UI, which is the correct approach for IDE configuration files.
internal/scheduler/policy.go (1)
10-12: Well-designed interface following Go best practices.The
PolicyFetcherinterface is cleanly designed with a focused responsibility, proper context usage, and clear method signature. This abstraction will enable good testability and modularity in the scheduler system.cmd/payroll/worker/main.go (1)
53-56: Good separation of concerns by removing scheduler from worker.The removal of the scheduler service and its queue from the payroll worker aligns well with the architectural refactoring. This separation allows the worker to focus solely on task execution while scheduling is handled by the dedicated scheduler service.
storage/postgres/migrations/plugin/20250721165241_scheduler.sql (1)
4-10: Well-designed scheduler table with proper indexing.The schema design is sound with
policy_idas UUID primary key ensuring one entry per policy andnext_executiontimestamp for scheduling queries. The index onnext_executionwill optimize the scheduler's queries for finding due tasks.storage/postgres/schema/schema.sql (2)
77-80: Schema addition looks correct but needs cleanup.The new scheduler table definition matches the migration and follows the established patterns.
8-11: Consider removing trigger_status enum if no longer used.The
trigger_statusenum appears to be related to the legacy time_triggers system. If the time_triggers table is being removed, this enum may also be obsolete.cmd/payroll/server/main.go (2)
11-11: Clean integration of new scheduler package.The import of the internal scheduler package is appropriate for accessing the new PostgreSQL storage implementation.
97-97: Well-structured dependency injection for scheduler service.The integration of
payroll.NewSchedulerServicewith PostgreSQL storage backend aligns perfectly with the architectural refactoring goals. This creates a clean separation between the API server and the scheduler worker while maintaining proper dependency injection.internal/scheduler/service.go (1)
11-15: Well-designed interface for scheduler abstraction.The interface provides a clean abstraction for scheduler operations with proper transactional support. The method signatures are consistent and follow Go conventions.
cmd/payroll/scheduler/main.go (3)
21-29: Redis client setup looks correct.The Redis client initialization follows proper patterns with deferred cleanup to prevent resource leaks.
31-34: Database initialization is appropriate.The PostgreSQL backend initialization follows the established pattern used elsewhere in the codebase.
36-50: Worker initialization and execution follow good practices.The worker is properly initialized with all required dependencies and the error handling with panic is appropriate for a main function.
internal/scheduler/service_nil.go (1)
23-33: Null object pattern implementation is correct.The no-op implementations for Create, Update, and Delete methods properly implement the null object pattern for plugins that don't require scheduling.
docker-compose.yaml (2)
118-138: New payroll-scheduler service is properly configured.The service follows the established patterns in the compose file with appropriate dependencies, environment variables, and network configuration.
86-86: Good cleanup of trailing whitespace.Removing trailing spaces from port mappings improves code consistency.
Also applies to: 151-151
plugin/payroll/interval.go (2)
17-40: Interval calculation logic is correct.The method properly implements the Interval interface and handles different frequency types with appropriate date arithmetic. The error handling is well-structured.
26-26: No missing frequency constants – definitions exist in transaction.goThe
frequency,daily,weekly,biWeekly, andmonthlyconstants are defined at lines 271–274 inplugin/payroll/transaction.go. Since both files belong to the samepayrollpackage, these package-level constants are visible ininterval.goand no changes are needed.cmd/payroll/scheduler/config.go (3)
12-17: Configuration structure looks good.The
PayrollSchedulerConfigstruct properly embeds thestorage.RedisConfigfrom the existing storage package and includes a simple database DSN configuration. The struct tags are consistent with the codebase patterns for both mapstructure and JSON serialization.
19-25: Environment variable handling is correct.The function properly reads the
VS_CONFIG_NAMEenvironment variable with a sensible default fallback to "config". This pattern is consistent with other configuration handling in the codebase.
27-42: Configuration loading implementation is solid.The Viper configuration setup follows best practices with proper config path, environment variable replacement, and automatic environment variable binding. Error handling appropriately wraps errors with context.
api/server.go (2)
54-54: Good dependency injection pattern.The change from accepting
redisOptsand creating a concrete scheduler internally to accepting ascheduler.Serviceinterface improves testability and follows dependency injection principles. This aligns well with the new scheduler architecture.
58-58: Correct usage of injected scheduler service.The
policyServiceis now properly instantiated using the injected scheduler service interface, which maintains the decoupling from concrete implementations while ensuring the policy service has access to scheduling functionality.Dockerfile.Payroll.scheduler (2)
14-21: Library setup and environment configuration look correct.The download and extraction of the go-wrappers library for DKLS functionality is properly handled. The
LD_LIBRARY_PATHenvironment variable is correctly set to include the native libraries directory, which will be needed for the scheduler's cryptographic operations.
26-31: Runtime stage properly configured.The Ubuntu 24.04 base image is appropriate, and the installation of
wgetfollowed by cleanup of apt caches follows Docker best practices for minimizing image size. The package installation is efficient and clean.internal/scheduler/storage.go (2)
11-14: Well-designed scheduler data model.The
Schedulerstruct is appropriately simple and focused, containing only the essential fields needed for scheduling: policy identification and next execution time. The JSON tags are properly configured for serialization.
16-23: Comprehensive and well-structured storage interface.The
Storageinterface provides a complete set of operations for scheduler management with proper separation between transactional and non-transactional methods. The consistent use of context for cancellation andpgx.Txfor transactional operations follows Go best practices. The interface supports both individual policy operations and bulk operations likeGetPending().plugin/payroll/scheduler.go (3)
14-22: Clean service structure and constructor.The
SchedulerServiceproperly encapsulates thescheduler.Storagedependency and follows the dependency injection pattern. The constructor is straightforward and correctly initializes the service.
52-67: Efficient update logic with proper comparison.The Update method efficiently compares old and new start dates before making database changes, avoiding unnecessary updates when the start date hasn't changed. The transactional approach ensures data consistency.
69-71: Simple and correct Delete implementation.The Delete method properly delegates to the repository's transactional delete method, maintaining consistency with the other operations.
service/policy.go (5)
29-29: Good use of interface segregation principleThe change from concrete
*scheduler.SchedulerServiceto interfacescheduler.Serviceimproves testability and follows SOLID principles.
33-33: Constructor properly updated for interface typeThe constructor signature correctly accepts the interface type, maintaining consistency with the struct field change.
64-68: Proper transactional scheduler integrationThe scheduler creation is correctly placed within the transaction context, ensuring atomicity between policy and scheduler operations.
84-98: Well-structured update flow with proper error handlingThe update logic correctly retrieves the old policy for comparison and maintains transactional integrity by updating the scheduler before the policy.
120-124: Transactional delete maintains consistencyThe scheduler deletion is properly integrated within the transaction, ensuring no orphaned scheduler entries if either operation fails.
plugin/payroll/transaction.go (1)
40-44: Correct migration to new scheduler typeThe change from
types.TimeTriggertoscheduler.Schedulerproperly aligns with the new scheduler architecture.internal/scheduler/storage_pg.go (4)
13-21: Well-structured storage implementationThe PostgresStorage struct and constructor follow standard patterns for database storage implementations.
34-47: Robust single record retrievalGood use of
QueryRowand defensiveLIMIT 1clause. Error handling is appropriate.
49-75: Well-implemented batch retrieval with proper resource managementThe method correctly handles multiple rows with proper resource cleanup and error checking for row iteration.
101-110: Clean deletion implementationThe delete method is properly implemented with transaction support and error handling.
internal/scheduler/worker.go (3)
15-50: Well-structured worker with appropriate dependenciesThe Worker struct properly encapsulates all necessary dependencies, and the constructor follows good initialization patterns.
52-85: Robust worker lifecycle managementThe implementation properly handles graceful shutdown and continues operation despite individual enqueue errors, which is appropriate for a scheduler worker.
117-127: Appropriate task configuration for schedulerGood choice to set
MaxRetry(0)- scheduled tasks should not be retried automatically. The 5-minute timeout and 10-minute retention are reasonable defaults.
# Conflicts: # go.mod # go.sum # plugin/payroll/transaction.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
storage/postgres/schema/schema.sql (1)
75-76: Timestamp storage lacks timezone information.
next_executionis declared astimestamp without time zone. Schedulers usually run in UTC or need to respect user time-zones. Storing valuesWITH TIME ZONEprevents silent, locale-dependent conversions.Confirm that every insert/query normalises to UTC; otherwise switch to
timestamptz.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)plugin/payroll/transaction.go(2 hunks)storage/postgres/schema/schema.sql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/payroll/transaction.go
- go.mod
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
storage/postgres/schema/schema.sql (4)
Learnt from: webpiratt
PR: #89
File: storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql:13-13
Timestamp: 2025-06-11T18:42:54.241Z
Learning: In the tx_indexer table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the policy_id column is intentionally not defined with a foreign-key constraint to plugin_policies(id).
Learnt from: johnnyluo
PR: #82
File: internal/types/time_trigger.go:22-22
Timestamp: 2025-06-03T23:38:43.733Z
Learning: In the vultisig/plugin codebase, enum types like rtypes.ScheduleFrequency are stored as integers (INT) in the PostgreSQL database rather than as text strings, providing better type safety and performance.
Learnt from: webpiratt
PR: #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.
Learnt from: garry-sharp
PR: #117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
⏰ 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: build
- GitHub Check: migration-test
🔇 Additional comments (2)
storage/postgres/schema/schema.sql (2)
123-123: Index choice looks good.
idx_scheduler_next_executionenables efficient polling for the next due job and is aligned with the expected access pattern.
73-76: Enforce referential integrity or document intentional omission forscheduler.policy_idThe
schedulertable definespolicy_idas its primary key but omits a foreign-key toplugin_policies(id), just liketx_indexer. If this is a deliberate pattern (e.g. for performance or decoupling), please add a brief comment next to the DDL in both the schema and migration so future maintainers aren’t left guessing. Otherwise, enforce the FK with cascade:ALTER TABLE ONLY "scheduler" ADD CONSTRAINT "scheduler_policy_id_fkey" FOREIGN KEY ("policy_id") REFERENCES "plugin_policies"("id") ON DELETE CASCADE;Key locations to update:
- storage/postgres/schema/schema.sql (CREATE TABLE at lines 73–76; ALTER TABLE near line 106)
- storage/postgres/migrations/plugin/20250721165241_scheduler.sql (
CREATE TABLE IF NOT EXISTS scheduler …)
3 participants
schedulefrom policy and implementedconfigurationhandling for payroll;time_triggerstable removed and reworked asschedulertable;payroll.schedulerrunning as separate instance, struct is generic enough and reusable for DCA scheduler handling / other plugins in future;payroll.scheduler;Ref: vultisig/recipes#44 & #132
Summary by CodeRabbit
New Features
Improvements
schedulertable for managing scheduled executions.Bug Fixes
Refactor
Chores
Documentation