⚡️ Tidied up config. Moved validation and recipe specs to policy.go#117
⚡️ Tidied up config. Moved validation and recipe specs to policy.go#117garry-sharp merged 1 commit intomainfrom
Conversation
WalkthroughThis change refactors the fee plugin’s configuration and initialization flow. It simplifies configuration structures, enforces collector whitelisting, introduces a maximum fee amount, and updates the plugin type constant. The fee plugin now validates policy documents and provides a formal recipe specification, with the verifier URL passed explicitly during instantiation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FeeServer
participant FeePlugin
participant VerifierAPI
User->>FeeServer: Start service
FeeServer->>FeePlugin: Initialize with file-based config, verifier URL
FeePlugin->>VerifierAPI: Instantiate if verifier URL provided
FeeServer-->>User: Service ready
sequenceDiagram
participant PolicyManager
participant FeePlugin
PolicyManager->>FeePlugin: ValidatePluginPolicy(policyDoc)
FeePlugin->>FeePlugin: Decode and unmarshal policy recipe
FeePlugin->>FeePlugin: Check plugin ID, rule ID, resource, and constraints
FeePlugin-->>PolicyManager: Return validation result
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"" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the fee plugin’s configuration loading and policy handling, centralizing validation and recipe specification logic in policy.go and cleaning up duplicate methods and outdated defaults.
- Consolidated
ValidatePluginPolicyandGetRecipeSpecificationintopolicy.go, removing duplicates fromfees.go - Overhauled
FeeConfiginconfig.gowith new defaults, whitelist validation, and file-based loading - Updated plugin initialization in worker and server commands to use the new config and include
verifierUrl
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/fees/policy.go | Added full implementation for policy validation and recipe specification |
| plugin/fees/fees.go | Removed duplicate methods, updated NewFeePlugin signature |
| plugin/fees/config.go | Refactored FeeConfig, default values, validation, and file loader |
| plugin/fees/constraints.go | Renamed PLUGIN_TYPE from "fees" to "fee" |
| fee.worker.example.json | Renamed base_file_path to base_config_path |
| etc/vultisig/fee.yml | Provided updated example with whitelist and collector address |
| docker-compose.dev.yaml | Mounted new fee.yml for worker and server services |
| cmd/fees/worker/main.go | Updated to use WithFileConfig and new NewFeePlugin signature |
| cmd/fees/server/main.go | Updated to use WithFileConfig and new NewFeePlugin signature |
Comments suppressed due to low confidence (1)
plugin/fees/config.go:79
- The
WithFileConfigfunction sets up Viper but never callsReadInConfigorUnmarshalto load the file intoFeeConfig. Addv.ReadInConfig()andv.Unmarshal(c)so the config struct is actually populated from the file.
func WithFileConfig(basePath string) ConfigOption {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugin/fees/policy.go (1)
43-48: Consider making hardcoded values configurableThe rule ID
"allow-usdc-transfer-to-collector"and resource are hardcoded, which limits flexibility. Consider making these configurable or at least defining them as constants to improve maintainability.Consider defining these as constants at the package level:
const ( AllowedRuleID = "allow-usdc-transfer-to-collector" AllowedResource = "ethereum.erc20.transfer" )Then use them in the validation:
- if rule.Id != "allow-usdc-transfer-to-collector" { - return fmt.Errorf("rule id must be allow-usdc-transfer-to-collector") + if rule.Id != AllowedRuleID { + return fmt.Errorf("rule id must be %s", AllowedRuleID)plugin/fees/config.go (1)
57-63: Address the TODO comment for collector address validationThe TODO indicates that address validation is needed. This should validate that the address is a valid Ethereum address format.
Would you like me to implement the address validation logic or create an issue to track this task?
Here's a suggested implementation:
+import "github.com/ethereum/go-ethereum/common" func WithCollectorAddress(collectorAddress string) ConfigOption { - // TODO garry. Verify address whitelisted. Verify address is valid. return func(c *FeeConfig) error { + if !common.IsHexAddress(collectorAddress) { + return fmt.Errorf("invalid collector address format: %s", collectorAddress) + } c.CollectorAddress = collectorAddress return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/fees/server/main.go(1 hunks)cmd/fees/worker/main.go(1 hunks)docker-compose.dev.yaml(2 hunks)etc/vultisig/fee.yml(1 hunks)fee.worker.example.json(1 hunks)plugin/fees/config.go(4 hunks)plugin/fees/constraints.go(1 hunks)plugin/fees/fees.go(2 hunks)plugin/fees/policy.go(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
fee.worker.example.json (1)
Learnt from: garry-sharp
PR: vultisig/plugin#113
File: docker-compose.yaml:80-81
Timestamp: 2025-07-03T15:55:04.565Z
Learning: For docker-compose.yaml files used for local development builds, hardcoded database credentials like "myuser:mypassword" in DATABASE_DSN environment variables are acceptable since they're only used locally and not exposed to external networks.
plugin/fees/constraints.go (2)
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.
docker-compose.dev.yaml (1)
Learnt from: garry-sharp
PR: vultisig/plugin#113
File: docker-compose.yaml:80-81
Timestamp: 2025-07-03T15:55:04.565Z
Learning: For docker-compose.yaml files used for local development builds, hardcoded database credentials like "myuser:mypassword" in DATABASE_DSN environment variables are acceptable since they're only used locally and not exposed to external networks.
cmd/fees/worker/main.go (3)
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: 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 (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: 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#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.
plugin/fees/config.go (1)
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/fees/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: 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.
plugin/fees/policy.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)
plugin/fees/config.go (1)
plugin/fees/constraints.go (1)
PLUGIN_TYPE(3-3)
plugin/fees/fees.go (3)
storage/db.go (1)
DatabaseStorage(15-36)plugin/fees/config.go (1)
FeeConfig(30-37)internal/verifierapi/verifierapi.go (1)
NewVerifierApi(29-34)
plugin/fees/policy.go (2)
plugin/fees/fees.go (1)
FeePlugin(34-47)service/policy.go (1)
Policy(16-22)
🔇 Additional comments (7)
fee.worker.example.json (1)
29-29: Configuration key rename looks consistent with refactoring.The rename from
base_file_pathtobase_config_pathaligns with the updated configuration loading approach mentioned in the AI summary.docker-compose.dev.yaml (1)
16-16: Volume mounts correctly support new file-based configuration.The new volume mounts for
fee.ymlconfiguration file are properly configured for both fee-worker and fee-server services, enabling the new file-based configuration approach.Also applies to: 32-32
etc/vultisig/fee.yml (1)
1-8: Fee configuration validated and approved
- RPC URL “https://ethereum.publicnode.com/” responded with HTTP 200.
- Both collector whitelist addresses are valid, checksummed Ethereum addresses.
collector_addressis correctly included in the whitelist.No further changes needed; this configuration is ready to merge.
cmd/fees/worker/main.go (1)
85-90: Constructor signature validation passedThe
NewFeePluginconstructor inplugin/fees/fees.go(lines 49–58) exactly matches the invocation incmd/fees/worker/main.go(lines 85–90). All ten parameters are in the correct order and type, andWithFileConfig(cfg.BaseConfigPath)correctly returns aConfigOption. No further changes are required.cmd/fees/server/main.go (1)
76-81: Configuration refactoring looks good!The changes correctly align with the new configuration structure where:
- Fee config is loaded from a file (
fee.yml) instead of programmatically setting the verifier URL- The verifier URL is explicitly passed as a parameter to
NewFeePluginplugin/fees/fees.go (1)
49-58: Excellent refactoring of the plugin constructor!The changes improve the design by:
- Making the verifier URL an explicit parameter rather than embedding it in config
- Adding proper validation to ensure the verifier URL is not empty
- Maintaining clear separation of concerns
Also applies to: 76-81
plugin/fees/config.go (1)
108-140: Excellent configuration validation!The new
NewFeeConfigconstructor implements comprehensive validation:
- Validates plugin type matches expected value
- Ensures required fields are not empty
- Validates whitelist is not empty
- Ensures collector address is in the whitelist
This significantly improves configuration safety and provides clear error messages.
garry-sharp
force-pushed
the
fee-validation-and-config
branch
from
8314077 to
9e97f93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugin/fees/config.go (2)
31-37: Fix the incomplete comment.The configuration refactoring looks good - the new fields provide clear whitelist functionality and fee amount controls. However, there's a typo in the comment for
CollectorWhitelistAddresses.- CollectorWhitelistAddresses []string `mapstructure:"collector_whitelist_addresses"` // The for which fee transactions are collected. These can include previous addresses. Fee plugins with a recipient address that is not in this list will not be processed. + CollectorWhitelistAddresses []string `mapstructure:"collector_whitelist_addresses"` // The addresses for which fee transactions are collected. These can include previous addresses. Fee plugins with a recipient address that is not in this list will not be processed.
58-58: Consider updating the TODO comment.The TODO comment mentions verifying address whitelisted, but this validation is now properly handled in the
NewFeeConfigfunction (lines 135-137). Consider updating or removing this TODO.- // TODO garry. Verify address whitelisted. Verify address is valid. + // TODO garry. Verify address is valid (checksum, format).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/fees/server/main.go(1 hunks)cmd/fees/worker/main.go(1 hunks)docker-compose.dev.yaml(2 hunks)etc/vultisig/fee.yml(1 hunks)fee.worker.example.json(1 hunks)plugin/fees/config.go(4 hunks)plugin/fees/constraints.go(1 hunks)plugin/fees/fees.go(2 hunks)plugin/fees/policy.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- fee.worker.example.json
- plugin/fees/constraints.go
- docker-compose.dev.yaml
- etc/vultisig/fee.yml
- cmd/fees/worker/main.go
- plugin/fees/policy.go
- plugin/fees/fees.go
- cmd/fees/server/main.go
🧰 Additional context used
🧠 Learnings (1)
plugin/fees/config.go (1)
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.
🧬 Code Graph Analysis (1)
plugin/fees/config.go (1)
plugin/fees/constraints.go (1)
PLUGIN_TYPE(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
plugin/fees/config.go (6)
6-6: LGTM!The addition of the
slicesimport is necessary for theslices.Containsfunction used in the validation logic.
41-48: LGTM!The default values are reasonable:
- Empty whitelist and collector address require explicit configuration
- 500 USDC max fee amount provides a sensible upper bound
- Default RPC URL uses a reliable public endpoint
50-55: LGTM!The simplified function signature aligns with the removal of deprecated USDC address configuration.
65-77: LGTM!The new configuration option functions follow the established pattern and provide clear ways to set the whitelist and maximum fee amount.
83-83: LGTM!The config file name change from "fees" to "fee" correctly aligns with the updated plugin type constant.
108-141: Excellent validation logic with strong security controls.The validation logic is comprehensive and well-implemented:
- Enforces plugin type consistency
- Validates required fields (RpcURL, CollectorAddress)
- Ensures whitelist is not empty (security requirement)
- Validates collector address is whitelisted (prevents misconfiguration)
- Uses appropriate error messages for debugging
The whitelist membership validation using
slices.Containsis efficient and the error message includes both the address and whitelist for easy debugging.
garry-sharp
force-pushed
the
fee-validation-and-config
branch
from
9e97f93 to
97458e8
Compare
| MaxFeeAmount uint64 `mapstructure:"max_fee_amount"` // Policies that are created/submitted which do not have this amount will be rejected. | ||
| } | ||
|
|
||
| type ConfigOption func(*FeeConfig) error |
There was a problem hiding this comment.
you never return error , why not drop it?
Summary by CodeRabbit
New Features
Bug Fixes
Chores