This repository was archived by the owner on Feb 8, 2026. It is now read-only.
Conversation
Contributor
|
Warning Rate limit exceeded@webpiratt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 17 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 ignored due to path filters (1)
📒 Files selected for processing (3)
## Walkthrough
This update removes the `SignPluginMessages` API endpoint and its handler, restructures plugin and worker configuration to include a verifier service with URL and token, and updates plugin logic to interact with the verifier for signing operations. Several function signatures and struct fields are adjusted to accommodate these changes, and configuration formats are updated accordingly.
## Changes
| File(s) | Change Summary |
|-----------------------------------------|-------------------------------------------------------------------------------------------------------|
| api/plugin.go, api/server.go | Removed `SignPluginMessages` method and its route; cleaned up related imports. |
| cmd/fees/server/main.go, cmd/fees/worker/main.go | Reformatted `fees.NewFeePlugin` invocation for readability; no logic changes. |
| cmd/payroll/server/main.go, cmd/payroll/worker/main.go | Changed `payroll.NewPayrollPlugin` to `payroll.NewPlugin` with updated arguments and added signer; removed unused inspector. |
| cmd/payroll/worker/config.go | Added `Rpc` and `Verifier` fields to `PayrollWorkerConfig`; introduced new `Rpc`, `rpcItem`, and updated `verifier` structs. |
| common/util.go | Removed temporary plugin/verifier party ID constants. |
| etc/vultisig/payroll.yml, plugin/payroll/config.go, plugin/payroll/constants.go | Removed payroll config file, constants, and plugin config; restructured config with nested RPC and verifier sections. |
| internal/libhttp/libhttp.go | Added generic `Call` function for flexible HTTP requests and typed responses. |
| internal/verifierapi/fees.go, internal/verifierapi/verifierapi.go | Changed `VerifierApi` methods to pointer receivers; renamed URL field to lowercase; updated constructor and methods. |
| internal/keysign/api_emitter.go | Added `apiEmitter` struct and `NewVerifierEmitter` to emit signing requests via HTTP API with auth token. |
| internal/keysign/plugin_emitter.go | Added `PluginEmitter` struct to enqueue signing tasks asynchronously via asynq client. |
| internal/keysign/signer.go | Introduced `Signer` type coordinating multi-party signing sessions with emitters and relay client. |
| plugin/dca/dca.go | Removed use of static party IDs in `PluginKeysignRequest` construction. |
| plugin/fees/fees.go | Reformatted `verifierApi` instantiation call; no logic change. |
| plugin/payroll/payroll.go | Renamed `PayrollPlugin` to `Plugin`; removed config and inspector fields; added `signer` field; updated constructor to accept `signer` and `ethRpc`. |
| plugin/payroll/transaction.go | Refactored signing flow to use `signer.Sign` directly; updated to support multiple recipients per rule; changed helper function signature. |
| payroll.worker.example.json | Updated config example by adding `verifier` and `rpc` sections; removed `base_file_path`. |
| Dockerfile.Payroll.server, Dockerfile.Payroll.worker | Removed copying of `payroll.yml` config file. |
| .run/[git] payroll server.run.xml, .run/[git] payroll worker.run.xml | Updated `DATABASE_DSN` environment variable to use `vultisig-payroll` database. |
| test/e2e/keygen_test.go, test/keys/iPad/.gitignore, test/keys/iPhone/.gitignore, test/test-2of2-vault-backups/* | Removed test files and ignore rules related to keygen and vault backups. |
| internal/scheduler/scheduler.go | Improved error messages for scheduler initialization and trigger creation; reordered imports. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Plugin
participant VerifierApi
participant VerifierService
Plugin->>VerifierApi: Sign(ctx, PluginKeysignRequest)
VerifierApi->>VerifierService: POST /plugin-signer/sign (with Bearer token)
VerifierService-->>VerifierApi: Task ID or error
VerifierApi-->>Plugin: Task ID or errorPossibly related PRs
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
internal/libhttp/libhttp.go (3)
20-23: Handle nil/empty body gracefully.The function always marshals the body to JSON, which could cause issues if the body is nil or shouldn't be JSON. Consider handling empty bodies separately.
- b, err := json.Marshal(body) - if err != nil { - return *new(T), fmt.Errorf("json.Marshal: %w", err) - } + var b []byte + if body != nil { + var err error + b, err = json.Marshal(body) + if err != nil { + return *new(T), fmt.Errorf("json.Marshal: %w", err) + } + }
43-46: Consider using a configurable HTTP client.Using
http.DefaultClientdoesn't allow for timeout configuration or other customizations. Consider accepting an HTTP client as a parameter or using a client with reasonable defaults.- res, err := http.DefaultClient.Do(req) + client := &http.Client{ + Timeout: 30 * time.Second, + } + res, err := client.Do(req)
59-63: Improve string type detection.The current approach to detect string response types using type assertion is somewhat indirect. Consider using type switches or reflection for cleaner type detection.
- _, isString := any(new(T)).(string) - if isString { - // for string response type no need to unmarshal JSON - return any(string(bodyBytes)).(T), nil - } + var zero T + if _, ok := any(zero).(string); ok { + // for string response type no need to unmarshal JSON + return any(string(bodyBytes)).(T), nil + }etc/vultisig/payroll.yml (1)
6-8: Verify verifier token for production deployment.The verifier configuration looks correct for local development. Ensure that production deployments use secure, environment-specific tokens rather than the "localhost-apikey" placeholder.
plugin/payroll/transaction.go (1)
91-96: The verifier Sign response is not utilized.The call to
p.verifier.Sign(ctx, req)returns a response that could contain important information (like a task ID), but it's currently discarded. Consider using the response for tracking or logging purposes.Consider utilizing the response:
- _, e = p.verifier.Sign(ctx, req) + taskID, e := p.verifier.Sign(ctx, req) if e != nil { p.logger.WithError(e).Error("failed to make Sign request with verifier") return fmt.Errorf("p.verifier.Sign: %w", e) } + p.logger.WithField("verifier_task_id", taskID).Info("verifier sign request submitted")internal/verifierapi/verifierapi.go (1)
57-75: Well-implemented Sign method with proper error handling.The Sign method correctly implements authenticated HTTP requests with proper error handling and logging. The use of
libhttp.Callprovides consistent HTTP client behavior.Consider making the endpoint path configurable:
func (v *VerifierApi) Sign(ctx context.Context, req types.PluginKeysignRequest) (string, error) { - endpoint := v.url + "/plugin-signer/sign" + endpoint := v.url + "/plugin-signer/sign" // TODO: make endpoint path configurable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
api/plugin.go(0 hunks)api/server.go(0 hunks)cmd/fees/server/main.go(1 hunks)cmd/fees/worker/main.go(1 hunks)cmd/payroll/server/main.go(1 hunks)cmd/payroll/worker/config.go(2 hunks)cmd/payroll/worker/main.go(1 hunks)common/util.go(0 hunks)etc/vultisig/payroll.yml(1 hunks)internal/libhttp/libhttp.go(1 hunks)internal/verifierapi/fees.go(3 hunks)internal/verifierapi/verifierapi.go(2 hunks)plugin/dca/dca.go(1 hunks)plugin/fees/fees.go(2 hunks)plugin/payroll/config.go(1 hunks)plugin/payroll/payroll.go(4 hunks)plugin/payroll/transaction.go(3 hunks)
💤 Files with no reviewable changes (3)
- api/server.go
- common/util.go
- api/plugin.go
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
Learnt from: webpiratt
PR: vultisig/plugin#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: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
cmd/payroll/server/main.go (5)
Learnt from: webpiratt
PR: vultisig/plugin#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: 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.
Learnt from: webpiratt
PR: vultisig/plugin#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: johnnyluo
PR: vultisig/plugin#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: vultisig/plugin#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.
cmd/payroll/worker/main.go (6)
Learnt from: webpiratt
PR: vultisig/plugin#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: vultisig/plugin#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: 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.
Learnt from: RaghavSood
PR: vultisig/plugin#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: vultisig/plugin#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: vultisig/plugin#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.
cmd/fees/server/main.go (9)
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.005Z
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: vultisig/plugin#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.
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#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: vultisig/plugin#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.
Learnt from: webpiratt
PR: vultisig/plugin#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: johnnyluo
PR: vultisig/plugin#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: vultisig/plugin#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.
cmd/payroll/worker/config.go (5)
Learnt from: johnnyluo
PR: vultisig/plugin#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: vultisig/plugin#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: 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.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#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.
etc/vultisig/payroll.yml (4)
Learnt from: johnnyluo
PR: vultisig/plugin#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: webpiratt
PR: vultisig/plugin#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: 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.
Learnt from: johnnyluo
PR: vultisig/plugin#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/dca/dca.go (4)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: webpiratt
PR: vultisig/plugin#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: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
internal/verifierapi/fees.go (4)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.005Z
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: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: garry-sharp
PR: vultisig/plugin#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.
cmd/fees/worker/main.go (10)
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.005Z
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: vultisig/plugin#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.
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#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: vultisig/plugin#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: 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.
Learnt from: johnnyluo
PR: vultisig/plugin#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: vultisig/plugin#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: RaghavSood
PR: vultisig/plugin#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.
plugin/payroll/payroll.go (7)
Learnt from: webpiratt
PR: vultisig/plugin#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: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#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: vultisig/plugin#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: vultisig/plugin#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: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: johnnyluo
PR: vultisig/plugin#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.
plugin/payroll/transaction.go (5)
Learnt from: webpiratt
PR: vultisig/plugin#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: 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.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#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.
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
plugin/payroll/config.go (8)
Learnt from: webpiratt
PR: vultisig/plugin#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: 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.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#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: johnnyluo
PR: vultisig/plugin#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: vultisig/plugin#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: vultisig/plugin#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: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
plugin/fees/fees.go (6)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.005Z
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: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: garry-sharp
PR: vultisig/plugin#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.
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.
Learnt from: johnnyluo
PR: vultisig/plugin#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/verifierapi/verifierapi.go (1)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
🧬 Code Graph Analysis (6)
cmd/payroll/worker/main.go (1)
plugin/payroll/payroll.go (1)
NewPayrollPlugin(33-76)
internal/verifierapi/fees.go (1)
internal/verifierapi/verifierapi.go (1)
VerifierApi(28-32)
plugin/payroll/payroll.go (1)
internal/verifierapi/verifierapi.go (2)
VerifierApi(28-32)NewVerifierApi(34-40)
plugin/payroll/transaction.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/fees.go (1)
internal/verifierapi/verifierapi.go (1)
NewVerifierApi(34-40)
internal/verifierapi/verifierapi.go (2)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)internal/libhttp/libhttp.go (1)
Call(13-72)
🔇 Additional comments (21)
plugin/dca/dca.go (1)
344-346: LGTM: Party IDs removal aligns with refactor objectives.The change from hardcoded party identifiers to an empty slice is consistent with the broader refactor that removes temporary placeholder party IDs and moves signing responsibility to the verifier API. The
PluginIDandPolicyIDfields are properly set using the policy data.cmd/payroll/worker/main.go (1)
84-92: LGTM: Improved code formatting.The multi-line formatting with trailing comma improves readability and makes future modifications easier. This follows Go formatting best practices.
cmd/payroll/server/main.go (1)
74-82: LGTM: Consistent formatting improvement.The multi-line formatting matches the style used in the worker file and improves code readability. The trailing comma facilitates future modifications.
etc/vultisig/payroll.yml (1)
3-5: LGTM: Improved configuration structure.The nested RPC configuration is more organized and extensible than the flat structure. This aligns with the broader refactor to support multiple chains and structured configuration.
cmd/payroll/worker/config.go (2)
16-16: Configuration integration looks good.The addition of the
Verifierfield to thePayrollWorkerConfigstruct follows the established pattern and supports the new verifier service integration.
29-32: Well-structured verifier configuration.The
verifierstruct is properly defined with appropriate field tags for configuration mapping and JSON serialization.cmd/fees/worker/main.go (1)
95-107: Multi-line formatting improves readability.The refactoring to multi-line format makes the function call more readable and maintainable.
cmd/fees/server/main.go (1)
81-93: Consistent formatting with worker implementation.The multi-line formatting is consistent with the worker implementation and improves code readability. The empty token placeholder follows the same pattern as the worker.
internal/verifierapi/fees.go (3)
30-30: Improved receiver type consistency.Changing from value receiver to pointer receiver is a good practice for the
VerifierApistruct, which contains multiple fields includingurl,token, andlogger. This avoids unnecessary copying and maintains consistency with other methods in the package.
62-62: Consistent pointer receiver pattern.The pointer receiver change maintains consistency across all
VerifierApimethods.
93-93: Completes the receiver type standardization.All three methods now use pointer receivers, ensuring consistent behavior across the
VerifierApiinterface.plugin/payroll/payroll.go (4)
9-9: Proper import addition for verifier integration.The import of
verifierapipackage is correctly added to support the new verifier functionality.
22-22: Verifier field properly added to plugin struct.The
verifierfield is appropriately added to thePayrollPluginstruct with the correct type*verifierapi.VerifierApi.
50-50: Configuration structure update handled correctly.The change from
cfg.RpcURLtocfg.Rpc.Ethereum.URLindicates a restructuring to nested configuration, which improves organization and supports future multi-chain expansion.
62-66: Verifier initialization follows best practices.The verifier is properly initialized using
verifierapi.NewVerifierApiwith the configuration values and a scoped logger. This follows the constructor pattern defined in the verifierapi package.plugin/payroll/transaction.go (1)
238-337: Excellent refactoring to support multiple recipients per rule.The refactoring to handle multiple recipients per rule is well-implemented with proper concurrency control using errgroup. The goroutine-based approach ensures each recipient is processed independently while maintaining thread safety with the mutex.
plugin/fees/fees.go (2)
57-57: LGTM: Clean integration of verifier token parameter.The addition of the
verifierTokenparameter to the constructor is properly integrated and enables authenticated communication with the verifier service.
85-89: LGTM: Proper verifier API initialization with token.The verifier API initialization correctly passes the token parameter, enabling authenticated requests to the verifier service.
internal/verifierapi/verifierapi.go (3)
29-30: LGTM: Proper encapsulation with token field addition.The addition of the
tokenfield and making theurlfield lowercase improves encapsulation and enables authenticated requests to the verifier service.
34-40: LGTM: Constructor properly updated for token support.The constructor correctly accepts and initializes the token parameter, enabling authenticated verifier API interactions.
42-55: LGTM: Receiver consistency improved.Changing the receiver from value to pointer improves consistency and allows for potential future modifications to the struct state.
webpiratt
commented
Jul 11, 2025
RaghavSood
previously approved these changes
Jul 11, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/payroll/worker/config.go (3)
16-17: Inconsistent field naming convention.The field
Rpcis capitalized whileverifieris not. For consistency with Go conventions and the rest of the struct fields, consider using eitherRPCandVerifierorrpcandverifier.Apply this diff for consistency:
- Rpc Rpc `mapstructure:"Rpc" json:"Rpc,omitempty"` - Verifier verifier `mapstructure:"verifier" json:"verifier,omitempty"` + RPC RPC `mapstructure:"rpc" json:"rpc,omitempty"` + Verifier Verifier `mapstructure:"verifier" json:"verifier,omitempty"`
29-35: Type names should follow Go naming conventions for acronyms.According to Go conventions, acronyms in type names should be capitalized. Consider renaming
RpctoRPCandrpcItemtoRPCItem.Apply this diff to follow Go naming conventions:
-type Rpc struct { - Ethereum rpcItem `mapstructure:"ethereum" json:"ethereum,omitempty"` +type RPC struct { + Ethereum RPCItem `mapstructure:"ethereum" json:"ethereum,omitempty"` } -type rpcItem struct { +type RPCItem struct { URL string `mapstructure:"url" json:"url,omitempty"` }
37-41: Type name should be capitalized.The type
verifiershould beVerifierto follow Go naming conventions for exported types.Apply this diff:
-type verifier struct { +type Verifier struct { URL string `mapstructure:"url"` Token string `mapstructure:"token"` PartyPrefix string `mapstructure:"party_prefix"` }internal/keysign/api_emitter.go (1)
38-44: Consider logging API response for debugging.The response from the API call is ignored. While this works for the happy path, the response might contain useful error details or request IDs that could help with debugging failures.
Consider logging the response:
func (e *apiEmitter[T]) Sign(ctx context.Context, req types.PluginKeysignRequest) error { - _, err := libhttp.Call[T](ctx, e.method, e.endpoint, e.headers, req, nil) + resp, err := libhttp.Call[T](ctx, e.method, e.endpoint, e.headers, req, nil) if err != nil { return fmt.Errorf("libhttp.Call: %w", err) } + // Log response if needed for debugging + // logrus.WithField("response", resp).Debug("API emitter response") return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cmd/payroll/server/main.go(1 hunks)cmd/payroll/worker/config.go(2 hunks)cmd/payroll/worker/main.go(2 hunks)etc/vultisig/payroll.yml(0 hunks)internal/keysign/api_emitter.go(1 hunks)internal/keysign/plugin_emitter.go(1 hunks)internal/keysign/signer.go(1 hunks)internal/verifierapi/verifierapi.go(1 hunks)payroll.worker.example.json(1 hunks)plugin/payroll/config.go(0 hunks)plugin/payroll/constants.go(0 hunks)plugin/payroll/payroll.go(2 hunks)plugin/payroll/transaction.go(4 hunks)
💤 Files with no reviewable changes (3)
- etc/vultisig/payroll.yml
- plugin/payroll/constants.go
- plugin/payroll/config.go
✅ Files skipped from review due to trivial changes (1)
- payroll.worker.example.json
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/verifierapi/verifierapi.go
- cmd/payroll/server/main.go
- plugin/payroll/transaction.go
- plugin/payroll/payroll.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
Learnt from: webpiratt
PR: vultisig/plugin#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: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
cmd/payroll/worker/main.go (10)
Learnt from: webpiratt
PR: vultisig/plugin#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: vultisig/plugin#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: vultisig/plugin#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: 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.
Learnt from: webpiratt
PR: vultisig/plugin#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.
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: johnnyluo
PR: vultisig/plugin#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: vultisig/plugin#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.
cmd/payroll/worker/config.go (5)
Learnt from: johnnyluo
PR: vultisig/plugin#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: webpiratt
PR: vultisig/plugin#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: johnnyluo
PR: vultisig/plugin#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: 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.
Learnt from: webpiratt
PR: vultisig/plugin#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/keysign/api_emitter.go (1)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
internal/keysign/plugin_emitter.go (1)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
🧬 Code Graph Analysis (3)
cmd/payroll/worker/main.go (6)
cmd/payroll/worker/config.go (1)
Rpc(29-31)plugin/payroll/payroll.go (1)
NewPayrollPlugin(31-63)internal/keysign/signer.go (2)
NewSigner(30-47)Emitter(19-21)internal/keysign/api_emitter.go (1)
NewVerifierEmitter(12-21)internal/keysign/plugin_emitter.go (1)
NewPluginEmitter(19-25)internal/tasks/tasks.go (2)
TypeKeySignDKLS(13-13)QUEUE_NAME(9-9)
internal/keysign/api_emitter.go (3)
internal/keysign/signer.go (1)
Emitter(19-21)internal/types/keysign.go (1)
PluginKeysignRequest(7-12)internal/libhttp/libhttp.go (1)
Call(13-72)
internal/keysign/plugin_emitter.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (5)
cmd/payroll/worker/main.go (3)
8-11: LGTM! Clean import organization.The new imports for Ethereum client and keysign package are properly organized and necessary for the refactored signing architecture.
88-91: Ethereum RPC client initialization looks good.Proper error handling and clear error message for RPC client creation failure.
93-112: Complex but correct PayrollPlugin initialization with new Signer architecture.The initialization properly sets up the keysign.Signer with both verifier and plugin emitters, enabling the dual signing path architecture. All parameters are correctly sourced from the configuration.
internal/keysign/signer.go (2)
99-119: Review error handling in signature collection loop.The
continuestatement on line 108 continues the inner loop but doesn't affect the outer polling loop. IfCheckKeysignCompleteconsistently fails for one message, the method will keep polling indefinitely without returning the partial results or aggregating errors.Consider whether partial failures should be handled differently:
sigs := make(map[string]tss.KeysignResponse, len(messageIDs)) + var errors []error for _, messageID := range messageIDs { sig, completeErr := s.relay.CheckKeysignComplete(sessionID, messageID) if completeErr != nil { s.logger.WithFields(logrus.Fields{ "sessionID": sessionID, "messageID": messageID, "partyIDs": partyIDs, }).WithError(completeErr).Info("continue polling: CheckKeysignComplete") - continue + errors = append(errors, completeErr) + break // Break inner loop to retry in outer loop } if sig == nil { return nil, fmt.Errorf( "unexpected empty sig: messageID: %s, sessionID: %s", messageID, sessionID, ) } sigs[messageID] = *sig } + if len(errors) > 0 { + continue // Retry in outer loop + } return sigs, nil
49-75: Well-designed signing coordination implementation.The
Signmethod properly orchestrates the multi-party signing process by:
- Emitting requests to all configured emitters
- Waiting for parties to join with proper prefix filtering
- Collecting results with context-aware polling
Excellent use of context for cancellation support.
webpiratt
marked this pull request as draft
July 12, 2025 23:29
johnnyluo
reviewed
Jul 17, 2025
# Conflicts: # go.mod # go.sum
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
webpiratt
marked this pull request as ready for review
July 19, 2025 11:49
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/keysign/signer.go (1)
182-187: Fix grammar in error message.There's a grammatical error in the error message.
Apply this diff:
return nil, fmt.Errorf( "too many parties joined: [%s], expected prefixes: [%s],"+ - " it may be caused by a bug in calling code", + " it may be caused by a bug in the calling code", strings.Join(partiesIDs, ","), strings.Join(partiesPrefixes, ","), )
🧹 Nitpick comments (1)
plugin/payroll/transaction.go (1)
88-97: Simplify single signature extractionThe current approach uses a loop to extract a single signature from the map. Consider a more direct approach.
if len(sigs) != 1 { p.logger. WithField("sigs_count", len(sigs)). Error("expected only 1 message+sig per request for evm") return fmt.Errorf("failed to sign transaction: invalid signature count: %d", len(sigs)) } - var sig tss.KeysignResponse - for _, s := range sigs { - sig = s - } + // Extract the single signature from the map + var sig tss.KeysignResponse + for _, s := range sigs { + sig = s + break + }Or even better, since we know there's exactly one message:
+ // Get the signature for the single message + sig, ok := sigs[req.Messages[0].Hash] + if !ok { + return fmt.Errorf("signature not found for message hash") + }
📜 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 (26)
.run/[git] payroll server.run.xml(2 hunks).run/[git] payroll worker.run.xml(2 hunks)Dockerfile.Payroll.server(1 hunks)Dockerfile.Payroll.worker(1 hunks)cmd/fees/server/main.go(1 hunks)cmd/fees/worker/main.go(1 hunks)cmd/payroll/server/main.go(1 hunks)cmd/payroll/worker/main.go(2 hunks)go.mod(1 hunks)internal/keysign/api_emitter.go(1 hunks)internal/keysign/plugin_emitter.go(1 hunks)internal/keysign/signer.go(1 hunks)internal/libhttp/libhttp.go(1 hunks)internal/scheduler/scheduler.go(5 hunks)internal/verifierapi/verifierapi.go(1 hunks)payroll.worker.example.json(1 hunks)plugin/fees/fees.go(1 hunks)plugin/payroll/payroll.go(2 hunks)plugin/payroll/policy.go(2 hunks)plugin/payroll/transaction.go(12 hunks)test/e2e/keygen_test.go(0 hunks)test/keys/iPad/.gitignore(0 hunks)test/keys/iPhone/.gitignore(0 hunks)test/test-2of2-vault-backups/Fast Vault #2-0985-part1of2-Vultiserver.vult(0 hunks)test/test-2of2-vault-backups/Fast Vault #2-0985-part2of2.vult(0 hunks)test/test-2of2-vault-backups/Vault Info.txt(0 hunks)
💤 Files with no reviewable changes (6)
✅ Files skipped from review due to trivial changes (7)
- Dockerfile.Payroll.worker
- Dockerfile.Payroll.server
- .run/[git] payroll server.run.xml
- .run/[git] payroll worker.run.xml
- plugin/payroll/policy.go
- internal/scheduler/scheduler.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/fees/server/main.go
- cmd/payroll/server/main.go
- payroll.worker.example.json
- plugin/fees/fees.go
- cmd/fees/worker/main.go
- internal/verifierapi/verifierapi.go
- cmd/payroll/worker/main.go
- internal/libhttp/libhttp.go
- internal/keysign/api_emitter.go
- internal/keysign/plugin_emitter.go
- plugin/payroll/payroll.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
Learnt from: webpiratt
PR: vultisig/plugin#127
File: internal/keysign/api_emitter.go:12-21
Timestamp: 2025-07-19T11:09:26.168Z
Learning: In the vultisig/plugin codebase, URL formatting and validation for endpoints like the verifier emitter is handled at the configuration level, so additional URL validation in individual functions is not needed.
Learnt from: webpiratt
PR: vultisig/plugin#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/keysign/signer.go (1)
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
plugin/payroll/transaction.go (8)
Learnt from: webpiratt
PR: vultisig/plugin#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: 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.
Learnt from: webpiratt
PR: vultisig/plugin#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: vultisig/plugin#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: vultisig/plugin#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.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
webpiratt
enabled auto-merge
July 19, 2025 12:28
RaghavSood
approved these changes
Jul 19, 2025
RaghavSood
left a comment
RaghavSood
left a comment
Contributor
There was a problem hiding this comment.
One nit, non-blocking
I believe a good portion of this should be split up and probably go into verifier or vultisig-go, but that is not a blocker for getting this in and keeping things moving first
Amazing effort, thank you @webpiratt
|
|
||
| type PayrollWorkerConfig struct { | ||
| Redis storage.RedisConfig `mapstructure:"redis" json:"redis,omitempty"` | ||
| Rpc Rpc `mapstructure:"Rpc" json:"Rpc,omitempty"` |
Contributor
There was a problem hiding this comment.
Suggested change
| Rpc Rpc `mapstructure:"Rpc" json:"Rpc,omitempty"` | |
| Rpc Rpc `mapstructure:"rpc" json:"rpc,omitempty"` |
Contributor
Author
There was a problem hiding this comment.
Thank you, PR already auto-merged, I'll put this typo fix in next PR, seems viper loads it as case-insensitive - I didn't notice because config loaded fine
This was referenced Jul 30, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ref: #124 , vultisig/verifier#266
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Documentation