feat: update plugin policy handling to use UUID and add payroll plugi…#84
Conversation
WalkthroughThis set of changes refactors plugin policy handling throughout the codebase. It replaces string-based policy IDs with UUIDs in APIs, services, and storage layers, and updates policy input types from Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant Service
participant Storage
API->>API: Parse policyID as UUID
API->>Service: GetPluginPolicy(ctx, policyID: UUID)
Service->>Storage: GetPluginPolicy(ctx, id: UUID)
Storage-->>Service: PluginPolicy
Service-->>API: PluginPolicy
API->>API: Bind request as PluginPolicyCreateUpdate
API->>API: Convert to PluginPolicy for signature verification
API->>API: Verify policy signature
sequenceDiagram
participant Scheduler
participant PayrollPlugin
participant DB
Scheduler->>PayrollPlugin: HandleSchedulerTrigger(ctx, task)
PayrollPlugin->>DB: GetPluginPolicy(ctx, policyID: UUID)
DB-->>PayrollPlugin: PluginPolicy
PayrollPlugin-->>Scheduler: (processing continues or completes)
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" ✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates policy handling to use UUIDs and integrates a payroll plugin with scheduler tasks.
- Switched plugin policy identifiers from string to
uuid.UUIDacross storage, service, and API layers - Introduced
PluginPolicyCreateUpdatetype in plugin methods for create/update operations - Added a scheduler-trigger handler for the payroll plugin and updated worker configuration
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/policy.go | Change Get/DeletePluginPolicyTx to accept uuid.UUID |
| storage/db.go | Update DatabaseStorage interface signatures |
| service/policy.go | Update service methods to use uuid.UUID |
| plugin/payroll/transaction.go | Add HandleSchedulerTrigger and switch policy type |
| plugin/payroll/policy.go | Use PluginPolicyCreateUpdate in validation methods |
| plugin/payroll/payroll.go | Add plugin interface assertion |
| plugin/dca/dca.go | Switch DCA plugin methods to PluginPolicyCreateUpdate and add assertion |
| cmd/payroll/worker/main.go | Wire in payroll plugin and scheduler handler |
| cmd/payroll/worker/config.go | Add BaseConfigPath for payroll plugin |
| api/plugin.go | Parse uuid.UUID in endpoints and use CreateUpdate type |
| go.mod | Bump recipes and verifier module versions |
Comments suppressed due to low confidence (5)
plugin/payroll/payroll.go:8
- Importing the plugin interface from
verifier/pluginlikely mismatches your actual plugin definition. It should align with the module path that definesplugin.Plugin(e.g.,github.com/vultisig/plugin).
github.com/vultisig/verifier/plugin
api/plugin.go:70
- You’re setting
BillingRecipeto an empty string when constructingpolicyUpdate, which may drop existing billing configuration. Consider copyingpolicy.BillingRecipeinstead of hardcoding"".
BillingRecipe: "",
plugin/dca/dca.go:24
- As with the payroll plugin, this import likely mismatches your actual plugin interface module. Ensure you import the correct package that defines
plugin.Plugin.
github.com/vultisig/verifier/plugin
plugin/payroll/transaction.go:13
- Missing import for the Asynq package. You’re using
*asynq.Taskandasynq.SkipRetrybut haven’t importedgithub.com/hibiken/asynq.
github.com/vultisig/vultiserver/contexthelper
cmd/payroll/worker/main.go:13
- Import path looks incorrect (
plugin/plugin/payroll). It should match the actual module path for your payroll plugin (e.g.,github.com/vultisig/plugin/payroll).
github.com/vultisig/plugin/plugin/payroll
| return fmt.Errorf("failed to get plugin policy: %s, %w", err, asynq.SkipRetry) | ||
| } | ||
| // propose transaction and get it signed | ||
| _ = pluginPolicy |
There was a problem hiding this comment.
[nitpick] The placeholder _ = pluginPolicy is unused. You should implement the intended logic or remove this line to avoid confusion.
| _ = pluginPolicy |
![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
🔭 Outside diff range comments (5)
plugin/payroll/transaction.go (3)
55-100:⚠️ Potential issueType mismatch for
PolicyIDbreaks compilation
PluginKeysignRequest.PolicyIDis declared asstring, yet the builder passes auuid.UUID:PolicyID: policy.ID, // ← uuid.UUIDThis fails to compile.
-PolicyID: policy.ID, +PolicyID: policy.ID.String(),Please audit all call-sites (
DCAPlugin, API handlers, etc.) for the same issue.
175-186:⚠️ Potential issueIncorrect pre-sign value for
v– EIP-155 signing will produce an invalid hashWhen computing the RLP payload to hash you should store the chain-id, not
chainId*2+35.
v = chainId(with emptyr,s) is what EIP-155 specifies for the unsigned tx; the additive constant is applied only after the signature is produced.- V := new(big.Int).Set(chainIDInt) - V = V.Mul(V, big.NewInt(2)) - V = V.Add(V, big.NewInt(35)) + V := new(big.Int).Set(chainIDInt) // unsigned tx – plain chain-idUsing the inflated value will yield the wrong hash, causing the subsequent signature to be rejected by every client.
305-311: 🛠️ Refactor suggestion
convertDatauses an un-initialised policy – potential zero chain-id
payrollPolicyis instantiated but never populated, sopayrollPolicy.ChainID[0]is an empty string.
The fallback tooriginalTx.ChainId()is commented out, leavingchainID=0, which will breakNewLondonSigner.Either parse the recipe or derive the chain-id from
originalTxuntil recipe parsing is wired.api/plugin.go (1)
52-57:⚠️ Potential issue
GetPluginPolicynow expects auuid.UUID, not a string
storage.DatabaseStorage.GetPluginPolicywas switched to UUID, but the call site still passesreq.PolicyID(string). Compile-time failure.-policy, err := s.db.GetPluginPolicy(c.Request().Context(), req.PolicyID) +uID, err := uuid.Parse(req.PolicyID) +if err != nil { + return fmt.Errorf("invalid policy_id: %w", err) +} +policy, err := s.db.GetPluginPolicy(c.Request().Context(), uID)Apply the same parsing in every place you fetch a policy by ID.
plugin/dca/dca.go (1)
274-357:⚠️ Potential issue
PolicyIDfield suffers from the same UUID/string mismatchBoth
ProposeTransactionsandValidateProposedTransactionsembedpolicy.ID(uuid.UUID) into astringfield.-PolicyID: policy.ID, +PolicyID: policy.ID.String(),Search-replace across the entire plugin to fix.
🧹 Nitpick comments (5)
plugin/payroll/policy.go (2)
52-52: Address TODO comments for production readiness.There are two TODO comments that should be resolved:
- Line 52: Converting recipes to PayrollPolicy
- Line 70: Clarification needed on token ID vs recipient address comparison logic
These indicate incomplete functionality that may affect correctness.
Would you like me to help implement the recipe conversion or clarify the comparison logic?
Also applies to: 70-70
85-85: Remove debug print statement.The
fmt.Printf("Decoded: %+v\n", v)statement should be removed before production deployment to avoid cluttering logs.- fmt.Printf("Decoded: %+v\n", v)service/policy.go (1)
146-151: Consider updating method signature for consistency.The
GetPluginPolicyTransactionHistorymethod still accepts astringparameter and converts it to UUID internally, while other methods now acceptuuid.UUIDdirectly. This inconsistency breaks the pattern established by the broader UUID refactoring.Consider updating the method signature to accept
uuid.UUIDdirectly:-func (s *PolicyService) GetPluginPolicyTransactionHistory(ctx context.Context, policyID string) ([]types.TransactionHistory, error) { - // Convert string to UUID - policyUUID, err := uuid.Parse(policyID) - if err != nil { - return []types.TransactionHistory{}, fmt.Errorf("invalid policy_id: %s", policyID) - } +func (s *PolicyService) GetPluginPolicyTransactionHistory(ctx context.Context, policyID uuid.UUID) ([]types.TransactionHistory, error) { + policyUUID := policyIDapi/plugin.go (2)
72-74: Error message prints whole struct instead of hashreturn fmt.Errorf("message hash does not match transaction hash. expected %s, got %s", txHash, req.Messages[0])Second
%sreceives the entireKeysignMessagestruct. Use the hash field:- return fmt.Errorf("message hash does not match transaction hash. expected %s, got %s", txHash, req.Messages[0]) + return fmt.Errorf("message hash does not match transaction hash. expected %s, got %s", txHash, req.Messages[0].Hash)
340-369:verifyPolicySignatureduplicates heavy vault I/O for every callThis method opens & decrypts the vault on every signature check, which occurs for every create/update/delete request.
Consider caching the decrypted vault per(publicKey, pluginID)pair for the lifetime of the request to eliminate redundant disk and decryption work.
📜 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 (11)
api/plugin.go(10 hunks)cmd/payroll/worker/config.go(1 hunks)cmd/payroll/worker/main.go(2 hunks)go.mod(1 hunks)plugin/dca/dca.go(9 hunks)plugin/payroll/payroll.go(1 hunks)plugin/payroll/policy.go(2 hunks)plugin/payroll/transaction.go(3 hunks)service/policy.go(3 hunks)storage/db.go(1 hunks)storage/postgres/policy.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
plugin/payroll/payroll.go (1)
plugin/plugin.go (1)
Plugin(13-20)
plugin/payroll/policy.go (2)
plugin/payroll/payroll.go (1)
PayrollPlugin(15-21)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
storage/postgres/policy.go (1)
storage/postgres/db.go (1)
PostgresBackend(19-21)
api/plugin.go (2)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)api/server.go (1)
Server(32-45)
plugin/dca/dca.go (2)
plugin/plugin.go (1)
Plugin(13-20)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
🔇 Additional comments (14)
cmd/payroll/worker/config.go (1)
24-24: LGTM! Clean configuration field addition.The
BaseConfigPathfield follows proper Go conventions with appropriate struct tags and omitempty handling.plugin/payroll/payroll.go (2)
8-8: Good practice: adding plugin package import for interface assertion.The import is necessary to support the compile-time interface compliance check.
13-13:Details
✅ Verification successful
Verify that PayrollPlugin implements all Plugin interface methods.
The compile-time assertion is excellent practice, but please ensure that
PayrollPluginimplements all required methods from theplugin.Plugininterface.Based on the relevant code snippet, the Plugin interface requires these methods:
FrontendSchema() embed.FSGetRecipeSpecification() rtypes.RecipeSchemaValidatePluginPolicy(policyDoc vtypes.PluginPolicy) errorProposeTransactions(policy vtypes.PluginPolicy) ([]types.PluginKeysignRequest, error)ValidateProposedTransactions(policy vtypes.PluginPolicy, txs []types.PluginKeysignRequest) errorSigningComplete(ctx context.Context, signature tss.KeysignResponse, signRequest types.PluginKeysignRequest, policy vtypes.PluginPolicy) error
🏁 Script executed:
#!/bin/bash # Verify PayrollPlugin implements all Plugin interface methods ast-grep --pattern $'func ($_ *PayrollPlugin) FrontendSchema() $_ { $$$ }' ast-grep --pattern $'func ($_ *PayrollPlugin) GetRecipeSpecification() $_ { $$$ }' ast-grep --pattern $'func ($_ *PayrollPlugin) ValidatePluginPolicy($_) $_ { $$$ }' ast-grep --pattern $'func ($_ *PayrollPlugin) ProposeTransactions($_) $_ { $$$ }' ast-grep --pattern $'func ($_ *PayrollPlugin) ValidateProposedTransactions($_, $_) $_ { $$$ }' ast-grep --pattern $'func ($_ *PayrollPlugin) SigningComplete($_, $_, $_, $_) $_ { $$$ }'Length of output: 17383
PayrollPlugin correctly implements the
plugin.Plugininterface
All six required methods are present with matching signatures (FrontendSchema,GetRecipeSpecification,ValidatePluginPolicy,ProposeTransactions,ValidateProposedTransactions, andSigningComplete). The compile‐time assertion is satisfied—no further changes needed.go.mod (1)
22-23:Details
❓ Verification inconclusive
Verify the updated dependency versions are valid and secure.
The dependency updates use pseudo-versions based on git commits. Please ensure these commits exist and don't introduce breaking changes or security vulnerabilities.
🏁 Script executed:
#!/bin/bash # Verify the updated dependencies exist and check for any security advisories echo "Checking vultisig/recipes version..." curl -s "https://api.github.com/repos/vultisig/recipes/commits/58772375f814" | jq '.sha, .commit.message, .commit.author.date' echo "Checking vultisig/verifier version..." curl -s "https://api.github.com/repos/vultisig/verifier/commits/2f9ec25b5689" | jq '.sha, .commit.message, .commit.author.date' # Check if modules can be fetched go list -m github.com/vultisig/recipes@v0.0.0-20250604212709-58772375f814 go list -m github.com/vultisig/verifier@v0.0.0-20250606071917-2f9ec25b5689Length of output: 1108
Dependencies validated successfully
- Both pseudo-versions exist on GitHub and fetch correctly with
go list:
- recipes@v0.0.0-20250604212709-58772375f814
• sha: 58772375f814733251481370991b84e706f578cd
• date: 2025-06-04T21:27:09Z
• message: “Merge pull request #18…engine: policy and transaction evaluation engine”- verifier@v0.0.0-20250606071917-2f9ec25b5689
• sha: 2f9ec25b5689679738e1ee8aac2a62e011756f98
• date: 2025-06-06T07:19:17Z
• message: “Remove policy…remove PolicyID field from PluginKeysignRequest struct”No errors were returned by
go list.Please review any code that depended on the now-removed
PolicyIDfield in theverifiermodule and run your integration/security tests to confirm there are no breaking changes or new vulnerabilities.cmd/payroll/worker/main.go (3)
13-13: LGTM! Proper import for payroll plugin integration.The import is correctly added to support the payroll plugin initialization.
60-63: Good integration pattern with proper error handling.The payroll plugin initialization follows the established pattern:
- Uses the postgres backend dependency
- Leverages the new BaseConfigPath configuration
- Includes appropriate error handling consistent with other service initializations
73-73:Details
✅ Verification successful
Verify HandleSchedulerTrigger method signature matches asynq handler requirements.
Please ensure that the
HandleSchedulerTriggermethod on PayrollPlugin has the correct signature for asynq task handling.
🏁 Script executed:
#!/bin/bash # Verify HandleSchedulerTrigger method exists and has correct signature ast-grep --pattern $'func ($_ *PayrollPlugin) HandleSchedulerTrigger($_, $_) $_ { $$$ }' # Also search for any HandleSchedulerTrigger method definition rg -A 5 "func.*HandleSchedulerTrigger"Length of output: 2043
HandleSchedulerTrigger signature is correct
The
HandleSchedulerTriggermethod onPayrollPluginis declared asfunc (p *PayrollPlugin) HandleSchedulerTrigger(ctx context.Context, t *asynq.Task) errorwhich exactly matches the
asynq.HandlerFuncsignature. No changes are needed.storage/db.go (1)
17-17: LGTM! Interface updated for improved type safety.The change from
stringtouuid.UUIDfor policy ID parameters enhances type safety and ensures consistent UUID validation throughout the system.Also applies to: 19-19
plugin/payroll/policy.go (1)
40-40: LGTM! Method signatures updated for policy type consistency.The change from
vtypes.PluginPolicytovtypes.PluginPolicyCreateUpdatealigns with the broader refactoring to separate input types from stored policy types.Also applies to: 199-199
storage/postgres/policy.go (2)
8-8: LGTM! UUID import added for type safety.The addition of the
github.com/google/uuidimport supports the parameter type changes in the method signatures.
14-14: LGTM! Method signatures correctly updated for UUID usage.The parameter type changes from
stringtouuid.UUIDproperly implement the interface updates and maintain the existing logic while improving type safety.Also applies to: 155-155
service/policy.go (1)
20-20: LGTM! Service interface and implementation updated for UUID consistency.The method signatures correctly updated to use
uuid.UUIDparameters, maintaining consistency with the storage layer changes while preserving all business logic.Also applies to: 22-22, 110-110, 138-138
plugin/dca/dca.go (2)
51-52: Interface assertion may fail – signature list diverged
var _ plugin.Plugin = (*DCAPlugin)(nil)compiles only if the method set matchesplugin.Plugin.
The local implementation now acceptsPluginPolicyCreateUpdate, but the interface snippet (still in the repo) requiresPluginPolicy.Please verify the interface definition was updated across the codebase; otherwise builds will break.
362-415:completePolicycall needs the create/update type
ValidateProposedTransactionsconverts back toPluginPolicyin order to mark the policy completed:if err := p.completePolicy(ctx, policy.ToPluginPolicy()); …
completePolicyultimately persists the policy viaUpdatePluginPolicyTx, which now accepts the newer type in other layers. Make sure these signatures are consistent to avoid mismatches.
…n integration
Summary by CodeRabbit
New Features
Improvements
Dependency Updates