Conversation
WalkthroughThis set of changes standardizes error handling and logging in API handlers, removes vault upload and migration logic, updates policy service method names by removing the "WithSync" suffix, and eliminates references to ECDSA and chain code fields from plugin policy storage and migrations. Task queue constants and types are also updated for consistency. Additionally, UUID types replace string IDs for policies across multiple components, and dependency declarations are adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant PolicyService
participant Storage
Client->>API_Server: Create/Update/Delete Plugin Policy Request
API_Server->>PolicyService: CreatePolicy / UpdatePolicy / DeletePolicy
PolicyService->>Storage: Persist policy changes
Storage-->>PolicyService: Success/Error
PolicyService-->>API_Server: Result
API_Server-->>Client: Standardized ErrorResponse or Success
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 the plugin policy functionality by removing deprecated cryptographic fields and deprecating legacy DKLS-related code. Key changes include:
- Updates to SQL queries in storage/postgres/policy.go and corresponding migration to drop the “is_ecdsa” and “chain_code_hex” columns.
- Removal of several DKLS TSS and MPC wrapper files (e.g. service/worker_dkls.go, service/reshare_dkls.go, service/reshare.go, service/mpc_wrapper.go, service/keysign_dkls.go, service/dkls.go) to simplify the codebase.
- Refactoring of policy service method names and API error responses for improved clarity and consistency.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/policy.go | Updated SQL queries to remove deprecated cryptographic columns and match the new database schema. |
| storage/postgres/migrations/20250514060223_remove_chaincode_isecdsa.sql | Migration script that drops “chain_code_hex” and “is_ecdsa” columns, with a reversible down migration. |
| service/worker_dkls.go, service/reshare_dkls.go, service/reshare.go, service/mpc_wrapper.go, service/keysign_dkls.go, service/dkls.go | Entire removal of DKLS-related service files, reducing code complexity and maintenance overhead. |
| service/policy.go | Renamed policy methods (dropping “WithSync” suffix) to improve clarity in the policy service interface. |
| api/plugin.go | Standardized error responses via the new ErrorResponse type and related helper for improved API consistency. |
| scripts/dev/create_vault/main.go | Updated the import for the MPCWrapperImp to use the vault package, ensuring consistency with recent changes. |
Comments suppressed due to low confidence (3)
service/policy.go:51
- [nitpick] Renaming methods to remove the 'WithSync' suffix improves clarity. Ensure that this naming convention is used consistently across the codebase.
func (s *PolicyService) CreatePolicy(ctx context.Context, policy vtypes.PluginPolicy) (*vtypes.PluginPolicy, error) {
api/plugin.go:29
- [nitpick] The introduction of a dedicated ErrorResponse type standardizes error responses. Confirm that all API endpoints adopt this pattern for consistent client communication.
type ErrorResponse struct { Message string `json:"message"` }
storage/postgres/policy.go:23
- The updated query now omits 'is_ecdsa' and 'chain_code_hex', which aligns with the new schema. Please verify that all business logic relying on these columns has been refactored accordingly.
SELECT id, public_key, plugin_id, plugin_version, policy_version, plugin_type, signature, active, policy
![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 (3)
storage/postgres/policy.go (1)
92-110:⚠️ Potential issueINSERT column / placeholder mismatch will panic on first write
INSERT INTO plugin_policiesnow declares 9 columns (id … policy) but still provides 11 placeholders($1 … $11).
At runtime PostgreSQL will returnERROR: INSERT has more expressions than target columns, completely blocking policy creation.- ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)No other code changes are required because only 9 arguments are passed below.
Please update the placeholder list (and run migration tests) before merging.service/policy.go (1)
130-136:⚠️ Potential issueArguments are swapped – query will return the wrong rows
GetPluginPolicieshands parameters to the storage layer as
GetAllPluginPolicies(ctx, pluginType, publicKey)
while the storage signature is(ctx, publicKey, pluginType).This will search for a public_key = pluginType and plugin_type = publicKey, yielding empty or misleading results and potentially leaking data if
pluginTypecontains SQL wild-cards.- policies, err := s.db.GetAllPluginPolicies(ctx, pluginType, publicKey) + policies, err := s.db.GetAllPluginPolicies(ctx, publicKey, pluginType)api/plugin.go (1)
124-148:⚠️ Potential issueUndefined helper
minwill not compile
min(20, len(req.Transaction))(l.146) is not a built-in in Go ≤1.22 and no helper is defined in this file.- s.logger.Infof("Created transaction history for tx from plugin: %s...", req.Transaction[:min(20, len(req.Transaction))]) + size := 20 + if len(req.Transaction) < size { + size = len(req.Transaction) + } + s.logger.Infof("Created transaction history for tx from plugin: %s...", req.Transaction[:size])Alternatively, create a small
min(a, b int) inthelper.
🧹 Nitpick comments (6)
.gitignore (1)
14-15: Use explicit directory patterns in .gitignore
To avoid unintentionally ignoring files named “worker” or “vultisigner” elsewhere and to clearly denote these as directories, specify trailing slashes and anchor to the repo root.-worker -vultisigner +/worker/ +/vultisigner/cmd/worker/main.go (2)
25-29: Remember to release resources owned by the storage implementation
vault.NewBlockStorageImpprobably opens network connections or file handles.
If it implementsio.Closer, we should close it when the worker shuts down to avoid leaks.v := logrus.StandardLogger() client := asynq.NewClient(redisOptions) +// Ensure resources are released on exit +if c, ok := vaultStorage.(io.Closer); ok { + defer c.Close() +}
55-57: Remove commented-out handler to avoid dead code driftThe old
HandlePluginTransactionline is kept as a comment:// mux.HandleFunc(tasks.TypePluginTransaction, vaultService.HandlePluginTransaction)If it is no longer relevant, delete it; if it might return, leave a
TODOwith context (issue link).
Commented code tends to rot and confuses newcomers.api/server.go (1)
343-346: Unnecessary full vault deserialization on the hot signing path
DecryptVaultFromBackupreturns the entire vault, but the value is discarded (_, err := …).
This allocates & unmarshals potentially large protobuf objects just to validate the password.Consider adding a lightweight “password check only” helper inside
commonto avoid the extra CPU & memory:-_, err = common.DecryptVaultFromBackup(req.VaultPassword, content) +err = common.VerifyVaultPassword(req.VaultPassword, content) // new helper that stops after successful decryptionThis keeps latency down, especially when the vault size grows.
service/policy.go (1)
110-118:signatureparameter is accepted but never used
DeletePolicy(ctx, policyID, signature string)ignores thesignature.
Consider dropping the parameter (and its callers) or validating it to avoid a false sense of security.api/plugin.go (1)
299-304: Signature is verified, then ignored by the serviceAfter validating the signature, the call forwards only
policyIDtoDeletePolicy, but the service method still accepts asignatureparameter.
Pass it along or drop the parameter throughout to avoid dead code paths.- if err := s.policyService.DeletePolicy(c.Request().Context(), policyID, reqBody.Signature); err != nil { + if err := s.policyService.DeletePolicy(c.Request().Context(), policyID, reqBody.Signature); err != nil {(If the service truly doesn’t need it, remove the parameter altogether.)
📜 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 (24)
.gitignore(1 hunks)api/plugin.go(12 hunks)api/server.go(1 hunks)cmd/worker/main.go(3 hunks)config/config.go(2 hunks)go.mod(3 hunks)internal/tasks/tasks.go(1 hunks)relay/message.go(0 hunks)relay/messenger.go(0 hunks)relay/messenger_test.go(0 hunks)relay/session.go(0 hunks)relay/state.go(0 hunks)scripts/dev/create_vault/main.go(3 hunks)service/dkls.go(0 hunks)service/keysign_dkls.go(0 hunks)service/mpc_wrapper.go(0 hunks)service/policy.go(4 hunks)service/reshare.go(0 hunks)service/reshare_dkls.go(0 hunks)service/tss.go(0 hunks)service/worker.go(0 hunks)service/worker_dkls.go(0 hunks)storage/postgres/migrations/20250514060223_remove_chaincode_isecdsa.sql(1 hunks)storage/postgres/policy.go(4 hunks)
💤 Files with no reviewable changes (13)
- relay/message.go
- relay/messenger_test.go
- relay/state.go
- service/worker_dkls.go
- relay/messenger.go
- service/mpc_wrapper.go
- service/reshare_dkls.go
- service/dkls.go
- service/keysign_dkls.go
- relay/session.go
- service/worker.go
- service/reshare.go
- service/tss.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
api/server.go (2)
common/util.go (1)
DecryptVaultFromBackup(140-169)internal/tasks/tasks.go (1)
TypeKeySignDKLS(13-13)
config/config.go (1)
storage/block_storage.go (1)
BlockStorage(19-24)
cmd/worker/main.go (2)
storage/block_storage.go (1)
BlockStorage(19-24)internal/tasks/tasks.go (3)
TypeKeyGenerationDKLS(12-12)TypeKeySignDKLS(13-13)TypeReshareDKLS(14-14)
service/policy.go (1)
internal/types/transaction.go (1)
TransactionHistory(21-31)
🔇 Additional comments (11)
internal/tasks/tasks.go (1)
9-9: Appropriate rename aligning with architectural changesThe queue name change from "vultisigner" to "plugin_queue" better reflects its purpose and aligns with the broader refactoring where relay, TSS, and DKLS cryptographic service code is being removed from the codebase.
storage/postgres/migrations/20250514060223_remove_chaincode_isecdsa.sql (1)
1-5: Schema simplification looks goodThe removal of unused columns from the plugin_policies table aligns with the broader simplification of policy data handling in the codebase.
scripts/dev/create_vault/main.go (2)
15-16: Good shift to external packagesUsing the external vault and relay packages aligns well with the PR objective of refactoring the system to rely on a vault-based service for DKLS operations.
102-102: Refactor using new vault implementationThe MPC wrapper implementation is correctly updated to use the external vault package.
go.mod (3)
22-22: Dependency update for vault service integrationUpdating the verifier dependency is appropriate given the architectural shift to using its vault implementation.
131-131: Dependency cleanupMoving
github.com/vultisig/go-wrappersto an indirect dependency aligns with the codebase cleanup where internal cryptographic service code is being removed.
153-153: Dependency categorization updateMarking
github.com/decred/dcrd/dcrec/secp256k1/v4as indirect is consistent with delegating cryptographic operations to external packages.config/config.go (2)
8-9: Minimise cross-package coupling from the configuration layer
confignow importsgithub.com/vultisig/verifier/vault.
While harmless at compile-time, bringing a high-level service package into the config layer means every place that needsconfigtransitively depends onvault, increasing compile times, the public API surface, and the risk of circular imports in the future (e.g. ifvaultever needsconfigagain).If the only thing you need is a data type, consider one of the following:
- Duplicate a tiny struct definition locally – cheaper than the dependency.
- Move the DTOs into a thin shared package (e.g.
vaulttypes) that contains no logic.
45-47: Breaking config keys – update samples and CI secrets
BlockStorageis nowvault.BlockStorageConfigand a brand-newVaultServiceConfigsection was added.
Any existingconfig.yaml/ environment overrides will break at runtime (silent zero-values) if these keys are not present.Action items:
• Provide a migration note inCHANGELOG.md.
• Add validation (e.g.validate:"required") or an explicit sanity check afterviper.Unmarshalso the process fails fast instead of running with empty values.api/server.go (1)
353-357: Hard-codingTypeKeySignDKLSmay disable future extensibilityAll signatures are now enqueued as
key:signDKLS.
If you plan to re-introduce other schemes (ECDSA/EdDSA) or plugin-specific signing later, it might be safer to decide task type dynamically based on request metadata.Please confirm that every client expects DKLS exclusively; otherwise this change is a silent breaking change.
api/plugin.go (1)
29-37: Nice addition – consistent error payloadsIntroducing
ErrorResponseandNewErrorResponseimproves API consistency and simplifies client handling. 👍
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/plugin.go (1)
378-383:⚠️ Potential issue
ChainCodeHexremoved from persistence – signature verification will fail
verifyPolicySignaturestill passespolicy.ChainCodeHextosigutil.VerifySignature, but the field is no longer stored/retrieved after the DB schema change.
For policies loaded from the database (e.g., inDeletePluginPolicyById) the value will be empty, causing deterministic verification failure.Options:
- Re-introduce
chain_code_hexcolumn and include it in CRUD queries, or- Amend
sigutil.VerifySignature/ calling code to work without it (e.g., derive it on-the-fly or drop the parameter).Please align persistence with verification before production usage.
🧹 Nitpick comments (1)
api/plugin.go (1)
323-326: Improved error handling for JSON unmarshalingThe error handling for JSON unmarshaling has been improved with proper error logging. Consider using a more descriptive variable name than
jsonErrto avoid confusion with the previously declarederr.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/plugin.go(12 hunks)api/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/server.go
🔇 Additional comments (8)
api/plugin.go (8)
29-37: Good addition of standardized error responsesIntroducing the
ErrorResponsestruct andNewErrorResponsehelper function standardizes JSON error responses across API handlers, replacing previous custom error maps. This improves API consistency and makes error handling more maintainable.
127-127: Task type constant updated correctlyThe change from a previous value to
tasks.TypeKeySignDKLSis consistent with the task type renaming mentioned in the PR summary.
159-162: Enhanced logging with structured fieldsGood improvement to logging by using
WithError()andWithField()for structured logs. This makes debugging easier by providing context about the operation that failed.
203-206: Improved error handling with contextual loggingThe error handling now includes more context with the plugin type, which will help with debugging. The standardized error response also improves API consistency.
223-223: Updated method name without "WithSync" suffixThe service method call has been renamed from something with a "WithSync" suffix to just
CreatePolicy, which aligns with the method naming changes mentioned in the PR summary.
261-261: Updated method name without "WithSync" suffixThe service method call has been renamed from something with a "WithSync" suffix to just
UpdatePolicy, which aligns with the method naming changes mentioned in the PR summary.
299-299: Updated method name without "WithSync" suffixThe service method call has been renamed from something with a "WithSync" suffix to just
DeletePolicy, which aligns with the method naming changes mentioned in the PR summary.
315-315: TODO comment added for multiple plugin typesA TODO has been added to remind developers that the code needs to handle both DCA and Payroll plugins. Consider assigning this TODO to a specific person or ticket to ensure it doesn't get forgotten.
…gin_policies table
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
storage/db.go (1)
20-24: 🛠️ Refactor suggestionInconsistent policy ID types across the interface
While you've updated the time trigger methods to use
uuid.UUIDfor policy IDs (lines 32-37), the plugin policy methods (lines 20-24) still usestring. This inconsistency could lead to confusion and potential bugs.For better type safety and consistency, consider updating all policy ID parameters across the interface to use
uuid.UUID.- GetPluginPolicy(ctx context.Context, id string) (vtypes.PluginPolicy, error) - GetAllPluginPolicies(ctx context.Context, publicKey string, pluginType string) ([]vtypes.PluginPolicy, error) - DeletePluginPolicyTx(ctx context.Context, dbTx pgx.Tx, id string) error - InsertPluginPolicyTx(ctx context.Context, dbTx pgx.Tx, policy vtypes.PluginPolicy) (*vtypes.PluginPolicy, error) - UpdatePluginPolicyTx(ctx context.Context, dbTx pgx.Tx, policy vtypes.PluginPolicy) (*vtypes.PluginPolicy, error) + GetPluginPolicy(ctx context.Context, id uuid.UUID) (vtypes.PluginPolicy, error) + GetAllPluginPolicies(ctx context.Context, publicKey string, pluginType string) ([]vtypes.PluginPolicy, error) + DeletePluginPolicyTx(ctx context.Context, dbTx pgx.Tx, id uuid.UUID) error + InsertPluginPolicyTx(ctx context.Context, dbTx pgx.Tx, policy vtypes.PluginPolicy) (*vtypes.PluginPolicy, error) + UpdatePluginPolicyTx(ctx context.Context, dbTx pgx.Tx, policy vtypes.PluginPolicy) (*vtypes.PluginPolicy, error)Also applies to: 32-37
♻️ Duplicate comments (1)
api/plugin.go (1)
379-380: Empty chain code used in signature verification.The
ChainCodeHexparameter is now passed as an empty string tosigutil.VerifySignature(). This aligns with removing chain code fields from storage, but verify if signature verification still works properly.This change addresses the issue where signature verification would fail due to
chain_code_hexbeing removed from persistence. Please verify through thorough testing that signature verification still works correctly with an empty chain code.
🧹 Nitpick comments (4)
scripts/dev/create_dca_policy/main.go (1)
82-83: Update comment with clear TODO for plugin identification.The comment indicates this UUID should be replaced with the actual DCA plugin ID, but doesn't specify when or how this should happen.
- PluginID: uuid.New(), // update it to DCA plugin ID + PluginID: uuid.New(), // TODO: Replace with the actual DCA plugin ID before production usescripts/dev/create_payroll_policy/main.go (1)
111-112: Update comment with clear TODO for plugin identification.The comment indicates this UUID should be replaced with the actual payroll plugin ID, but doesn't specify when or how this should happen.
- PluginID: uuid.New(), // update it to payroll plugin ID + PluginID: uuid.New(), // TODO: Replace with the actual payroll plugin ID before production usestorage/postgres/time_trigger.go (2)
151-151: Use String() method for UUID formatting in error messagesWhen formatting a UUID in an error message, it's clearer to explicitly convert it to a string using the
String()method rather than using the%sformat specifier.- return "", fmt.Errorf("trigger not found for policy_id: %s", policyID) + return "", fmt.Errorf("trigger not found for policy_id: %s", policyID.String())
16-18: Consider using constant error variables for common errorsThe error check for nil database pool is repeated across all methods. Consider defining a constant error variable at the package level to improve consistency and testability.
package postgres import ( "context" "errors" "fmt" "time" "github.com/google/uuid" "github.com/jackc/pgx/v5" "github.com/vultisig/plugin/internal/types" ) +// Common errors +var ( + ErrNilDatabasePool = errors.New("database pool is nil") +) func (p *PostgresBackend) CreateTimeTriggerTx(ctx context.Context, tx pgx.Tx, trigger types.TimeTrigger) error { if p.pool == nil { - return fmt.Errorf("database pool is nil") + return ErrNilDatabasePool } // Rest of the method... } // Update other methods similarlyAlso applies to: 40-42, 98-100, 114-116, 137-139, 160-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/plugin.go(11 hunks)cmd/vultisigner/main.go(1 hunks)internal/types/time_trigger.go(2 hunks)plugin/dca/dca.go(5 hunks)plugin/payroll/transaction.go(2 hunks)scripts/dev/create_dca_policy/main.go(1 hunks)scripts/dev/create_payroll_policy/main.go(1 hunks)storage/db.go(1 hunks)storage/postgres/db.go(1 hunks)storage/postgres/migrations/20250514060223_remove_chaincode_isecdsa.sql(1 hunks)storage/postgres/time_trigger.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/postgres/migrations/20250514060223_remove_chaincode_isecdsa.sql
🧰 Additional context used
🧬 Code Graph Analysis (4)
cmd/vultisigner/main.go (1)
storage/postgres/db.go (1)
NewPostgresBackend(29-45)
storage/db.go (1)
internal/types/time_trigger.go (2)
TimeTrigger(16-25)TimeTriggerStatus(9-9)
storage/postgres/time_trigger.go (2)
storage/postgres/db.go (1)
PostgresBackend(25-27)internal/types/time_trigger.go (2)
TimeTrigger(16-25)TimeTriggerStatus(9-9)
api/plugin.go (3)
internal/tasks/tasks.go (1)
TypeKeySignDKLS(13-13)api/server.go (1)
Server(34-49)internal/sigutil/signing.go (1)
VerifySignature(73-100)
🔇 Additional comments (25)
internal/types/time_trigger.go (2)
3-7: Added UUID import for enhanced type safety.Importing the
uuidpackage supports the change from string to UUID type for policy identifiers.
17-17: Improved type safety by changing PolicyID from string to uuid.UUID.This change ensures type safety and consistency in policy identifier handling across the codebase.
scripts/dev/create_dca_policy/main.go (1)
78-78: UUID type used directly instead of string conversion.Good refactoring to use UUID type directly rather than converting to string first.
scripts/dev/create_payroll_policy/main.go (1)
107-107: UUID type used directly instead of string conversion.Good refactoring to use UUID type directly rather than converting to string first.
api/plugin.go (8)
29-37: Well-structured error response standardization.Creating a consistent error response structure improves API usability and client experience.
59-59: Updated UUID string comparison.This change correctly handles the new UUID type when comparing policy plugin IDs.
127-127: Task type updated to match new constant naming.Updated to use the current
TypeKeySignDKLSconstant, which aligns with the task queue naming changes.
180-187: Enhanced structured logging with context fields.Improved logging practices by using structured logging with relevant context fields.
214-216: Properly handling empty UUID and generation.Correctly checks for empty UUID and generates a new one when needed, using the proper UUID type.
223-223: Updated policy service method name.Method call changed from
CreatePolicyWithSynctoCreatePolicy, aligning with service interface changes.
261-261: Updated policy service method name.Method call changed from
UpdatePolicyWithSynctoUpdatePolicy, aligning with service interface changes.
299-299: Updated policy service method name.Method call changed from
DeletePolicyWithSynctoDeletePolicy, aligning with service interface changes.storage/postgres/db.go (3)
17-17: Updated import statement for storage package.This change aligns the imports with the code changes made in this PR.
23-24: Added compile-time interface compliance check.This is a good practice to ensure that
PostgresBackendproperly implements thestorage.DatabaseStorageinterface. If it doesn't, the code will fail to compile, preventing potential runtime errors.
29-29: Simplified function signature by removing the readonly parameter.The constructor no longer accepts a boolean
readonlyparameter, simplifying the API. This aligns with the updated invocation incmd/vultisigner/main.go.plugin/payroll/transaction.go (2)
75-76: Enhanced type safety with explicit string conversion for UUIDs.The code now explicitly converts UUID fields to strings using
.String()method, which is a good practice for type safety and clarity. This suggests that these fields have been changed from string types to UUID types in the broader codebase.
55-55:Details
✅ Verification successful
Chain code parameter replaced with empty string.
The code now passes an empty string instead of
policy.ChainCodeHexwhen callinggeneratePayrollTransaction. This aligns with the broader refactoring to remove chain code fields from plugin policies.Ensure this change doesn't affect transaction generation by checking the usage of this parameter:
🏁 Script executed:
#!/bin/bash # Check how the chain code parameter is used in the generatePayrollTransaction function rg -A 2 -B 2 "chainCodeHex" plugin/payroll/transaction.goLength of output: 518
🏁 Script executed:
#!/bin/bash # Locate the DeriveAddress function definition and usages rg -n "func DeriveAddress" -C 3 rg -n "DeriveAddress" -C 3Length of output: 2645
Chain code parameter usage validated
- In plugin/payroll/transaction.go, generatePayrollTransaction now passes
""for thechainCodeHexargument.- common.DeriveAddress (common/util.go) delegates to tss.GetDerivedPubKey, and an empty chain code is already used without issue in the DCA plugin (see plugin/dca/dca.go lines 332 & 407).
- This change is consistent with the policy refactor to drop chain code fields—and no further updates are required.
cmd/vultisigner/main.go (1)
58-58: Updated function call to match simplified constructor signature.The call to
postgres.NewPostgresBackendhas been updated to remove the boolean parameter, matching the simplified function signature instorage/postgres/db.go. This ensures proper initialization of the database backend.plugin/dca/dca.go (6)
157-159: Simplified public key validation to always use ECDSA.The code now assumes all plugin policy public keys are ECDSA by hardcoding
trueas the second parameter toCheckIfPublicKeyIsValid. This removes the dependency on the deprecatedIsEcdsaflag from policies.
355-355: Hardcoded ECDSA flag in keysign request.The code now hardcodes
IsECDSA: truein the keysign request instead of using the chain'sIsEdDSA()method. This aligns with the removal of ECDSA fields from plugin policies.
360-362: Enhanced type safety with explicit string conversion for UUIDs.Similar to changes in
plugin/payroll/transaction.go, this code now explicitly converts UUID fields to strings using.String(). This is good practice for type safety and clarity.
406-408: Removed chain code from address derivation with TODO note.This is another instance of replacing chain code with an empty string, with a TODO comment explaining that chain codes should be retrieved from the vault. Consistent with the earlier change at lines 331-333.
743-745: Updated method signature to use UUID type instead of string.The
getCompletedSwapTransactionsCountmethod now accepts auuid.UUIDtype forpolicyIDinstead of a string. This improves type safety and consistency with the rest of the codebase.
331-333:Details
❓ Verification inconclusive
Removed chain code from address derivation.
The code now passes an empty string instead of a chain code when deriving addresses. The TODO comment indicates that chain codes should be retrieved from the vault in the future.
Verify that address derivation still works correctly without the chain code:
🏁 Script executed:
#!/bin/bash # Check how DeriveAddress handles empty chain code rg -A 3 -B 3 "DeriveAddress" plugin/commonLength of output: 96
🏁 Script executed:
#!/bin/bash set -e # Locate DeriveAddress definition across the entire repository rg -n "func DeriveAddress" -C 3 .Length of output: 473
🏁 Script executed:
#!/bin/bash # Show full DeriveAddress implementation for review rg -C 20 "func DeriveAddress" common/util.goLength of output: 1149
🏁 Script executed:
#!/bin/bash set -e # Locate GetDerivedPubKey definition across the repository rg -n "func GetDerivedPubKey" -C 10 .Length of output: 48
Validate address derivation with empty hexChainCode
The change in plugin/dca/dca.go (lines 331–333) now passes an empty string as the chain code to common.DeriveAddress. Since DeriveAddress calls tss.GetDerivedPubKey(compressedPubKeyHex, hexChainCode, derivePath, false), we need to ensure this won’t break key/address derivation.
Please verify:
- tss.GetDerivedPubKey’s behavior when hexChainCode is an empty string (it must handle or default this case).
- That no errors are returned and the correct address is derived in your integration tests.
- Whether the TODO (“hex chain code can be retrieved from vault”) should be addressed now to avoid future regressions.
storage/postgres/time_trigger.go (1)
39-48: LGTM - Type changes properly implementedThe implementation correctly updates all method signatures to use
uuid.UUIDfor policy IDs, aligning with the interface changes and theTimeTriggerstruct definition. The SQL queries and parameter usage are correctly handled.Also applies to: 98-111, 113-134, 136-157, 159-172
| // TODO: need to deal with both DCA and Payroll plugins | ||
| keyPath := filepath.Join("plugin", pluginType, "dcaPluginUiSchema.json") |
There was a problem hiding this comment.
Schema file path is hardcoded for DCA regardless of plugin type.
The code always uses the DCA schema file path even when handling different plugin types, which contradicts the TODO comment above.
// TODO: need to deal with both DCA and Payroll plugins
-keyPath := filepath.Join("plugin", pluginType, "dcaPluginUiSchema.json")
+// Determine schema file based on plugin type
+var schemaFileName string
+switch pluginType {
+case "dca":
+ schemaFileName = "dcaPluginUiSchema.json"
+case "payroll":
+ schemaFileName = "payrollPluginUiSchema.json"
+default:
+ schemaFileName = "dcaPluginUiSchema.json" // Default fallback
+}
+keyPath := filepath.Join("plugin", pluginType, schemaFileName)📝 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.
| // TODO: need to deal with both DCA and Payroll plugins | |
| keyPath := filepath.Join("plugin", pluginType, "dcaPluginUiSchema.json") | |
| // TODO: need to deal with both DCA and Payroll plugins | |
| // Determine schema file based on plugin type | |
| var schemaFileName string | |
| switch pluginType { | |
| case "dca": | |
| schemaFileName = "dcaPluginUiSchema.json" | |
| case "payroll": | |
| schemaFileName = "payrollPluginUiSchema.json" | |
| default: | |
| schemaFileName = "dcaPluginUiSchema.json" // Default fallback | |
| } | |
| keyPath := filepath.Join("plugin", pluginType, schemaFileName) |
Summary by CodeRabbit